diff mbox

[05/10] rt2800: make ba_size depend on ampdu_factor

Message ID 20161114124323.GA31857@redhat.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Stanislaw Gruszka Nov. 14, 2016, 12:43 p.m. UTC 
On Mon, Nov 14, 2016 at 09:45:36AM +0100, Stanislaw Gruszka wrote:
> Could you check below patch and see if it helps? If it does not,
> could you printk sta->ht_cap.ampdu_density and ba_size values
> and provide them here.

Actually please print parameters from below patch. I think ba_size
should be based on per TID buf_size instead of ampdu_factor, in case
STA has buf size different for some TIDs.

Also adding Felix to cc since my orginal patch:
http://marc.info/?l=linux-wireless&m=147809595316289&w=2
was shamelessly stolen from mt76 driver. Perhaps Felix could provide
us some expertise.

Thanks
Stanislaw

Comments

Mathias Kresin Nov. 16, 2016, 8:07 a.m. UTC | #1 
14.11.2016 13:43, Stanislaw Gruszka:
> On Mon, Nov 14, 2016 at 09:45:36AM +0100, Stanislaw Gruszka wrote:
>> Could you check below patch and see if it helps? If it does not,
>> could you printk sta->ht_cap.ampdu_density and ba_size values
>> and provide them here.
>
> Actually please print parameters from below patch. I think ba_size
> should be based on per TID buf_size instead of ampdu_factor, in case
> STA has buf size different for some TIDs.
>
> Also adding Felix to cc since my orginal patch:
> http://marc.info/?l=linux-wireless&m=147809595316289&w=2
> was shamelessly stolen from mt76 driver. Perhaps Felix could provide
> us some expertise.
>
> Thanks
> Stanislaw
>
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> index 2515702..35bc38c 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> @@ -7950,6 +7950,8 @@ int rt2800_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>  	struct rt2x00_sta *sta_priv = (struct rt2x00_sta *)sta->drv_priv;
>  	int ret = 0;
>
> +	printk("action %d sta %pM tid %u buf_size %u ampdu_factor %u\n", params->action, sta->addr, params->tid, params->buf_size, sta->ht_cap.ampdu_factor);
> +
>  	/*
>  	 * Don't allow aggregation for stations the hardware isn't aware
>  	 * of because tx status reports for frames to an unknown station
>

Here are the results of the requested tests. Please keep in mind, I'm 
not in a lab environment:

LEDE head
   connect
     action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
     action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
     action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3

   wireless (iperf client) to wired (iperf server)
     Interval           Transfer     Bandwidth
       0.00-60.01  sec   544 MBytes  76.1 Mbits/sec           sender
       0.00-60.01  sec   544 MBytes  76.1 Mbits/sec           receiver

   wired (iperf client) to wireless (iperf server)
     Interval           Transfer     Bandwidth       Retr
       0.00-60.00  sec   609 MBytes  85.1 Mbits/sec   96      sender
       0.00-60.00  sec   606 MBytes  84.8 Mbits/sec           receiver

   on interface down
     action 4 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 1 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 1 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 0 ampdu_factor 3

LEDE + vanilla driver (without LEDE rt2x00 patches)
   connect
     action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
     action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
     action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3

   wireless (iperf client) to wired (iperf server)
     Interval           Transfer     Bandwidth
       0.00-60.02  sec   522 MBytes  73.0 Mbits/sec           sender
       0.00-60.02  sec   522 MBytes  73.0 Mbits/sec           receiver

   wired (iperf client) to wireless (iperf server)
     Interval           Transfer     Bandwidth       Retr
       0.00-60.00  sec   583 MBytes  81.5 Mbits/sec  104      sender
       0.00-60.00  sec   581 MBytes  81.2 Mbits/sec           receiver

   on interface down
     action 4 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 1 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 1 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 0 ampdu_factor 3

LEDE + vanilla driver + patchset
   connect
     action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
     action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
     action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3

   wireless (iperf client) to wired (iperf server)
     Interval           Transfer     Bandwidth
       0.00-60.02  sec   377 MBytes  52.7 Mbits/sec           sender
       0.00-60.02  sec   377 MBytes  52.6 Mbits/sec           receive

     on interface down
       action 4 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
       action 1 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
       action 1 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 0 ampdu_factor 3
       ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
                       to flush
       ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
                       to flush

   wired (iperf client) to wireless (iperf server)
     * not reliable reproducible stalls/reconnects:
         action 1 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 0 ampdu_factor 3
         action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
         action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
         action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
         action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
     * one time I've seen this:
         ieee80211 phy0: rt2x00lib_rxdone_read_signal: Warning - Frame
                         received with unrecognized signal, mode=0x0001,
                         signal=0x010c, type=4
     * sometimes the same rt2x00queue_flush_queue warning on interface
       down

     Interval           Transfer     Bandwidth       Retr
       0.00-60.00  sec   576 MBytes  80.6 Mbits/sec  447      sender
       0.00-60.00  sec   574 MBytes  80.2 Mbits/sec           receiver

     on interface down
       action 4 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
       action 1 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
       action 1 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 0 ampdu_factor 3

LEDE + vanilla driver + patchset + increased max psdu
   connect
     action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
     action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
     action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3

   wireless (iperf client) to wired (iperf server)
     Interval           Transfer     Bandwidth
       0.00-60.02  sec   366 MBytes  51.1 Mbits/sec           sender
       0.00-60.02  sec   365 MBytes  51.1 Mbits/sec           receiver

   wired (iperf client) to wireless (iperf server)
     Interval           Transfer     Bandwidth       Retr
       0.00-60.00  sec   569 MBytes  79.6 Mbits/sec  471      sender
       0.00-60.00  sec   566 MBytes  79.1 Mbits/sec           receiver

   on interface down
     action 4 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 1 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
     action 1 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 0 ampdu_factor 3
     ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
                     to flush
     ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
                     to flush
Stanislaw Gruszka Nov. 16, 2016, 12:02 p.m. UTC | #2 
On Wed, Nov 16, 2016 at 09:07:00AM +0100, Mathias Kresin wrote:
> Here are the results of the requested tests. Please keep in mind, I'm not in
> a lab environment:
> 
> LEDE head
>   connect
>     action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
>     action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
>     action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
>     action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3

No problem here - buf_size corresponds to ampdu_factor.

>     ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
>                     to flush
>     ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
>                     to flush

I think we do not give device enough time to post AMPDU consisted
with bigger amount of frames. If we want to increase ba_size we will
need also some other changes in the driver. Anyway I already request
Kalle to drop this patch. I assume other patches do not cause
regression for you, correct? 

Thanks for testing.
Stanislaw
Mathias Kresin Nov. 16, 2016, 3:54 p.m. UTC | #3 
2016-11-16 13:02 GMT+01:00 Stanislaw Gruszka <sgruszka@redhat.com>:
> On Wed, Nov 16, 2016 at 09:07:00AM +0100, Mathias Kresin wrote:
>> Here are the results of the requested tests. Please keep in mind, I'm not in
>> a lab environment:
>>
>> LEDE head
>>   connect
>>     action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
>>     action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
>>     action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
>>     action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
>
> No problem here - buf_size corresponds to ampdu_factor.
>
>>     ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
>>                     to flush
>>     ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
>>                     to flush
>
> I think we do not give device enough time to post AMPDU consisted
> with bigger amount of frames. If we want to increase ba_size we will
> need also some other changes in the driver. Anyway I already request
> Kalle to drop this patch. I assume other patches do not cause
> regression for you, correct?


Correct. Just 05/10 caused issues.
diff mbox

Patch

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 2515702..35bc38c 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -7950,6 +7950,8 @@  int rt2800_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	struct rt2x00_sta *sta_priv = (struct rt2x00_sta *)sta->drv_priv;
 	int ret = 0;
 
+	printk("action %d sta %pM tid %u buf_size %u ampdu_factor %u\n", params->action, sta->addr, params->tid, params->buf_size, sta->ht_cap.ampdu_factor);
+
 	/*
 	 * Don't allow aggregation for stations the hardware isn't aware
 	 * of because tx status reports for frames to an unknown station