[PATCH] mac80211: Fix output of minstrels rc_stats

[PATCH] mac80211: Fix output of minstrels rc_stats

[Arnd Hannemann]

An integer overflow in the minstrel debug code prevented the
throughput to be displayed correctly. This patch fixes that,
by swaping the division and multiplication.

Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
---
 net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
index 98f4807..caf9453 100644
--- a/net/mac80211/rc80211_minstrel_debugfs.c
+++ b/net/mac80211/rc80211_minstrel_debugfs.c
@@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
 		p += sprintf(p, "%3u%s", mr->bitrate / 2,
 				(mr->bitrate & 1 ? ".5" : "  "));
 
-		tp = ((mr->cur_tp * 96) / 18000) >> 10;
+		tp = ((mr->cur_tp / 18000) * 96) >> 10;
 		prob = mr->cur_prob / 18;
 		eprob = mr->probability / 18;
 
-- 
1.6.4.1

Re: [PATCH] mac80211: Fix output of minstrels rc_stats

[Joe Perches]

On Mon, 2009-08-24 at 19:42 +0200, Arnd Hannemann wrote:
> An integer overflow in the minstrel debug code prevented the
> throughput to be displayed correctly. This patch fixes that,
> by swaping the division and multiplication.
> 
> Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
> ---
>  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
> index 98f4807..caf9453 100644
> --- a/net/mac80211/rc80211_minstrel_debugfs.c
> +++ b/net/mac80211/rc80211_minstrel_debugfs.c
> @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
>  		p += sprintf(p, "%3u%s", mr->bitrate / 2,
>  				(mr->bitrate & 1 ? ".5" : "  "));
>  
> -		tp = ((mr->cur_tp * 96) / 18000) >> 10;
> +		tp = ((mr->cur_tp / 18000) * 96) >> 10;

Maybe do_div instead?

Re: [PATCH] mac80211: Fix output of minstrels rc_stats

[Arnd Hannemann]

Joe Perches schrieb:
> On Mon, 2009-08-24 at 19:42 +0200, Arnd Hannemann wrote:
>> An integer overflow in the minstrel debug code prevented the
>> throughput to be displayed correctly. This patch fixes that,
>> by swaping the division and multiplication.
>>
>> Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
>> ---
>>  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
>> index 98f4807..caf9453 100644
>> --- a/net/mac80211/rc80211_minstrel_debugfs.c
>> +++ b/net/mac80211/rc80211_minstrel_debugfs.c
>> @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
>>  		p += sprintf(p, "%3u%s", mr->bitrate / 2,
>>  				(mr->bitrate & 1 ? ".5" : "  "));
>>  
>> -		tp = ((mr->cur_tp * 96) / 18000) >> 10;
>> +		tp = ((mr->cur_tp / 18000) * 96) >> 10;
> 
> Maybe do_div instead?

Do you mean the do_div from asm/div64h ?
It seems overly complicated to me. It would result in something like:
tp = mr->cur_tp;
do_div(tp, 18000);
tp = (tp * 96) >> 10;

Or am I missing something?
Probably
tp = mr->cur_tp * 96;
would not work..., as it would already overflow. Not sure if
do_div(tp * 96, 18000);
would work?

Best regards,
Arnd

Re: [PATCH] mac80211: Fix output of minstrels rc_stats

[Pavel Roskin]

On Mon, 2009-08-24 at 10:57 -0700, Joe Perches wrote:
> On Mon, 2009-08-24 at 19:42 +0200, Arnd Hannemann wrote:
> > An integer overflow in the minstrel debug code prevented the
> > throughput to be displayed correctly. This patch fixes that,
> > by swaping the division and multiplication.
> > 
> > Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
> > ---
> >  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
> > index 98f4807..caf9453 100644
> > --- a/net/mac80211/rc80211_minstrel_debugfs.c
> > +++ b/net/mac80211/rc80211_minstrel_debugfs.c
> > @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
> >  		p += sprintf(p, "%3u%s", mr->bitrate / 2,
> >  				(mr->bitrate & 1 ? ".5" : "  "));
> >  
> > -		tp = ((mr->cur_tp * 96) / 18000) >> 10;
> > +		tp = ((mr->cur_tp / 18000) * 96) >> 10;
> 
> Maybe do_div instead?

How about this?

tp = mr->cur_tp / ((18000 << 10) / 96);

((18000 << 10) / 96) is exactly 192000.

-- 
Regards,
Pavel Roskin

Re: [PATCH] mac80211: Fix output of minstrels rc_stats

[Arnd Hannemann]

Pavel Roskin schrieb:
> On Mon, 2009-08-24 at 10:57 -0700, Joe Perches wrote:
>> On Mon, 2009-08-24 at 19:42 +0200, Arnd Hannemann wrote:
>>> An integer overflow in the minstrel debug code prevented the
>>> throughput to be displayed correctly. This patch fixes that,
>>> by swaping the division and multiplication.
>>>
>>> Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
>>> ---
>>>  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
>>> index 98f4807..caf9453 100644
>>> --- a/net/mac80211/rc80211_minstrel_debugfs.c
>>> +++ b/net/mac80211/rc80211_minstrel_debugfs.c
>>> @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
>>>  		p += sprintf(p, "%3u%s", mr->bitrate / 2,
>>>  				(mr->bitrate & 1 ? ".5" : "  "));
>>>  
>>> -		tp = ((mr->cur_tp * 96) / 18000) >> 10;
>>> +		tp = ((mr->cur_tp / 18000) * 96) >> 10;
>> Maybe do_div instead?
> 
> How about this?
> 
> tp = mr->cur_tp / ((18000 << 10) / 96);
> 
> ((18000 << 10) / 96) is exactly 192000.
> 

Hmm don't seems to be equivalent with the original statement:

  a = 61027734;
  b = ((a * 96) / 18000) >> 10;
  c = a / ((18000 << 10) / 96);
  printf("%u %u\n", b, c);

Outputs:
 84 317


Best regards,
Arnd

Re: [PATCH] mac80211: Fix output of minstrels rc_stats

[Arnd Hannemann]

Arnd Hannemann schrieb:
> Pavel Roskin schrieb:
>> On Mon, 2009-08-24 at 10:57 -0700, Joe Perches wrote:
>>> On Mon, 2009-08-24 at 19:42 +0200, Arnd Hannemann wrote:
>>>> An integer overflow in the minstrel debug code prevented the
>>>> throughput to be displayed correctly. This patch fixes that,
>>>> by swaping the division and multiplication.
>>>>
>>>> Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
>>>> ---
>>>>  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
>>>> index 98f4807..caf9453 100644
>>>> --- a/net/mac80211/rc80211_minstrel_debugfs.c
>>>> +++ b/net/mac80211/rc80211_minstrel_debugfs.c
>>>> @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
>>>>  		p += sprintf(p, "%3u%s", mr->bitrate / 2,
>>>>  				(mr->bitrate & 1 ? ".5" : "  "));
>>>>  
>>>> -		tp = ((mr->cur_tp * 96) / 18000) >> 10;
>>>> +		tp = ((mr->cur_tp / 18000) * 96) >> 10;
>>> Maybe do_div instead?
>> How about this?
>>
>> tp = mr->cur_tp / ((18000 << 10) / 96);
>>
>> ((18000 << 10) / 96) is exactly 192000.
>>
> 
> Hmm don't seems to be equivalent with the original statement:
> 
>   a = 61027734;
>   b = ((a * 96) / 18000) >> 10;
>   c = a / ((18000 << 10) / 96);
>   printf("%u %u\n", b, c);
> 
> Outputs:
>  84 317

Outch, sorry I took the overflowing formula not the fixed one!

Best regards,
Arnd

[PATCH v2] mac80211: Fix output of minstrels rc_stats

[Arnd Hannemann]

An integer overflow in the minstrel debug code prevented the
throughput to be displayed correctly. This patch fixes that,
by permutating operations like proposed by Pavel Roskin.

Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
---
 net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
index 98f4807..3d72ec5 100644
--- a/net/mac80211/rc80211_minstrel_debugfs.c
+++ b/net/mac80211/rc80211_minstrel_debugfs.c
@@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
 		p += sprintf(p, "%3u%s", mr->bitrate / 2,
 				(mr->bitrate & 1 ? ".5" : "  "));
 
-		tp = ((mr->cur_tp * 96) / 18000) >> 10;
+		tp = mr->cur_tp / ((18000 << 10) / 96);
 		prob = mr->cur_prob / 18;
 		eprob = mr->probability / 18;
 
-- 
1.6.4.1

Re: [PATCH v2] mac80211: Fix output of minstrels rc_stats

[Julian Calaby]

On Tue, Aug 25, 2009 at 04:51, Arnd
Hannemann<hannemann@nets.rwth-aachen.de> wrote:
> An integer overflow in the minstrel debug code prevented the
> throughput to be displayed correctly. This patch fixes that,
> by permutating operations like proposed by Pavel Roskin.
>
> Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
> ---
>  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
> index 98f4807..3d72ec5 100644
> --- a/net/mac80211/rc80211_minstrel_debugfs.c
> +++ b/net/mac80211/rc80211_minstrel_debugfs.c
> @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
>                p += sprintf(p, "%3u%s", mr->bitrate / 2,
>                                (mr->bitrate & 1 ? ".5" : "  "));
>
> -               tp = ((mr->cur_tp * 96) / 18000) >> 10;
> +               tp = mr->cur_tp / ((18000 << 10) / 96);

Sorry about being so late, but wouldn't:

tp = ((mr->cur_tp * 2) / 375) >> 10;

also work? (Assuming that the numbers in the constant aren't important)

or even:

tp = (mr->cur_tp / 375) >> 9;

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH v2] mac80211: Fix output of minstrels rc_stats

[Arnd Hannemann]

Julian Calaby wrote:
> On Tue, Aug 25, 2009 at 04:51, Arnd
> Hannemann<hannemann@nets.rwth-aachen.de> wrote:
>> An integer overflow in the minstrel debug code prevented the
>> throughput to be displayed correctly. This patch fixes that,
>> by permutating operations like proposed by Pavel Roskin.
>>
>> Signed-off-by: Arnd Hannemann <hannemann@nets.rwth-aachen.de>
>> ---
>>  net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
>> index 98f4807..3d72ec5 100644
>> --- a/net/mac80211/rc80211_minstrel_debugfs.c
>> +++ b/net/mac80211/rc80211_minstrel_debugfs.c
>> @@ -83,7 +83,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
>>                p += sprintf(p, "%3u%s", mr->bitrate / 2,
>>                                (mr->bitrate & 1 ? ".5" : "  "));
>>
>> -               tp = ((mr->cur_tp * 96) / 18000) >> 10;
>> +               tp = mr->cur_tp / ((18000 << 10) / 96);
> 
> Sorry about being so late, but wouldn't:
> 
> tp = ((mr->cur_tp * 2) / 375) >> 10;
> 
> also work? (Assuming that the numbers in the constant aren't important)

I needed a while to figure out why those constants are used, but finally
they made some sense, so I think its best to preserve them.

mr->cur_tp is the time to send one 1200 byte packet (9600 bits),
the loss probelity is scaled between 0-18000 to reduce rounding error.

> 
> or even:
> 
> tp = (mr->cur_tp / 375) >> 9;

See above.

Best regards,
Arnd