Setting tx retry count in ath10k

Setting tx retry count in ath10k

[Ben Greear]

I spent a bit of time looking into setting the tx retry count in
ath10k (wave-1 firmware).  The firmware has support for setting this as
a vdev parameter, and it defaults to '2', at least in my wave-1 firmware.

I enabled propagating the setting from mac80211, ie:
iw phy wiphy0 set retry short 2 long 2

And while debugging this, I noticed that mac80211 has a default of
4, but the ath10k firmware has a default of 2.  Now, I am not sure
if I should enable setting the retry count since it will change
the behaviour even if users don't set anything.

Any opinions on this?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Setting tx retry count in ath10k

[Benjamin Beichler]

Hi,

Am 17.07.2018 um 02:37 schrieb Ben Greear:
> I spent a bit of time looking into setting the tx retry count in
> ath10k (wave-1 firmware).=C2=A0 The firmware has support for setting th=
is as
> a vdev parameter, and it defaults to '2', at least in my wave-1 firmwar=
e.
>
> I enabled propagating the setting from mac80211, ie:
> iw phy wiphy0 set retry short 2 long 2
>
> And while debugging this, I noticed that mac80211 has a default of
> 4, but the ath10k firmware has a default of 2.=C2=A0 Now, I am not sure=

> if I should enable setting the retry count since it will change
> the behaviour even if users don't set anything.
>
Maybe I'm wrong, but I have in mind, that 7 retries is the default
setting of mac80211. Although 2 or even 4 seems to be pretty low for the
overall retry count, so maybe the values are somehow changed in the
firmware? From our experiments we know (at least for 802.11n) you need
for normal operation a retry count of something between 5 - 9, but
sometimes also 12 or 15 is beneficial.

We use for our experimental setup mainly ath9k cards and rt28xx nics,
and with them you need definitely more retries.

Nonetheless, I don't think the change from 2 to 4 does really affect the
behavior in a negative way (if it works as expected).

> Any opinions on this?
>
> Thanks,
> Ben
>

--=20
M.Sc. Benjamin Beichler

Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrote=
chnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik

University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE

Richard-Wagner-Stra=C3=9Fe 31
18119 Rostock
Deutschland/Germany

phone: +49 (0) 381 498 - 7278
email: Benjamin.Beichler@uni-rostock.de
www: http://www.imd.uni-rostock.de/

Re: Setting tx retry count in ath10k

[SEDE]

Hi,

In the standard, 7 is the default for the short retry count, 4 is well 
the default for long retry count.

In ath10k, there is also non_agg_sw_retry_th to control this, will this 
still be used?
Or what is the difference with rc_num_retries?

Kind regards,
Sébastien.


On 2018-07-17 09:39, Benjamin Beichler wrote:
> Hi,
>
> Am 17.07.2018 um 02:37 schrieb Ben Greear:
>> I spent a bit of time looking into setting the tx retry count in
>> ath10k (wave-1 firmware).  The firmware has support for setting this as
>> a vdev parameter, and it defaults to '2', at least in my wave-1 firmware.
>>
>> I enabled propagating the setting from mac80211, ie:
>> iw phy wiphy0 set retry short 2 long 2
>>
>> And while debugging this, I noticed that mac80211 has a default of
>> 4, but the ath10k firmware has a default of 2.  Now, I am not sure
>> if I should enable setting the retry count since it will change
>> the behaviour even if users don't set anything.
>>
> Maybe I'm wrong, but I have in mind, that 7 retries is the default
> setting of mac80211. Although 2 or even 4 seems to be pretty low for the
> overall retry count, so maybe the values are somehow changed in the
> firmware? From our experiments we know (at least for 802.11n) you need
> for normal operation a retry count of something between 5 - 9, but
> sometimes also 12 or 15 is beneficial.
>
> We use for our experimental setup mainly ath9k cards and rt28xx nics,
> and with them you need definitely more retries.
>
> Nonetheless, I don't think the change from 2 to 4 does really affect the
> behavior in a negative way (if it works as expected).
>
>> Any opinions on this?
>>
>> Thanks,
>> Ben
>>

Re: Setting tx retry count in ath10k

[Ben Greear]

On 07/17/2018 12:56 AM, SEDE wrote:
> Hi,
>
> In the standard, 7 is the default for the short retry count, 4 is well the default for long retry count.
>
> In ath10k, there is also non_agg_sw_retry_th to control this, will this still be used?
> Or what is the difference with rc_num_retries?
>
> Kind regards,
> Sébastien.

The ath10k firmware has no idea of long vs short retries, so I just used the
long setting.

I will investigate that non_agg_sw_retry_th as well, and I did notice my wave-1
firmware (at least) uses 15 retries for self-generated frames.  But, in my case,
those self-gen frames are not much used anyway since I disable firmware keep-alive.

And, I need to see how the mac80211 stack handles its own retries when working
with ath10k.

Thanks,
Ben

>
>
> On 2018-07-17 09:39, Benjamin Beichler wrote:
>> Hi,
>>
>> Am 17.07.2018 um 02:37 schrieb Ben Greear:
>>> I spent a bit of time looking into setting the tx retry count in
>>> ath10k (wave-1 firmware).  The firmware has support for setting this as
>>> a vdev parameter, and it defaults to '2', at least in my wave-1 firmware.
>>>
>>> I enabled propagating the setting from mac80211, ie:
>>> iw phy wiphy0 set retry short 2 long 2
>>>
>>> And while debugging this, I noticed that mac80211 has a default of
>>> 4, but the ath10k firmware has a default of 2.  Now, I am not sure
>>> if I should enable setting the retry count since it will change
>>> the behaviour even if users don't set anything.
>>>
>> Maybe I'm wrong, but I have in mind, that 7 retries is the default
>> setting of mac80211. Although 2 or even 4 seems to be pretty low for the
>> overall retry count, so maybe the values are somehow changed in the
>> firmware? From our experiments we know (at least for 802.11n) you need
>> for normal operation a retry count of something between 5 - 9, but
>> sometimes also 12 or 15 is beneficial.
>>
>> We use for our experimental setup mainly ath9k cards and rt28xx nics,
>> and with them you need definitely more retries.
>>
>> Nonetheless, I don't think the change from 2 to 4 does really affect the
>> behavior in a negative way (if it works as expected).
>>
>>> Any opinions on this?
>>>
>>> Thanks,
>>> Ben
>>>
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

RE: Setting tx retry count in ath10k

[Jean-Pierre TOSONI]

SGksDQoNCldlIG1hZGUgcmV0cmllcyBjb25maWd1cmFibGUgaW4gb3VyIG1hYzgwMjExK2F0aDlr
IHN5c3RlbSwgYW5kIHdlIGVuZGVkIHdpdGggMyBjb3VudHM6DQoxKSBzaG9ydCByZXRyeSBjb3Vu
dCwgZGVmYXVsdHMgdG8gNA0KMikgbG9uZyByZXRyeXMgY291bnQsIGRlZmF1dHMgdG8gNw0KMykg
c29mdHdhcmUgcmV0cnkgY291bnQsIGRlZmF1bHRzIHRvIDMwDQpUaGlzIGxhc3Qgb25lIGlzIHVz
ZWQgc2VwYXJhdGVseSBmb3IgZWFjaCBmcmFtZSBpbiBhbiBhZ2dyZWdhdGVkIGZyYW1lLCBzaW5j
ZSB0aGV5IGNhbiBiZSBzZXBhcmF0ZWx5IGFja25vd2xlZGdlZC4NCg0KUmVnYXJkcywNCkplYW4t
UGllcnJlDQoNCj4gLS0tLS1NZXNzYWdlIGQnb3JpZ2luZS0tLS0tDQo+IERlwqA6IGxpbnV4LXdp
cmVsZXNzLW93bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXdpcmVsZXNzLW93bmVy
QHZnZXIua2VybmVsLm9yZ10gRGUgbGEgcGFydCBkZQ0KPiBCZW4gR3JlZWFyDQo+IEVudm95w6nC
oDogbWFyZGkgMTcganVpbGxldCAyMDE4IDE3OjA4DQo+IMOAwqA6IFNFREU7IEJlbmphbWluIEJl
aWNobGVyOyBhdGgxMGs7IGxpbnV4LXdpcmVsZXNzQHZnZXIua2VybmVsLm9yZw0KPiBPYmpldMKg
OiBSZTogU2V0dGluZyB0eCByZXRyeSBjb3VudCBpbiBhdGgxMGsNCj4gDQo+IA0KPiANCj4gT24g
MDcvMTcvMjAxOCAxMjo1NiBBTSwgU0VERSB3cm90ZToNCj4gPiBIaSwNCj4gPg0KPiA+IEluIHRo
ZSBzdGFuZGFyZCwgNyBpcyB0aGUgZGVmYXVsdCBmb3IgdGhlIHNob3J0IHJldHJ5IGNvdW50LCA0
IGlzIHdlbGwgdGhlIGRlZmF1bHQgZm9yIGxvbmcgcmV0cnkNCj4gY291bnQuDQo+ID4NCj4gPiBJ
biBhdGgxMGssIHRoZXJlIGlzIGFsc28gbm9uX2FnZ19zd19yZXRyeV90aCB0byBjb250cm9sIHRo
aXMsIHdpbGwgdGhpcyBzdGlsbCBiZSB1c2VkPw0KPiA+IE9yIHdoYXQgaXMgdGhlIGRpZmZlcmVu
Y2Ugd2l0aCByY19udW1fcmV0cmllcz8NCj4gPg0KPiA+IEtpbmQgcmVnYXJkcywNCj4gPiBTw6li
YXN0aWVuLg0KPiANCj4gVGhlIGF0aDEwayBmaXJtd2FyZSBoYXMgbm8gaWRlYSBvZiBsb25nIHZz
IHNob3J0IHJldHJpZXMsIHNvIEkganVzdCB1c2VkIHRoZQ0KPiBsb25nIHNldHRpbmcuDQo+IA0K
PiBJIHdpbGwgaW52ZXN0aWdhdGUgdGhhdCBub25fYWdnX3N3X3JldHJ5X3RoIGFzIHdlbGwsIGFu
ZCBJIGRpZCBub3RpY2UgbXkgd2F2ZS0xDQo+IGZpcm13YXJlIChhdCBsZWFzdCkgdXNlcyAxNSBy
ZXRyaWVzIGZvciBzZWxmLWdlbmVyYXRlZCBmcmFtZXMuICBCdXQsIGluIG15IGNhc2UsDQo+IHRo
b3NlIHNlbGYtZ2VuIGZyYW1lcyBhcmUgbm90IG11Y2ggdXNlZCBhbnl3YXkgc2luY2UgSSBkaXNh
YmxlIGZpcm13YXJlIGtlZXAtYWxpdmUuDQo+IA0KPiBBbmQsIEkgbmVlZCB0byBzZWUgaG93IHRo
ZSBtYWM4MDIxMSBzdGFjayBoYW5kbGVzIGl0cyBvd24gcmV0cmllcyB3aGVuIHdvcmtpbmcNCj4g
d2l0aCBhdGgxMGsuDQo+IA0KPiBUaGFua3MsDQo+IEJlbg0KPiANCj4gPg0KPiA+DQo+ID4gT24g
MjAxOC0wNy0xNyAwOTozOSwgQmVuamFtaW4gQmVpY2hsZXIgd3JvdGU6DQo+ID4+IEhpLA0KPiA+
Pg0KPiA+PiBBbSAxNy4wNy4yMDE4IHVtIDAyOjM3IHNjaHJpZWIgQmVuIEdyZWVhcjoNCj4gPj4+
IEkgc3BlbnQgYSBiaXQgb2YgdGltZSBsb29raW5nIGludG8gc2V0dGluZyB0aGUgdHggcmV0cnkg
Y291bnQgaW4NCj4gPj4+IGF0aDEwayAod2F2ZS0xIGZpcm13YXJlKS4gIFRoZSBmaXJtd2FyZSBo
YXMgc3VwcG9ydCBmb3Igc2V0dGluZyB0aGlzIGFzDQo+ID4+PiBhIHZkZXYgcGFyYW1ldGVyLCBh
bmQgaXQgZGVmYXVsdHMgdG8gJzInLCBhdCBsZWFzdCBpbiBteSB3YXZlLTEgZmlybXdhcmUuDQo+
ID4+Pg0KPiA+Pj4gSSBlbmFibGVkIHByb3BhZ2F0aW5nIHRoZSBzZXR0aW5nIGZyb20gbWFjODAy
MTEsIGllOg0KPiA+Pj4gaXcgcGh5IHdpcGh5MCBzZXQgcmV0cnkgc2hvcnQgMiBsb25nIDINCj4g
Pj4+DQo+ID4+PiBBbmQgd2hpbGUgZGVidWdnaW5nIHRoaXMsIEkgbm90aWNlZCB0aGF0IG1hYzgw
MjExIGhhcyBhIGRlZmF1bHQgb2YNCj4gPj4+IDQsIGJ1dCB0aGUgYXRoMTBrIGZpcm13YXJlIGhh
cyBhIGRlZmF1bHQgb2YgMi4gIE5vdywgSSBhbSBub3Qgc3VyZQ0KPiA+Pj4gaWYgSSBzaG91bGQg
ZW5hYmxlIHNldHRpbmcgdGhlIHJldHJ5IGNvdW50IHNpbmNlIGl0IHdpbGwgY2hhbmdlDQo+ID4+
PiB0aGUgYmVoYXZpb3VyIGV2ZW4gaWYgdXNlcnMgZG9uJ3Qgc2V0IGFueXRoaW5nLg0KPiA+Pj4N
Cj4gPj4gTWF5YmUgSSdtIHdyb25nLCBidXQgSSBoYXZlIGluIG1pbmQsIHRoYXQgNyByZXRyaWVz
IGlzIHRoZSBkZWZhdWx0DQo+ID4+IHNldHRpbmcgb2YgbWFjODAyMTEuIEFsdGhvdWdoIDIgb3Ig
ZXZlbiA0IHNlZW1zIHRvIGJlIHByZXR0eSBsb3cgZm9yIHRoZQ0KPiA+PiBvdmVyYWxsIHJldHJ5
IGNvdW50LCBzbyBtYXliZSB0aGUgdmFsdWVzIGFyZSBzb21laG93IGNoYW5nZWQgaW4gdGhlDQo+
ID4+IGZpcm13YXJlPyBGcm9tIG91ciBleHBlcmltZW50cyB3ZSBrbm93IChhdCBsZWFzdCBmb3Ig
ODAyLjExbikgeW91IG5lZWQNCj4gPj4gZm9yIG5vcm1hbCBvcGVyYXRpb24gYSByZXRyeSBjb3Vu
dCBvZiBzb21ldGhpbmcgYmV0d2VlbiA1IC0gOSwgYnV0DQo+ID4+IHNvbWV0aW1lcyBhbHNvIDEy
IG9yIDE1IGlzIGJlbmVmaWNpYWwuDQo+ID4+DQo+ID4+IFdlIHVzZSBmb3Igb3VyIGV4cGVyaW1l
bnRhbCBzZXR1cCBtYWlubHkgYXRoOWsgY2FyZHMgYW5kIHJ0Mjh4eCBuaWNzLA0KPiA+PiBhbmQg
d2l0aCB0aGVtIHlvdSBuZWVkIGRlZmluaXRlbHkgbW9yZSByZXRyaWVzLg0KPiA+Pg0KPiA+PiBO
b25ldGhlbGVzcywgSSBkb24ndCB0aGluayB0aGUgY2hhbmdlIGZyb20gMiB0byA0IGRvZXMgcmVh
bGx5IGFmZmVjdCB0aGUNCj4gPj4gYmVoYXZpb3IgaW4gYSBuZWdhdGl2ZSB3YXkgKGlmIGl0IHdv
cmtzIGFzIGV4cGVjdGVkKS4NCj4gPj4NCj4gPj4+IEFueSBvcGluaW9ucyBvbiB0aGlzPw0KPiA+
Pj4NCj4gPj4+IFRoYW5rcywNCj4gPj4+IEJlbg0KPiA+Pj4NCj4gPg0KPiANCj4gLS0NCj4gQmVu
IEdyZWVhciA8Z3JlZWFyYkBjYW5kZWxhdGVjaC5jb20+DQo+IENhbmRlbGEgVGVjaG5vbG9naWVz
IEluYyAgaHR0cDovL3d3dy5jYW5kZWxhdGVjaC5jb20NCg==

Re: Setting tx retry count in ath10k

[Ben Greear]

On 07/18/2018 08:50 AM, Jean-Pierre TOSONI wrote:
> Hi,
>
> We made retries configurable in our mac80211+ath9k system, and we ended with 3 counts:
> 1) short retry count, defaults to 4
> 2) long retrys count, defauts to 7
> 3) software retry count, defaults to 30
> This last one is used separately for each frame in an aggregated frame, since they can be separately acknowledged.

Did you have to change code for #3, and if so, can you share the patch?

I wonder also if retries should be different for different types of data.  For instance,
if someone is using UDP, maybe they don't care so much about lost packets and would
prefer a lower retry count.  Or, maybe IP type-of-service could be taken into account
and retry frames different amounts based on ToS?

Thanks,
Ben

>
> Regards,
> Jean-Pierre
>
>> -----Message d'origine-----
>> De : linux-wireless-owner@vger.kernel.org [mailto:linux-wireless-owner@vger.kernel.org] De la part de
>> Ben Greear
>> Envoyé : mardi 17 juillet 2018 17:08
>> À : SEDE; Benjamin Beichler; ath10k; linux-wireless@vger.kernel.org
>> Objet : Re: Setting tx retry count in ath10k
>>
>>
>>
>> On 07/17/2018 12:56 AM, SEDE wrote:
>>> Hi,
>>>
>>> In the standard, 7 is the default for the short retry count, 4 is well the default for long retry
>> count.
>>>
>>> In ath10k, there is also non_agg_sw_retry_th to control this, will this still be used?
>>> Or what is the difference with rc_num_retries?
>>>
>>> Kind regards,
>>> Sébastien.
>>
>> The ath10k firmware has no idea of long vs short retries, so I just used the
>> long setting.
>>
>> I will investigate that non_agg_sw_retry_th as well, and I did notice my wave-1
>> firmware (at least) uses 15 retries for self-generated frames.  But, in my case,
>> those self-gen frames are not much used anyway since I disable firmware keep-alive.
>>
>> And, I need to see how the mac80211 stack handles its own retries when working
>> with ath10k.
>>
>> Thanks,
>> Ben
>>
>>>
>>>
>>> On 2018-07-17 09:39, Benjamin Beichler wrote:
>>>> Hi,
>>>>
>>>> Am 17.07.2018 um 02:37 schrieb Ben Greear:
>>>>> I spent a bit of time looking into setting the tx retry count in
>>>>> ath10k (wave-1 firmware).  The firmware has support for setting this as
>>>>> a vdev parameter, and it defaults to '2', at least in my wave-1 firmware.
>>>>>
>>>>> I enabled propagating the setting from mac80211, ie:
>>>>> iw phy wiphy0 set retry short 2 long 2
>>>>>
>>>>> And while debugging this, I noticed that mac80211 has a default of
>>>>> 4, but the ath10k firmware has a default of 2.  Now, I am not sure
>>>>> if I should enable setting the retry count since it will change
>>>>> the behaviour even if users don't set anything.
>>>>>
>>>> Maybe I'm wrong, but I have in mind, that 7 retries is the default
>>>> setting of mac80211. Although 2 or even 4 seems to be pretty low for the
>>>> overall retry count, so maybe the values are somehow changed in the
>>>> firmware? From our experiments we know (at least for 802.11n) you need
>>>> for normal operation a retry count of something between 5 - 9, but
>>>> sometimes also 12 or 15 is beneficial.
>>>>
>>>> We use for our experimental setup mainly ath9k cards and rt28xx nics,
>>>> and with them you need definitely more retries.
>>>>
>>>> Nonetheless, I don't think the change from 2 to 4 does really affect the
>>>> behavior in a negative way (if it works as expected).
>>>>
>>>>> Any opinions on this?
>>>>>
>>>>> Thanks,
>>>>> Ben
>>>>>
>>>
>>
>> --
>> Ben Greear <greearb@candelatech.com>
>> Candela Technologies Inc  http://www.candelatech.com


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Setting tx retry count in ath10k

[Toke Høiland-Jørgensen]

Ben Greear <greearb@candelatech.com> writes:

> On 07/18/2018 08:50 AM, Jean-Pierre TOSONI wrote:
>> Hi,
>>
>> We made retries configurable in our mac80211+ath9k system, and we ended with 3 counts:
>> 1) short retry count, defaults to 4
>> 2) long retrys count, defauts to 7
>> 3) software retry count, defaults to 30
>> This last one is used separately for each frame in an aggregated frame, since they can be separately acknowledged.
>
> Did you have to change code for #3, and if so, can you share the patch?
>
> I wonder also if retries should be different for different types of
> data. For instance, if someone is using UDP, maybe they don't care so
> much about lost packets and would prefer a lower retry count. Or,
> maybe IP type-of-service could be taken into account and retry frames
> different amounts based on ToS?

For general internet traffic, a retry count of 30 is way too high; that
is up to 120 ms of HOL blocking latency. Better to just drop the packet
at that point.

Ideally, the retry count should be dynamically calculated in units of
time (which would depend on the rate and aggregate size), and also take
queueing time into account. I've been meaning to experiment with this
for minstrel and ath9k, but haven't gotten around to it...

-Toke

RE: Setting tx retry count in ath10k

[Jean-Pierre TOSONI]

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

Hi Ben,

I attached the patch. Please remind that it applies to ath9k.

At the end there are 3 comments in French, translation follows:
1) " longretry gives directly the transmit count, the +1 is useless.
     Should rather have -1 to account for the first try"
2) " The retries just made for this frame must be added in to know
     If the max was overstepped"
3) " Add the 1st try (probably useless).
     Upstream version adds 1 to A-MPDU retries but I don't understand why.
     Since above I removed the +1 to fix the count, I add +1 here once per
     frame in case the "+1" is actually hiding a bug in the upstream version"

@Toke: As you can see in the patch, the value 30 was the fixed value defined
 in ath9k, I kept it for compatibility only (and that's why I wanted to make
 it configurable :-)
On another hand, Minstrel in mac80211 seems to vary retries according to what
you say, i.e. Minstrel tries to stay below a certain amount of time, but this
only applies to short/long retries.

Jean-Pierre

> -----Message d'origine-----
> De : Toke Høiland-Jørgensen [mailto:toke@toke.dk]
> Envoyé : mercredi 18 juillet 2018 18:22
> À : Ben Greear; Jean-Pierre TOSONI; SEDE; Benjamin Beichler; ath10k; linux-wireless@vger.kernel.org
> Objet : Re: Setting tx retry count in ath10k
> 
> Ben Greear <greearb@candelatech.com> writes:
> 
> > On 07/18/2018 08:50 AM, Jean-Pierre TOSONI wrote:
> >> Hi,
> >>
> >> We made retries configurable in our mac80211+ath9k system, and we ended with 3 counts:
> >> 1) short retry count, defaults to 4
> >> 2) long retrys count, defauts to 7
> >> 3) software retry count, defaults to 30
> >> This last one is used separately for each frame in an aggregated frame, since they can be
> separately acknowledged.
> >
> > Did you have to change code for #3, and if so, can you share the patch?
> >
> > I wonder also if retries should be different for different types of
> > data. For instance, if someone is using UDP, maybe they don't care so
> > much about lost packets and would prefer a lower retry count. Or,
> > maybe IP type-of-service could be taken into account and retry frames
> > different amounts based on ToS?
> 
> For general internet traffic, a retry count of 30 is way too high; that
> is up to 120 ms of HOL blocking latency. Better to just drop the packet
> at that point.
> 
> Ideally, the retry count should be dynamically calculated in units of
> time (which would depend on the rate and aggregate size), and also take
> queueing time into account. I've been meaning to experiment with this
> for minstrel and ath9k, but haven't gotten around to it...
> 
> -Toke

[-- Attachment #2: a919-acksys-Add-parameter-for-software-retry.patch --]
[-- Type: application/octet-stream, Size: 4644 bytes --]

From 3b614d719a6fb806fef7fc560841e6c93e812753 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20DEBARGE?= <cedric.debarge@acksys.fr>
Date: Thu, 5 Jun 2014 12:13:05 +0200
Subject: [PATCH 2/2] Add parameter for software retry

When the aggregate frame are sent, the ATH9K manage a sofware retries.
The maximum sofware retries is set to ATH9K_SW_MAX_RETRIES (30)
constant.

Add max_sw_retries field in ath_softc structure.
Set the default value to ATH9K_SW_MAX_RETRIES (to keep backward
compatibility).
Add entry in debugfs to configure this new value.
---
 drivers/net/wireless/ath/ath9k/ath9k.h |    1 +
 drivers/net/wireless/ath/ath9k/debug.c |   41 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/init.c  |    1 +
 drivers/net/wireless/ath/ath9k/xmit.c  |    2 +-
 4 files changed, 44 insertions(+), 1 deletion(-)

--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -990,6 +990,7 @@ struct ath_softc {
 
 	u8 gtt_cnt;
 	u32 intrstatus;
+	u8 max_sw_retries;
 	u16 ps_flags; /* PS_* */
 	bool ps_enabled;
 	bool ps_idle;
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -1296,6 +1296,45 @@ void ath9k_get_et_stats(struct ieee80211
 	WARN_ON(i != ATH9K_SSTATS_LEN);
 }
 
+static ssize_t read_file_max_sw_retries(struct file *file, char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	struct ath_softc *sc = file->private_data;
+	char buf[32];
+	unsigned int len;
+
+	len = sprintf(buf, "%d\n", sc->max_sw_retries);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t write_max_sw_retries(struct file *file, const char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	struct ath_softc *sc = file->private_data;
+	unsigned long max_sw_retries;
+	char buf[32];
+	ssize_t len;
+
+	len = min(count, sizeof(buf) - 1);
+	if (copy_from_user(buf, user_buf, len))
+		return -EFAULT;
+
+	buf[len] = '\0';
+	if (kstrtoul(buf, 0, &max_sw_retries))
+		return -EINVAL;
+
+	sc->max_sw_retries = max_sw_retries;
+	return count;
+}
+
+static const struct file_operations fops_max_sw_retries = {
+	.read = read_file_max_sw_retries,
+	.write = write_max_sw_retries,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 void ath9k_deinit_debug(struct ath_softc *sc)
 {
 	ath9k_cmn_spectral_deinit_debug(&sc->spec_priv);
@@ -1550,6 +1589,8 @@ int ath9k_init_debug(struct ath_hw *ah)
 			  &ah->rxchainmask);
 	debugfs_create_u8("tx_chainmask", S_IRUSR, sc->debug.debugfs_phy,
 			  &ah->txchainmask);
+	debugfs_create_file("max_sw_retries", S_IRUSR | S_IWUSR,
+			    sc->debug.debugfs_phy, sc, &fops_max_sw_retries);
 	debugfs_create_file("ani", S_IRUSR | S_IWUSR,
 			    sc->debug.debugfs_phy, sc, &fops_ani);
 	debugfs_create_bool("paprd", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -383,6 +383,7 @@ static void ath9k_init_misc(struct ath_s
 	sc->spec_priv.spec_config.endless = false;
 	sc->spec_priv.spec_config.period = 0xFF;
 	sc->spec_priv.spec_config.fft_period = 0xF;
+	sc->max_sw_retries = ATH_MAX_SW_RETRIES;
 }
 
 static void ath9k_init_pcoem_platform(struct ath_softc *sc)
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -471,7 +471,9 @@ static void ath_tx_complete_aggr(struct
 
 	memcpy(rates, bf->rates, sizeof(rates));
 
-	retries = ts->ts_longretry + 1;
+	/* longretry donne directement le nombre de transmissions, le +1 est
+	 * inutile. On devrait plutot avoir -1 pour enlever le 1er try. */
+	retries = ts->ts_longretry;
 	for (i = 0; i < ts->ts_rateindex; i++)
 		retries += rates[i].count;
 
@@ -560,10 +562,22 @@ static void ath_tx_complete_aggr(struct
 			acked_cnt++;
 		} else if (flush) {
 			txpending = 1;
-		} else if (fi->retries < ATH_MAX_SW_RETRIES) {
+		} else if (fi->retries+retries < sc->max_sw_retries) {
+			/* Les retries qui viennent d'être faits sur cette
+			 * trame doivent être ajoutés pour savoir si on a
+			 * dépassé le max */
+			/*
+			 * Ajouter le 1er essai (probablement inutile).
+			 * La version upstream ajoute 1 aux retries du A-MPDU
+			 * mais je ne comprends pas pourquoi.
+			 * Comme ci-dessus j'ai retiré le +1 pour corriger le
+			 * comptage, j'ajoute +1 une seule fois par trame au cas
+			 * ou le +1 cacherait un bug dans la version upstream.
+			 */
+			int try = !fi->retries; /* 1 si 1ere fois, 0 sinon */
 			if (txok || !an->sleeping)
 				ath_tx_set_retry(sc, txq, bf->bf_mpdu,
-						 retries);
+						 try + retries);
 
 			txpending = 1;
 		} else {

RE: Setting tx retry count in ath10k

[Toke Høiland-Jørgensen]

Jean-Pierre TOSONI <jp.tosoni@acksys.fr> writes:

> @Toke: As you can see in the patch, the value 30 was the fixed value
> defined in ath9k, I kept it for compatibility only (and that's why I
> wanted to make it configurable :-)

Yup, I'm aware that this is the default from ath9k; doesn't make it any
less wrong ;)

> On another hand, Minstrel in mac80211 seems to vary retries according
> to what you say, i.e. Minstrel tries to stay below a certain amount of
> time, but this only applies to short/long retries.

Right, haven't had time to poke into what exactly Minstrel does; have
just observed that it sometimes runs really long. But for most cases
it's not too bad...

-Toke

Re: Setting tx retry count in ath10k

[Benjamin Beichler]

Am 18.07.2018 um 19:01 schrieb Jean-Pierre TOSONI:
> Hi Ben,
>
> I attached the patch. Please remind that it applies to ath9k.
>
> At the end there are 3 comments in French, translation follows:
> 1) " longretry gives directly the transmit count, the +1 is useless.
>      Should rather have -1 to account for the first try"
> 2) " The retries just made for this frame must be added in to know
>      If the max was overstepped"
> 3) " Add the 1st try (probably useless).
>      Upstream version adds 1 to A-MPDU retries but I don't understand w=
hy.
>      Since above I removed the +1 to fix the count, I add +1 here once =
per
>      frame in case the "+1" is actually hiding a bug in the upstream ve=
rsion"
>
> @Toke: As you can see in the patch, the value 30 was the fixed value de=
fined
>  in ath9k, I kept it for compatibility only (and that's why I wanted to=
 make
>  it configurable :-)
> On another hand, Minstrel in mac80211 seems to vary retries according t=
o what
> you say, i.e. Minstrel tries to stay below a certain amount of time, bu=
t this
> only applies to short/long retries.
>
> Jean-Pierre
We also experienced the problems regarding the inconsistencies between
documentation and implementation. I will further throw in another set of
patches:
https://github.com/steinwurf/openwrt-patches/tree/master/openwrt-r41097/m=
ac80211

I think they have have similarities but also other aspects to really
restrict ath9k to the specified retry limits. This problem is as Toke
already stated additionally depended on minstrel. It is really sad, that
all this stuff is offtree and but there is the illusion of an setting in
the mainline kernel, which firstly will not set the value as documented
and different drivers will apply them (without warnings) in different way=
s.

>> For general internet traffic, a retry count of 30 is way too high; tha=
t
>> is up to 120 ms of HOL blocking latency. Better to just drop the packe=
t
>> at that point.
>>
>> Ideally, the retry count should be dynamically calculated in units of
>> time (which would depend on the rate and aggregate size), and also tak=
e
>> queueing time into account. I've been meaning to experiment with this
>> for minstrel and ath9k, but haven't gotten around to it...
We have running current research on this topic but focused on the
effects in 802.11s mesh networks. With multiple(forwarding) wireless
links, the retry limit have a bigger impact as only in managed mode
setups, since every forwarding step is doing repeated transmissions. But
I also totally agree, that the retry count needs to be considered in the
bufferbloat/airtime queuing stuff, which Toke proposed.

Nonetheless, since this ambiguities are known, wouldn't it be nice to
clean up all this patches to enable at least ath9k and ath10k to do the
right thing, or do anybody can tell why they weren't included the first
time ?


--=20
M.Sc. Benjamin Beichler

Universit=E4t Rostock, Fakult=E4t f=FCr Informatik und Elektrotechnik
Institut f=FCr Angewandte Mikroelektronik und Datentechnik

University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE

Richard-Wagner-Stra=DFe 31
18119 Rostock
Deutschland/Germany

phone: +49 (0) 381 498 - 7278
email: Benjamin.Beichler@uni-rostock.de
www: http://www.imd.uni-rostock.de/

Re: Setting tx retry count in ath10k

[Ben Greear]

On 07/19/2018 05:39 AM, Benjamin Beichler wrote:
> Am 18.07.2018 um 19:01 schrieb Jean-Pierre TOSONI:
>> Hi Ben,
>>
>> I attached the patch. Please remind that it applies to ath9k.
>>
>> At the end there are 3 comments in French, translation follows:
>> 1) " longretry gives directly the transmit count, the +1 is useless.
>>      Should rather have -1 to account for the first try"
>> 2) " The retries just made for this frame must be added in to know
>>      If the max was overstepped"
>> 3) " Add the 1st try (probably useless).
>>      Upstream version adds 1 to A-MPDU retries but I don't understand why.
>>      Since above I removed the +1 to fix the count, I add +1 here once per
>>      frame in case the "+1" is actually hiding a bug in the upstream version"
>>
>> @Toke: As you can see in the patch, the value 30 was the fixed value defined
>>  in ath9k, I kept it for compatibility only (and that's why I wanted to make
>>  it configurable :-)
>> On another hand, Minstrel in mac80211 seems to vary retries according to what
>> you say, i.e. Minstrel tries to stay below a certain amount of time, but this
>> only applies to short/long retries.
>>
>> Jean-Pierre
> We also experienced the problems regarding the inconsistencies between
> documentation and implementation. I will further throw in another set of
> patches:
> https://github.com/steinwurf/openwrt-patches/tree/master/openwrt-r41097/mac80211
>
> I think they have have similarities but also other aspects to really
> restrict ath9k to the specified retry limits. This problem is as Toke
> already stated additionally depended on minstrel. It is really sad, that
> all this stuff is offtree and but there is the illusion of an setting in
> the mainline kernel, which firstly will not set the value as documented
> and different drivers will apply them (without warnings) in different ways.
>
>>> For general internet traffic, a retry count of 30 is way too high; that
>>> is up to 120 ms of HOL blocking latency. Better to just drop the packet
>>> at that point.
>>>
>>> Ideally, the retry count should be dynamically calculated in units of
>>> time (which would depend on the rate and aggregate size), and also take
>>> queueing time into account. I've been meaning to experiment with this
>>> for minstrel and ath9k, but haven't gotten around to it...
> We have running current research on this topic but focused on the
> effects in 802.11s mesh networks. With multiple(forwarding) wireless
> links, the retry limit have a bigger impact as only in managed mode
> setups, since every forwarding step is doing repeated transmissions. But
> I also totally agree, that the retry count needs to be considered in the
> bufferbloat/airtime queuing stuff, which Toke proposed.
>
> Nonetheless, since this ambiguities are known, wouldn't it be nice to
> clean up all this patches to enable at least ath9k and ath10k to do the
> right thing, or do anybody can tell why they weren't included the first
> time ?

To change the topic slightly, has anyone even verified that ath10k
will do hardware (or firmware) retransmits for aggregated frames when doing block-ack?

I was expecting to find re-queue logic in the firmware when I looked the
other day, but I only saw it do re-queues for locally generated frames,
not stuff sent from the driver.  I may have just misread the code, however,
or maybe the hardware itself somehow magically retransmits dropped
aggregated frames based on the block-ack packets?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Setting tx retry count in ath10k

[Toke Høiland-Jørgensen]

Benjamin Beichler <Benjamin.Beichler@uni-rostock.de> writes:

>>> For general internet traffic, a retry count of 30 is way too high; that
>>> is up to 120 ms of HOL blocking latency. Better to just drop the packet
>>> at that point.
>>>
>>> Ideally, the retry count should be dynamically calculated in units of
>>> time (which would depend on the rate and aggregate size), and also take
>>> queueing time into account. I've been meaning to experiment with this
>>> for minstrel and ath9k, but haven't gotten around to it...
> We have running current research on this topic but focused on the
> effects in 802.11s mesh networks. With multiple(forwarding) wireless
> links, the retry limit have a bigger impact as only in managed mode
> setups, since every forwarding step is doing repeated transmissions.
> But I also totally agree, that the retry count needs to be considered
> in the bufferbloat/airtime queuing stuff, which Toke proposed.

Ah, cool. Looking forward to seeing your results. And yeah, it's
probably even worse in meshes...

> Nonetheless, since this ambiguities are known, wouldn't it be nice to
> clean up all this patches to enable at least ath9k and ath10k to do
> the right thing, or do anybody can tell why they weren't included the
> first time ?

No objection from me, certainly! :)

-Toke