Rate setting problem with pid algorithm for (at least) p54usb and b43

Rate setting problem with pid algorithm for (at least) p54usb and b43

[Larry Finger]

Johannes,

In testing today, I discovered that p54usb and b43 never advance beyond 1 Mb/s
when using the pid algorithm. When I monitored rc_pid_events in
/sys/kernel/debug/.../, I discovered the rate-setting details as follows:

3547 4302748627 pf_sample 12800 -9216 -9216 0

IIRC from debugging b43legacy in the past, this indicates that the pid code is
never seeing any successful transmissions. I debugged the code in
rate_control_pid_tx_status() and found that p54usb is passing 1 in
status.rates[0].count, thus the following fragment is handling each frame as
though it had retries, even though there were none:

        /* We count frames that totally failed to be transmitted as two bad
         * frames, those that made it out but had some retries as one good and
         * one bad frame. */
        if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
                spinfo->tx_num_failed += 2;
                spinfo->tx_num_xmit++;
        } else if (info->status.rates[0].count) {
                spinfo->tx_num_failed++;
                spinfo->tx_num_xmit++;
        }

This is a regression introduced in commit 9ea2c74 "mac80211/drivers: rewrite the
rate control API".

The following patch fixes the problem. Is it right?

Index: wireless-testing/net/mac80211/rc80211_pid_algo.c
===================================================================
--- wireless-testing.orig/net/mac80211/rc80211_pid_algo.c
+++ wireless-testing/net/mac80211/rc80211_pid_algo.c
@@ -256,7 +256,7 @@ static void rate_control_pid_tx_status(v
        if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
                spinfo->tx_num_failed += 2;
                spinfo->tx_num_xmit++;
-       } else if (info->status.rates[0].count) {
+       } else if (info->status.rates[0].count > 1) {
                spinfo->tx_num_failed++;
                spinfo->tx_num_xmit++;
        }

Larry

Re: Rate setting problem with pid algorithm for (at least) p54usb and b43

[Johannes Berg]

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

Larry,

> In testing today, I discovered that p54usb and b43 never advance beyond 1 Mb/s
> when using the pid algorithm. When I monitored rc_pid_events in
> /sys/kernel/debug/.../, I discovered the rate-setting details as follows:
> 
> 3547 4302748627 pf_sample 12800 -9216 -9216 0
> 
> IIRC from debugging b43legacy in the past, this indicates that the pid code is
> never seeing any successful transmissions. I debugged the code in
> rate_control_pid_tx_status() and found that p54usb is passing 1 in
> status.rates[0].count, thus the following fragment is handling each frame as
> though it had retries, even though there were none:
> 
>         /* We count frames that totally failed to be transmitted as two bad
>          * frames, those that made it out but had some retries as one good and
>          * one bad frame. */
>         if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
>                 spinfo->tx_num_failed += 2;
>                 spinfo->tx_num_xmit++;
>         } else if (info->status.rates[0].count) {
>                 spinfo->tx_num_failed++;
>                 spinfo->tx_num_xmit++;
>         }
> 
> This is a regression introduced in commit 9ea2c74 "mac80211/drivers: rewrite the
> rate control API".

Thank you so much for looking into this and finding it. I've stared at
the code a few times for a while, suspecting there was a problem from
limited testing with zd1211rw, and never found it; finally I gave up and
blamed it on zd1211rw. It never occurred to me check whether it was also
happening with b43 :(

> The following patch fixes the problem. Is it right?

Yes. This is a stupid search & replace error, the previous retry_count
was meant to be for "retries" thus 0 if the first transmission went
through, while "count" is now for the number of transmissions at that
rate, hence 1 if the first went through.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> Index: wireless-testing/net/mac80211/rc80211_pid_algo.c
> ===================================================================
> --- wireless-testing.orig/net/mac80211/rc80211_pid_algo.c
> +++ wireless-testing/net/mac80211/rc80211_pid_algo.c
> @@ -256,7 +256,7 @@ static void rate_control_pid_tx_status(v
>         if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
>                 spinfo->tx_num_failed += 2;
>                 spinfo->tx_num_xmit++;
> -       } else if (info->status.rates[0].count) {
> +       } else if (info->status.rates[0].count > 1) {
>                 spinfo->tx_num_failed++;
>                 spinfo->tx_num_xmit++;
>         }
> 
> Larry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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