[ovs-dev] [RFC PATCH 1/6] dpif-netdev: Add rxq processing cycle counters.
Kevin Traynor
ktraynor at redhat.com
Fri Jun 9 19:19:36 UTC 2017
On 06/02/2017 06:40 PM, Stokes, Ian wrote:
>> -----Original Message-----
>> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
>> bounces at openvswitch.org] On Behalf Of Kevin Traynor
>> Sent: Friday, May 5, 2017 5:34 PM
>> To: dev at openvswitch.org
>> Subject: [ovs-dev] [RFC PATCH 1/6] dpif-netdev: Add rxq processing cycle
>> counters.
>>
>> Add two counters to dp_netdev_rxq which will be used for storing the
>> processing cycles of the rxq. Processing cycles will stored in reference
>> to a defined interval. One counter is used to count cycles during the
>> current in progress interval, while the other is used to store the cycles
>> of the last complete interval.
>>
>> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
>> ---
>> lib/dpif-netdev.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 7352d6f..d2a02af
>> 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -343,4 +343,6 @@ struct dp_netdev_rxq {
>> particular core. */
>> struct dp_netdev_pmd_thread *pmd; /* pmd thread that will poll this
>> queue. */
>> + atomic_ullong cyc_curr; /* Current pmd interval proc
>> cycles. */
>> + atomic_ullong cyc_last; /* Last pmd interval proc cycles.
>> */
>> };
>>
>> @@ -665,4 +667,14 @@ static void pmd_load_cached_ports(struct
>> dp_netdev_pmd_thread *pmd) static inline void
>> dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd);
>> +static void
>> +dp_netdev_rxq_set_cyc_curr(struct dp_netdev_rxq *rx,
>> + unsigned long long cycles); static uint64_t
>> +dp_netdev_rxq_get_cyc_curr(const struct dp_netdev_rxq *rx); static void
>> +dp_netdev_rxq_set_cyc_last(struct dp_netdev_rxq *rx,
>> + unsigned long long cycles); static
>> +uint64_t dp_netdev_rxq_get_cyc_last(const struct dp_netdev_rxq *rx);
>>
>> static void
>> @@ -3046,4 +3058,35 @@ cycles_count_intermediate(struct
>> dp_netdev_pmd_thread *pmd, }
>>
>> +static void
>> +dp_netdev_rxq_set_cyc_curr(struct dp_netdev_rxq *rx,
>> + unsigned long long cycles) {
>> + atomic_store_relaxed(&rx->cyc_curr, cycles); }
>> +
>> +static uint64_t
>> +dp_netdev_rxq_get_cyc_curr(const struct dp_netdev_rxq *rx) {
>> + unsigned long long tmp;
>> + atomic_read_relaxed(&rx->cyc_curr, &tmp);
>> + return tmp;
>> +}
>> +
>> +static void
>> +dp_netdev_rxq_set_cyc_last(struct dp_netdev_rxq *rx,
>> + unsigned long long cycles) {
>> + atomic_store_relaxed(&rx->cyc_last, cycles); }
>> +
>> +static uint64_t
>> +dp_netdev_rxq_get_cyc_last(const struct dp_netdev_rxq *rx) {
>> + unsigned long long tmp;
>> + atomic_read_relaxed(&rx->cyc_last, &tmp);
>> + return tmp;
>> +}
>> +
> Some code duplication above for the get/set methods above.
>
> I was thinking could you have 1 function for dp_netdev_rxq_set_cyc and 1 for dp_netdev_rxq_get_cyc and use an enum to signal if its 'cyc_last' or 'cyc_curr'. But a concern with this approach is that the logical decision on the enum type will impact negatively on performance.
>
yes, could put them as an array and index the way you've suggested and
it would look a bit neater. I don't think it'll make any noticeable
difference for performance. I'll try it out and see.
> I know this is an RFC so it probably doesn't matter but as a heads up you'll get compilation warnings of the functions not being used as is after applying this patch. Maybe something to consider when rolling out the next patchset.
>
oops, thanks. Seems I have some clean up to do.
> Also clang complains of the following:
>
> lib/dpif-netdev.c:3071:5: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_ullong *' (aka 'const _Atomic(unsigned long long) *') invalid)
> atomic_read_relaxed(&rx->cyc_curr, &tmp);
> ^ ~~~~~~~~~~~~~
> ./lib/ovs-atomic.h:395:5: note: expanded from macro 'atomic_read_relaxed'
> atomic_read_explicit(VAR, DST, memory_order_relaxed)
> ^ ~~~
> ./lib/ovs-atomic-clang.h:53:15: note: expanded from macro 'atomic_read_explicit'
> (*(DST) = __c11_atomic_load(SRC, ORDER), \
> ^ ~~~
> lib/dpif-netdev.c:3086:5: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const atomic_ullong *' (aka 'const _Atomic(unsigned long long) *') invalid)
> atomic_read_relaxed(&rx->cyc_last, &tmp);
> ^ ~~~~~~~~~~~~~
> ./lib/ovs-atomic.h:395:5: note: expanded from macro 'atomic_read_relaxed'
> atomic_read_explicit(VAR, DST, memory_order_relaxed)
> ^ ~~~
> ./lib/ovs-atomic-clang.h:53:15: note: expanded from macro 'atomic_read_explicit'
> (*(DST) = __c11_atomic_load(SRC, ORDER), \
>
> Sparse throws errors for the same issue
>
> lib/dpif-netdev.c:3075:5: warning: incorrect type in argument 1 (different modifiers)
> lib/dpif-netdev.c:3075:5: expected void *<noident>
> lib/dpif-netdev.c:3075:5: got unsigned long long const *<noident>
> lib/dpif-netdev.c:3075:5: warning: incorrect type in argument 1 (different modifiers)
> lib/dpif-netdev.c:3075:5: expected void *<noident>
> lib/dpif-netdev.c:3075:5: got unsigned long long const *<noident>
> lib/dpif-netdev.c:3090:5: warning: incorrect type in argument 1 (different modifiers)
> lib/dpif-netdev.c:3090:5: expected void *<noident>
> lib/dpif-netdev.c:3090:5: got unsigned long long const *<noident>
> lib/dpif-netdev.c:3090:5: warning: incorrect type in argument 1 (different modifiers)
> lib/dpif-netdev.c:3090:5: expected void *<noident>
> lib/dpif-netdev.c:3090:5: got unsigned long long const *<noident>
>> +
>> static int
>> dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd,
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list