Question about code in rc80211_pid_algo.c

Question about code in rc80211_pid_algo.c

[Larry Finger]

Hi,

I'm writing to you as the listed authors of the PID code in the mainline kernel.

I am helping adapt the PID algorithm for an appliance that will use the Realtek 
vendor driver and the ALFA AWUS036H device. The vendor driver's rate-control 
mechanism is not very effective. As the target kernel for the appliance is 
2.6.21, we are not backporting mac80211 and the rtl8187 driver.

The package is working quite well, but Mark, the project manager, brought one 
section to my attention and I am forwarding it.

In routine rate_control_pid_sample(), the percentage failure is calculated, then 
if the rate has been changed, the code goes on to "update the rate behaviour 
info". At that point, the calculation is

          tmp = (pf - spinfo->last_pf);
          tmp = RC_PID_DO_ARITH_RIGHT_SHIFT(tmp, RC_PID_ARITH_SHIFT);

 From our analysis of the code, both pf and last_pf must be in the range 0-100, 
and the first statement results in tmp between -100 and 100. After the second 
line does an arithmetic right shift of 8, the resulting value will always be 
zero. Did we miss something, or is this really a convoluted way to set "tmp" to 
zero?

Thanks,

Larry

Re: Question about code in rc80211_pid_algo.c

[Mattias Nissler]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

Hi Larry,

comments inline.

On Fri, 2011-03-25 at 22:39 -0500, Larry Finger wrote:
> Hi,
> 
> I'm writing to you as the listed authors of the PID code in the mainline kernel.
> 
> I am helping adapt the PID algorithm for an appliance that will use the Realtek 
> vendor driver and the ALFA AWUS036H device. The vendor driver's rate-control 
> mechanism is not very effective. As the target kernel for the appliance is 
> 2.6.21, we are not backporting mac80211 and the rtl8187 driver.
> 
> The package is working quite well, but Mark, the project manager, brought one 
> section to my attention and I am forwarding it.
> 
> In routine rate_control_pid_sample(), the percentage failure is calculated, then 
> if the rate has been changed, the code goes on to "update the rate behaviour 
> info". At that point, the calculation is
> 
>           tmp = (pf - spinfo->last_pf);
>           tmp = RC_PID_DO_ARITH_RIGHT_SHIFT(tmp, RC_PID_ARITH_SHIFT);
> 
>  From our analysis of the code, both pf and last_pf must be in the range 0-100, 
> and the first statement results in tmp between -100 and 100. After the second 
> line does an arithmetic right shift of 8, the resulting value will always be 
> zero. Did we miss something, or is this really a convoluted way to set "tmp" to 
> zero?

I agree with your analysis, the code will effectively set tmp to zero. I
would consider this a bug :) The whole differences computation business
is Stefano's invention though. IIRC, his idea was that we should not
have a static ordering of rates, but rather learn the order of rates in
terms of achievable TX rate. I guess you really want to check with him
in order to find out what to do :)

Mattias


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Question about code in rc80211_pid_algo.c

[Larry Finger]

On 03/26/2011 06:50 AM, Mattias Nissler wrote:
>
> I agree with your analysis, the code will effectively set tmp to zero. I
> would consider this a bug :) The whole differences computation business
> is Stefano's invention though. IIRC, his idea was that we should not
> have a static ordering of rates, but rather learn the order of rates in
> terms of achievable TX rate. I guess you really want to check with him
> in order to find out what to do :)

Hi Mattias,

Thanks for responding. I saw from "git blame" that Stefano had added that part 
of the code, but it has been a long time since he has participated on the lists. 
I'm not sure that he will see this.

I'll try to figure out what might have been meant by that code.

Larry