On 06/02/2017 06:40 PM, Stokes, Ian wrote: >> -----Original Message----- >> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- >> boun...@openvswitch.org] On Behalf Of Kevin Traynor >> Sent: Friday, May 5, 2017 5:34 PM >> To: d...@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 <ktray...@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 >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev