2009-08-24 17:42:41

by Arnd Hannemann

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

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 <[email protected]>
---
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

2009-08-24 18:20:37

by Arnd Hannemann

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

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 <[email protected]>
>> ---
>> 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

2009-08-24 18:39:30

by Arnd Hannemann

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

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 <[email protected]>
>>>> ---
>>>> 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

2009-08-24 18:20:18

by Pavel Roskin

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

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 <[email protected]>
> > ---
> > 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

2009-08-24 18:37:07

by Arnd Hannemann

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

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 <[email protected]>
>>> ---
>>> 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

2009-08-24 17:57:54

by Joe Perches

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

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 <[email protected]>
> ---
> 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?

2009-08-24 18:51:33

by Arnd Hannemann

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

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 <[email protected]>
---
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

2009-09-01 00:54:54

by Julian Calaby

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

On Tue, Aug 25, 2009 at 04:51, Arnd
Hannemann<[email protected]> 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 <[email protected]>
> ---
> ?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: [email protected]
.Plan: http://sites.google.com/site/juliancalaby/

2009-09-01 08:06:04

by Arnd Hannemann

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

Julian Calaby wrote:
> On Tue, Aug 25, 2009 at 04:51, Arnd
> Hannemann<[email protected]> 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 <[email protected]>
>> ---
>> 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