2011-11-23 18:36:41

by Janusz Dziedzic

Subject: mac80211: AP mode - question about Tx buffered unicast frames drop timeout

Hello,

in sta_info.c I see function we call to remove expired unicast buffered frames.

sta_info_buffer_expired()
{
...
timeout = (sta->listen_interval *
sta->sdata->vif.bss_conf.beacon_int *
32 / 15625) * HZ;
if (timeout < STA_TX_BUFFER_EXPIRE)
timeout = STA_TX_BUFFER_EXPIRE;
...
}

STA_TX_BUFFER_EXPIRE is define as 10 seconds.
Do you remember why we set this as 10 seconds in case we calculate lower value?
This "listen_interval" calculation seems to be correct.
I found this during UAPSD debuging in AP mode.

BR
Janusz

2011-11-24 13:03:51

by Johannes Berg

Subject: Re: mac80211: AP mode - question about Tx buffered unicast frames drop timeout

On Wed, 2011-11-23 at 19:36 +0100, Janusz Dziedzic wrote:
> Hello,
>
> in sta_info.c I see function we call to remove expired unicast buffered frames.
>
> sta_info_buffer_expired()
> {
> ...
> timeout = (sta->listen_interval *
> sta->sdata->vif.bss_conf.beacon_int *
> 32 / 15625) * HZ;
> if (timeout < STA_TX_BUFFER_EXPIRE)
> timeout = STA_TX_BUFFER_EXPIRE;
> ...
> }
>
> STA_TX_BUFFER_EXPIRE is define as 10 seconds.
> Do you remember why we set this as 10 seconds in case we calculate lower value?
> This "listen_interval" calculation seems to be correct.
> I found this during UAPSD debuging in AP mode.

I have no idea. It's probably just really old code. Now -- why would you
sleep for longer than 10 seconds? That's pretty bad user experience :)
But really, I don't know, and I wouldn't mind changing it either I
think.

johannes

2011-11-24 21:28:13

by Jouni Malinen

Subject: Re: mac80211: AP mode - question about Tx buffered unicast frames drop timeout

On Wed, Nov 23, 2011 at 07:36:40PM +0100, Janusz Dziedzic wrote:
> in sta_info.c I see function we call to remove expired unicast buffered frames.
>
> sta_info_buffer_expired()
> {
> ...
> timeout = (sta->listen_interval *
> sta->sdata->vif.bss_conf.beacon_int *
> 32 / 15625) * HZ;
> if (timeout < STA_TX_BUFFER_EXPIRE)
> timeout = STA_TX_BUFFER_EXPIRE;
> ...
> }
>
> STA_TX_BUFFER_EXPIRE is define as 10 seconds.
> Do you remember why we set this as 10 seconds in case we calculate lower value?

Too old code to remember.. That's from almost 10 years ago, I think.

> This "listen_interval" calculation seems to be correct.

I would not agree with that. It seems to result in timeout being 0 in
most cases. (unsigned int) sta->listen_interval *
sta->sdata->vif.bss_conf.beacon_int * 32 / 125 * HZ / 125 could be a
more useful order for the calculation. Or maybe use something like
min(sta->listen_interval *
sta->sdata->vif.bss_conf.beacon_int, 100000) to avoid integer overflow
in more or less theoretical cases without having to split the operation
that much..

Anyway, yes, it should be fine to make STA_TX_BUFFER_EXPIRE smaller.

--
Jouni Malinen PGP id EFC895FA