[PATCH v3] linux-gen: tm: fix wrr/wfq bug when weight=1

[dkrot]

Usage of 0x10000 in the inverted weight calculation causes overflow of uint16_t if weight=1.
As a result of this bug, frame len used for virtual finish time calculation is always zero, despite of packet size, so packets from input with weight=1 always pass first.

Signed-off-by: Dmitriy Krot globaxbiz@gmail.com

[muvarov]

Hello Dmitriy, to accept your patch to ODP project ODP Contributor license agreement is needed to be signed. You can find a link to sign from main web site http://opendataplane.org/contributor/individual/
Thank you, for contribution! Maxim. ODP maintainer.

[Bill-Fischofer-Linaro]

This looks like a good catch. Since this is a bug fix I've opened Bug https://bugs.linaro.org/show_bug.cgi?id=2990 to track this. The commit log should reference this Bug as part of the merge.

[dkrot]

Hi Maxim. I signed ODP Contributor license agreement. Can you please check is it correctly accepted, because nothing happened after form submission.

[muvarov]

@dkrot yes, I see you signed ODP Agreement. There is no feedback for that yet, hope I will have time to work on it :) One small issue is your patch is that git commit message have to have empty line between short message and long message, like

The reason for that is short message goes to email subject. And long message goes to patch email body. Also it's common style for other commits.

[dkrot]

@muvarov fixed

[muvarov]

Interesting TravisCI did not run with reason "abuse detected: known offender (request looked fishy)" and "abuse detected: user page on GitHub gives 404 (request looked fishy)". All other pull request are validated. Might be some bug during maintenance work on github or travis..

[muvarov]

From: Maxim Uvarov 
 Bill, you clicked to review on github page, can you please
write review by in email if reviewed?

Also one comment bellow.


On 05/04/2017 01:00 PM, Github ODP bot wrote:
> From: Dmitriy Krot 
>
> Usage of 0x10000 in the inverted weight calculation causes overflow of uint16_t if weight=1.
> As a result of this bug, frame len used for virtual finish time calculation is always zero, despite of packet size, so packets from input with weight=1 always pass first.
>
> Signed-off-by: Dmitriy Krot 
> ---
> /** Email created from pull request 21 (dkrot:master)
>   ** https://github.com/Linaro/odp/pull/21
>   ** Patch: https://github.com/Linaro/odp/pull/21.patch
>   ** Base sha: 79ba737a404d2833ad33d8f84ed6ce82c9a8c18e
>   ** Merge commit sha: 52076413a304fa921e43dbbb64e4fa1989f2cabc
>   **/
>   platform/linux-generic/odp_traffic_mngr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/platform/linux-generic/odp_traffic_mngr.c b/platform/linux-generic/odp_traffic_mngr.c
> index 4e9358b..a93b3ba 100644
> --- a/platform/linux-generic/odp_traffic_mngr.c
> +++ b/platform/linux-generic/odp_traffic_mngr.c
> @@ -3238,7 +3238,7 @@ static void tm_sched_params_cvt_to(odp_tm_sched_params_t *odp_sched_params,
>   		if (weight == 0)
>   			inv_weight = 0;
>   		else
> -			inv_weight = 0x10000 / weight;
> +			inv_weight = 0xFFFF / weight;

|Will it be more readable if we write ((uint16_t)-1) instead of 0xFFFF ? 
I.e. maximum value for this type? Maxim. |



>   
>   		tm_sched_params->sched_modes[priority] = sched_mode;
>   		tm_sched_params->inverted_weights[priority] = inv_weight;
> @@ -3254,7 +3254,7 @@ static void tm_sched_params_cvt_from(tm_sched_params_t     *tm_sched_params,
>   	for (priority = 0; priority < ODP_TM_MAX_PRIORITIES; priority++) {
>   		sched_mode = tm_sched_params->sched_modes[priority];
>   		inv_weight = tm_sched_params->inverted_weights[priority];
> -		weight     = 0x10000 / inv_weight;
> +		weight     = 0xFFFF / inv_weight;
>   
>   		odp_sched_params->sched_modes[priority] = sched_mode;
>   		odp_sched_params->sched_weights[priority] = weight;
>

 

[Bill-Fischofer-Linaro]

On Wed, May 3, 2017 at 10:18 AM, Dmitry Krot ***@***.***> wrote: Usage of 0x10000 in the inverted weight calculation causes overflow of uint16_t if weight=1. As a result of this bug, frame len used for virtual finish time calculation is always zero, despite of packet size, so packets from input with weight=1 always pass first. Signed-off-by: Dmitriy Krot ***@***.***

Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>

…

------------------------------ You can view, comment on, or merge this pull request online at: #21 Commit Summary - linux-gen: tm: fix wrr/wfq bug when weight=1 File Changes - *M* platform/linux-generic/odp_traffic_mngr.c <https://github.com/Linaro/odp/pull/21/files#diff-0> (4) Patch Links: - https://github.com/Linaro/odp/pull/21.patch - https://github.com/Linaro/odp/pull/21.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#21>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMzO8o-D7CQC5gojsv_Li4CdBwFGmFKiks5r2JqsgaJpZM4NPjcr> .

[muvarov]

Merged.