On 11/23/2017 02:31 PM, Ilya Maximets wrote:
> 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.
>
hi Ilya, thanks for the suggestion. I agree it looks a lot neater this
way, so I used this construct and added you in the commit tags (let me
know if that's not ok). In case you notice some different logic, it's
because I want to have ascending order for the tiebreaks on port/queues.
Kevin.
> 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
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
