[PATCH iproute2] tc: fix warning in q_pie.c

[PATCH iproute2] tc: fix warning in q_pie.c

[Brian Vazquez]

Warning was:
q_pie.c:202:22: error: implicit conversion from 'unsigned long' to
'double'

Fixes: 492ec9558b30 ("tc: pie: change maximum integer value of tc_pie_xstats->prob")
Cc: Leslie Monis <lesliemonis@gmail.com>
Signed-off-by: Brian Vazquez <brianvv@google.com>
---
 tc/q_pie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/q_pie.c b/tc/q_pie.c
index 40982f96..52ba7256 100644
--- a/tc/q_pie.c
+++ b/tc/q_pie.c
@@ -199,7 +199,7 @@ static int pie_print_xstats(struct qdisc_util *qu, FILE *f,
 	st = RTA_DATA(xstats);
 	/*prob is returned as a fracion of maximum integer value */
 	fprintf(f, "prob %f delay %uus avg_dq_rate %u\n",
-		(double)st->prob / UINT64_MAX, st->delay,
+		(double)st->prob / (double)UINT64_MAX, st->delay,
 		st->avg_dq_rate);
 	fprintf(f, "pkts_in %u overlimit %u dropped %u maxq %u ecn_mark %u\n",
 		st->packets_in, st->overlimit, st->dropped, st->maxq,
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH iproute2] tc: fix warning in q_pie.c

[Stephen Hemminger]

On Tue, 26 Nov 2019 21:20:59 -0800
Brian Vazquez <brianvv@google.com> wrote:

> Warning was:
> q_pie.c:202:22: error: implicit conversion from 'unsigned long' to
> 'double'
> 
> Fixes: 492ec9558b30 ("tc: pie: change maximum integer value of tc_pie_xstats->prob")
> Cc: Leslie Monis <lesliemonis@gmail.com>
> Signed-off-by: Brian Vazquez <brianvv@google.com>
> ---
>  tc/q_pie.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tc/q_pie.c b/tc/q_pie.c
> index 40982f96..52ba7256 100644
> --- a/tc/q_pie.c
> +++ b/tc/q_pie.c
> @@ -199,7 +199,7 @@ static int pie_print_xstats(struct qdisc_util *qu, FILE *f,
>  	st = RTA_DATA(xstats);
>  	/*prob is returned as a fracion of maximum integer value */
>  	fprintf(f, "prob %f delay %uus avg_dq_rate %u\n",
> -		(double)st->prob / UINT64_MAX, st->delay,
> +		(double)st->prob / (double)UINT64_MAX, st->delay,
>  		st->avg_dq_rate);
>  	fprintf(f, "pkts_in %u overlimit %u dropped %u maxq %u ecn_mark %u\n",
>  		st->packets_in, st->overlimit, st->dropped, st->maxq,

What compiler is this?
The type seems correct already.  The type of double / unsigned long is double.
And the conversion may give different answer.

Re: [PATCH iproute2] tc: fix warning in q_pie.c

[Maciej Żenczykowski]

> What compiler is this?
> The type seems correct already.  The type of double / unsigned long is double.
> And the conversion may give different answer.

Probably some recent version of clang with -Wall.

That said, I think the warning/error is correct.
UINT64 doesn't fit in double (which is also 64 bits, but includes sign
and exponent) - you lose ~13 bits of precision.
I'm not aware of a way to (natively) divide a double by a uint64
without the loss (not that it really matters since the double doesn't
have the requisite precision in the first place).

Why do you think the conversion will give a different answer?
Isn't this exactly what the compiler will do anyway?
It's not like we have long double anymore...

Re: [PATCH iproute2] tc: fix warning in q_pie.c

[Brian Vazquez]

On Wed, Nov 27, 2019 at 8:44 AM Maciej Żenczykowski <maze@google.com> wrote:
>
> > What compiler is this?
> > The type seems correct already.  The type of double / unsigned long is double.
> > And the conversion may give different answer.

I don't think this conversion will give a different answer, the
compiler already change the value from UINT64_MAX to 'UINT64_MAX + 1'
which is pow of 2 and can be represented precisely in a double.  This
change is just making that conversion explicit to avoid the warning.

>
> Probably some recent version of clang with -Wall.

It's clang 10

>
> That said, I think the warning/error is correct.
> UINT64 doesn't fit in double (which is also 64 bits, but includes sign
> and exponent) - you lose ~13 bits of precision.
> I'm not aware of a way to (natively) divide a double by a uint64
> without the loss (not that it really matters since the double doesn't
> have the requisite precision in the first place).
>
> Why do you think the conversion will give a different answer?
> Isn't this exactly what the compiler will do anyway?
> It's not like we have long double anymore...

Re: [PATCH iproute2] tc: fix warning in q_pie.c

[Leslie Monis]

On Wed, Nov 27, 2019 at 10:55 PM Brian Vazquez <brianvv@google.com> wrote:
>
> On Wed, Nov 27, 2019 at 8:44 AM Maciej Żenczykowski <maze@google.com> wrote:
> >
> > > What compiler is this?
> > > The type seems correct already.  The type of double / unsigned long is double.
> > > And the conversion may give different answer.
>
> I don't think this conversion will give a different answer, the
> compiler already change the value from UINT64_MAX to 'UINT64_MAX + 1'
> which is pow of 2 and can be represented precisely in a double.  This
> change is just making that conversion explicit to avoid the warning.

If it helps get rid of the warning on clang, I don't see any issue
with this patch.
The explicit conversion doesn't change the final result at all.
I verified this on GCC 7.4.0 just to be sure.

UINT64_MAX is (2^64 - 1) -- the required value for the calculation.
(double)UINT64_MAX is (2^64) -- the value used by the compiler (in this case)
regardless of whether the conversion is implicit or explicit. This small change
in the required value doesn't affect the precision of the result.


> >
> > Probably some recent version of clang with -Wall.
>
> It's clang 10
>
> >
> > That said, I think the warning/error is correct.
> > UINT64 doesn't fit in double (which is also 64 bits, but includes sign
> > and exponent) - you lose ~13 bits of precision.
> > I'm not aware of a way to (natively) divide a double by a uint64
> > without the loss (not that it really matters since the double doesn't
> > have the requisite precision in the first place).
> >
> > Why do you think the conversion will give a different answer?
> > Isn't this exactly what the compiler will do anyway?
> > It's not like we have long double anymore...

Re: [PATCH iproute2] tc: fix warning in q_pie.c

[Stephen Hemminger]

On Tue, 26 Nov 2019 21:20:59 -0800
Brian Vazquez <brianvv@google.com> wrote:

> Warning was:
> q_pie.c:202:22: error: implicit conversion from 'unsigned long' to
> 'double'
> 
> Fixes: 492ec9558b30 ("tc: pie: change maximum integer value of tc_pie_xstats->prob")
> Cc: Leslie Monis <lesliemonis@gmail.com>
> Signed-off-by: Brian Vazquez <brianvv@google.com>

Does not apply to current version. Please rebase and resubmit