Re: [PATCH] dcb: fix tc-maxrate unit conversions

[Petr Machata <me@pmachata.org>]

Yijing Zeng <zengyijing19900106@gmail.com> writes:

> From: Yijing Zeng <yijingzeng@meta.com>
>
> The ieee_maxrate UAPI is defined as kbps, but dcb_maxrate uses Bps.

Hmm, indeed, "values are 64 bits long and specified in Kbps to enable
usage over both slow and very fast networks". Good catch.

I think that if anyone actually tried to use this in the wild, the issue
would have been discovered and reported already, so this might be
actually safe to fix without breaking the world.

Thanks for the patch. I have a number of relatively minor coding style
points. Please correct them and send v2. You may want to pass the
candidate patch through checkpatch:

 # ../linux-source/scripts/checkpatch.pl --max-line-length=80 the.patch

> This fix patch converts Bps to kbps for parse, and convert kbps to Bps for print_rate().

Please wrap this line to 75 characters.

> Fixes: 117939d9 ("dcb: Add a subtool for the DCB maxrate object")

This should show 12 characters of the hash.

> Signed-off-by: Yijing Zeng <yijingzeng@meta.com>
> ---
>  dcb/dcb_maxrate.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/dcb/dcb_maxrate.c b/dcb/dcb_maxrate.c
> index 1538c6d7..af012dba 100644
> --- a/dcb/dcb_maxrate.c
> +++ b/dcb/dcb_maxrate.c
> @@ -42,13 +42,16 @@ static void dcb_maxrate_help(void)
>  
>  static int dcb_maxrate_parse_mapping_tc_maxrate(__u32 key, char *value, void *data)
>  {
> -	__u64 rate;
> +	__u64 rate_Bps;

Lowercase this, please.

>  
> -	if (get_rate64(&rate, value))
> +	if (get_rate64(&rate_Bps, value))
>  		return -EINVAL;
>  
> +	/* get_rate64() returns Bps. ieee_maxrate UAPI expects kbps. */
> +	__u64 rate_kbps = (rate_Bps * 8) / 1000;

Keep all the declarations at the top of the function, please.
Make sure they are ordered longest line to shortest (RXT, reverse xmass tree).

There's a small concern regarding the * 8 overflowing (likewise * 1000
below). But it's u64, I don't feel like we need to really worry about it.

> +
>  	return dcb_parse_mapping("TC", key, IEEE_8021QAZ_MAX_TCS - 1,
> -				 "RATE", rate, -1,
> +				 "RATE", rate_kbps, -1,
>  				 dcb_set_u64, data);
>  }
>  
> @@ -62,8 +65,11 @@ static void dcb_maxrate_print_tc_maxrate(struct dcb *dcb, const struct ieee_maxr
>  	print_string(PRINT_FP, NULL, "tc-maxrate ", NULL);
>  
>  	for (i = 0; i < size; i++) {
> +		/* ieee_maxrate UAPI returns kbps. print_rate() expects Bps for display */

This line is too long.

> +		__u64 rate_Bps  = maxrate->tc_maxrate[i] * 1000 / 8;

This variable can stay here, it's start of the block, but it should be
lowercased.

>  		snprintf(b, sizeof(b), "%zd:%%s ", i);
> -		print_rate(dcb->use_iec, PRINT_ANY, NULL, b, maxrate->tc_maxrate[i]);
> +		print_rate(dcb->use_iec, PRINT_ANY, NULL, b, rate_Bps);
>  	}
>  
>  	close_json_array(PRINT_JSON, "tc_maxrate");