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

[Ilya Maximets]

Hi, Kevin.
Thanks for fixing this.

Just a minor comment:
Wouldn't it be more visual if we'll implement it in below way:

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 0a62630..c1c3ed8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3463,10 +3463,17 @@ rxq_cycle_sort(const void *a, const void *b)
     dp_netdev_rxq_set_cycles(qa, RXQ_CYCLES_PROC_HIST, total_qa);
     dp_netdev_rxq_set_cycles(qb, RXQ_CYCLES_PROC_HIST, total_qb);
 
-    if (total_qa >= total_qb) {
-        return -1;
+    if (total_qa != total_qb) {
+        return (total_qa < total_qb) ? 1 : -1;
+    } else {
+        /* Cycles are the same.  Tiebreak on port/queue id. */
+        if (qa->port->port_no != qb->port->port_no) {
+            return (qa->port->port_no < qb->port->port_no) ? 1 : -1;
+        } else {
+            return netdev_rxq_get_queue_id(qb->rx)
+                   - netdev_rxq_get_queue_id(qa->rx);
+        }
     }
-    return 1;
 }

What do you think?

I don't insist, I just don't like additional variables.
Please, ignore this comment, if you don't like it. It's just a matter of style.

Best regards, Ilya Maximets.

> 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);
>  
> -    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
>