[ovs-dev] [RFC PATCH v2] dpif-netdev: Add port/queue tiebreaker to rxq_cycle_sort.

[aserdean at ovn.org]

> -----Original Message-----
> From: Stokes, Ian [mailto:ian.stokes at intel.com]
> Sent: Thursday, November 23, 2017 3:30 PM
> To: aserdean at ovn.org; 'Kevin Traynor' <ktraynor at redhat.com>; O Mahony,
> Billy <billy.o.mahony at intel.com>; dev at openvswitch.org
> Subject: RE: [ovs-dev] [RFC PATCH v2] dpif-netdev: Add port/queue
> tiebreaker to rxq_cycle_sort.
> 
> > > -----Original Message-----
> > > From: Kevin Traynor [mailto:ktraynor at redhat.com]
> > > Sent: Thursday, November 23, 2017 1:05 PM
> > > To: O Mahony, Billy <billy.o.mahony at intel.com>; dev at openvswitch.org;
> > > aserdean at ovn.org
> > > Subject: Re: [ovs-dev] [RFC PATCH v2] dpif-netdev: Add port/queue
> > > tiebreaker to rxq_cycle_sort.
> > >
> > > On 11/23/2017 10:42 AM, O Mahony, Billy wrote:
> > > > Hi Kevin,
> > > >
> > > > My 2c below..
> > > >
> > > > /Billy
> > > >
> > > >> -----Original Message-----
> > > >> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> > > >> bounces at openvswitch.org] On Behalf Of Kevin Traynor
> > > >> Sent: Wednesday, November 22, 2017 7:11 PM
> > > >> To: dev at openvswitch.org; aserdean at ovn.org
> > > >> Subject: [ovs-dev] [RFC PATCH v2] dpif-netdev: Add port/queue
> > > >> tiebreaker to rxq_cycle_sort.
> > > >>
> > > >> rxq_cycle_sort is used to sort the rx queues by their measured
> > > >> number of
> > > cycles.
> > > >> In the event that they are equal 0 could be returned.
> > > >> However, it is observed that returning 0 results in a different
> > > >> sort order on Windows/Linux. This is ok in practice but it causes
> > > >> a unit test failure for
> > > >> "1007: PMD - pmd-cpu-mask/distribution of rx queues" on Windows.
> > > >>
> > > >> In order to have a consistent sort result, introduce a tiebreaker
> > > >> of
> > > port/queue.
> > > >>
> > > >> Fixes: 655856ef39b9 ("dpif-netdev: Change rxq_scheduling to use
> > > >> rxq processing
> > > >> cycles.")
> > > >> Reported-by: Alin Gabriel Serdean <aserdean at ovn.org>
> > > >> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> > > >> ---
> > > >>
> > > >> v2: Inadvertently reversed the order for non-tiebreak cases in v1.
> > Fix that.
> > > >>
> > > >>  lib/dpif-netdev.c | 15 ++++++++++++---
> > > >>  1 file changed, 12 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > > >> 0a62630..57451e9 100644
> > > >> --- a/lib/dpif-netdev.c
> > > >> +++ b/lib/dpif-netdev.c
> > > >> @@ -3452,4 +3452,5 @@ rxq_cycle_sort(const void *a, const void *b)
> > > >>      uint64_t total_qa, total_qb;
> > > >>      unsigned i;
> > > >> +    int winner = 1;
> > > >>
> > > >>      qa = *(struct dp_netdev_rxq **) a; @@ -3464,8 +3465,16 @@
> > > >> rxq_cycle_sort(const void *a, const void *b)
> > > >>      dp_netdev_rxq_set_cycles(qb, RXQ_CYCLES_PROC_HIST,
> > > >> total_qb);
> > > >>
> > > > [[BO'M]]
> > > > I think it's worth adding a comment to state this compare function
> > > > never
> > > return's 0 and that the reason for this (the sort order difference
> > > on different OSs).
> > > >
> > >
> > > Sure, I will add this for PATCH version.
> > >
> > > > Also the function should probably be called rxq_cycle_compare as
> > > > its really
> > > comparing entries rather than sorting a list of entries And it's
> > > used as the compar arg to qsort.
> > > >
> > >
> > > yeah, I guess it would be a better name. It would be a separate
> > > patch as it's an unrelated change, but I might submit at same time
> > > as making
> > this PATCH.
> > >
> > > thanks,
> > > Kevin.
> > >
> > [Alin Serdean] I applied the patch and the test passes now. Thanks a
> > lot Kevin.
> 
> Thanks for testing on windows Alin, I'll add a Tested-by tag for you on the
> PATCH version at commit if that's ok with you?
Sure 😊.
Alin.
> 
> Ian