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

Stokes, Ian ian.stokes at intel.com
Fri Nov 24 06:32:57 UTC 2017


> >> -----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.
> 
> Thanks for testing Alin. I've added the Tested-by: tag in the PATCH
> version just sent.
> 
> I've moved these return value changes back into the other balance related
> patchset that I split them out from, as now we know it fixes the Windows
> unit test and as independent patches for master they cause conflicts with
> each other. Grouping them together means I could rebase the series so they
> will all apply in order.

OK
> 
> Kevin.
> 
> >>
> >> Ian
> >
> >



More information about the dev mailing list