On 11/23/2017 10:42 AM, O Mahony, Billy wrote:
> Hi Kevin,
>
> My 2c below..
>
> /Billy
>
>> -----Original Message-----
>> From: [email protected] [mailto:ovs-dev-
>> [email protected]] On Behalf Of Kevin Traynor
>> Sent: Wednesday, November 22, 2017 7:11 PM
>> To: [email protected]; [email protected]
>> 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 <[email protected]>
>> Signed-off-by: Kevin Traynor <[email protected]>
>> ---
>>
>> 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.
>> - if (total_qa >= total_qb) {
>> - return -1;
>> + if (total_qa > total_qb) {
>> + winner = -1;
>> + } else if (total_qa == total_qb) {
>> + /* Cycles are the same. Tiebreak on port/queue id. */
>> + if (qb->port->port_no > qa->port->port_no) {
>> + winner = -1;
>> + } else if (qa->port->port_no == qb->port->port_no) {
>> + winner = netdev_rxq_get_queue_id(qa->rx)
>> + - netdev_rxq_get_queue_id(qb->rx);
>> + }
>> }
>> - return 1;
>> + return winner;
>> }
>>
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
