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
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
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