Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
requests. When this limit is reached aggregation is turned off for
given TID permanently. This doesn't seem right: three requests is
not that much, some 'blackout' can happen, but effect of it affects
whole connection indefinitely.
This patch adds a period of time (1 minute) after which counter of
sent addba requests is reset so addba requests can be sent again.
The traffic impact should be negligible, but connection will be more
stable.
Signed-off-by: Nikolay Martynov <[email protected]>
---
net/mac80211/agg-tx.c | 21 ++++++++++++++++++---
net/mac80211/sta_info.h | 3 +++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 39d72cc..521638c 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -339,6 +339,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
#endif
spin_lock_bh(&sta->lock);
+ sta->ampdu_mlme.last_addba_req_time[tid] = jiffies_to_msecs(jiffies);
sta->ampdu_mlme.addba_req_num[tid]++;
spin_unlock_bh(&sta->lock);
@@ -389,10 +390,24 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
spin_lock_bh(&sta->lock);
- /* we have tried too many times, receiver does not want A-MPDU */
if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_MAX_RETRIES) {
- ret = -EBUSY;
- goto err_unlock_sta;
+ unsigned int timestamp = jiffies_to_msecs(jiffies);
+ if (timestamp - sta->ampdu_mlme.last_addba_req_time[tid] >
+ HT_AGG_RETRIES_PERIOD) {
+ /*
+ * last addba attempt was more then
+ * HT_AGG_RETRIES_PERIOD time ago,
+ * we can start trying again now
+ */
+ sta->ampdu_mlme.addba_req_num[tid] = 0;
+ } else {
+ /*
+ * we have tried too many times,
+ * receiver does not want A-MPDU (yet)
+ */
+ ret = -EBUSY;
+ goto err_unlock_sta;
+ }
}
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 6280e8b..a16d60a 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -76,6 +76,7 @@ enum ieee80211_sta_info_flags {
#define STA_TID_NUM 16
#define ADDBA_RESP_INTERVAL HZ
#define HT_AGG_MAX_RETRIES 0x3
+#define HT_AGG_RETRIES_PERIOD (60*1000)
#define HT_AGG_STATE_DRV_READY 0
#define HT_AGG_STATE_RESPONSE_RECEIVED 1
@@ -169,6 +170,7 @@ struct tid_ampdu_rx {
* @tid_tx: aggregation info for Tx per TID
* @tid_start_tx: sessions where start was requested
* @addba_req_num: number of times addBA request has been sent.
+ * @last_addba_req_time: timestamp of the last addBA request.
* @dialog_token_allocator: dialog token enumerator for each new session;
* @work: work struct for starting/stopping aggregation
* @tid_rx_timer_expired: bitmap indicating on which TIDs the
@@ -188,6 +190,7 @@ struct sta_ampdu_mlme {
struct work_struct work;
struct tid_ampdu_tx __rcu *tid_tx[STA_TID_NUM];
struct tid_ampdu_tx *tid_start_tx[STA_TID_NUM];
+ unsigned int last_addba_req_time[STA_TID_NUM];
u8 addba_req_num[STA_TID_NUM];
u8 dialog_token_allocator;
};
--
1.7.4.1
Hi,
2011/11/29 Johannes Berg <[email protected]>:
> Hi Nikolay,
>
>> > Conceptually, this seems OK to me, although on broken APs it might mean
>> > connection stalls every minute, not sure how desirable that is?
>>
>> ? Do APs broken is such way exist? I.e. APs which declare aggregation
>> support but do not respond to addba.
>
> I seem to remember something ... not really sure.
If such APs exist I think it might be possible to extend my patch to
do something like the following. For the first time make 10 attempts
to establish addba. If this fails - make no more attempts for the
duration of the connection. If this succeeds - use logic I've added in
the patch. This should handle broken APs and won't allow agg to be
disable because of some random blackout.
>
>> ? On the other hand looking at the code I've got an impression that
>> connection doesn't stall while waiting for addba resp, i.e. packets
>> still go though non-agg path. Did I miss something in this regards?
>
> We queue up packets in ieee80211_tx_prep_agg() when
> HT_AGG_STATE_WANT_START is clear but HT_AGG_STATE_OPERATIONAL is not
> set, this is the case after we send the addBA request frame and before
> we get a response. So since the timeout is 1 second, the connection can
> stall for quite a while.
Is the any particular reason why packets are not being sent via
non-agg path while agg path is being established? I'm not suggesting
that it is wrong, I'm just curious. The delay in data transmission
you've mentioned in regards to my patch is probably applicable here
too, so won't it make sense to ignore agg until it is fully set up?
Thanks!
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]
Hi.
2011/11/29 Johannes Berg <[email protected]>:
> On Tue, 2011-11-29 at 17:01 -0500, Nikolay Martynov wrote:
>
>> >> ? Do APs broken is such way exist? I.e. APs which declare aggregation
>> >> support but do not respond to addba.
>> >
>> > I seem to remember something ... not really sure.
>>
>> ? If such APs exist I think it might be possible to extend my patch to
>> do something like the following. For the first time make 10 attempts
>> to establish addba. If this fails - make no more attempts for the
>> duration of the connection. If this succeeds - use logic I've added in
>> the patch. This should handle broken APs and won't allow agg to be
>> disable because of some random blackout.
>
> Wouldn't that be equivalent to just bumping the number of tries?
Well, sort of if you are willing to bump it to 10-15, I guess it
would be kind of same thing.
I've seen couple of times when 3 wasn't enough and agg got disabled,
but 10-15 seems reasonably large to give up completely.
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]
Hi Nikolay,
> > Conceptually, this seems OK to me, although on broken APs it might mean
> > connection stalls every minute, not sure how desirable that is?
>
> Do APs broken is such way exist? I.e. APs which declare aggregation
> support but do not respond to addba.
I seem to remember something ... not really sure.
> On the other hand looking at the code I've got an impression that
> connection doesn't stall while waiting for addba resp, i.e. packets
> still go though non-agg path. Did I miss something in this regards?
We queue up packets in ieee80211_tx_prep_agg() when
HT_AGG_STATE_WANT_START is clear but HT_AGG_STATE_OPERATIONAL is not
set, this is the case after we send the addBA request frame and before
we get a response. So since the timeout is 1 second, the connection can
stall for quite a while.
johannes
On Mon, 2011-11-28 at 21:27 -0500, Nikolay Martynov wrote:
> Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
> requests. When this limit is reached aggregation is turned off for
> given TID permanently. This doesn't seem right: three requests is
> not that much, some 'blackout' can happen, but effect of it affects
> whole connection indefinitely.
> This patch adds a period of time (1 minute) after which counter of
> sent addba requests is reset so addba requests can be sent again.
> The traffic impact should be negligible, but connection will be more
> stable.
Conceptually, this seems OK to me, although on broken APs it might mean
connection stalls every minute, not sure how desirable that is?
> - /* we have tried too many times, receiver does not want A-MPDU */
> if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_MAX_RETRIES) {
> - ret = -EBUSY;
> - goto err_unlock_sta;
> + unsigned int timestamp = jiffies_to_msecs(jiffies);
> + if (timestamp - sta->ampdu_mlme.last_addba_req_time[tid] >
> + HT_AGG_RETRIES_PERIOD) {
this logic is confusing -- why ms? jiffies should be absolutely OK for
minute-long timeouts.
johannes
On Tue, 2011-11-29 at 17:28 -0500, Nikolay Martynov wrote:
> Hi.
>
> 2011/11/29 Johannes Berg <[email protected]>:
> > On Tue, 2011-11-29 at 17:01 -0500, Nikolay Martynov wrote:
> >
> >> >> Do APs broken is such way exist? I.e. APs which declare aggregation
> >> >> support but do not respond to addba.
> >> >
> >> > I seem to remember something ... not really sure.
> >>
> >> If such APs exist I think it might be possible to extend my patch to
> >> do something like the following. For the first time make 10 attempts
> >> to establish addba. If this fails - make no more attempts for the
> >> duration of the connection. If this succeeds - use logic I've added in
> >> the patch. This should handle broken APs and won't allow agg to be
> >> disable because of some random blackout.
> >
> > Wouldn't that be equivalent to just bumping the number of tries?
>
> Well, sort of if you are willing to bump it to 10-15, I guess it
> would be kind of same thing.
> I've seen couple of times when 3 wasn't enough and agg got disabled,
> but 10-15 seems reasonably large to give up completely.
Maybe the better approach would be to bump it up, but also only allow an
attempt once every say 15 seconds or so? It seems kinda useless to try 3
(or 10/15) times in quick succession and then give up, vs. just trying
more spaced out?
johannes
On Tue, 2011-11-29 at 17:01 -0500, Nikolay Martynov wrote:
> >> Do APs broken is such way exist? I.e. APs which declare aggregation
> >> support but do not respond to addba.
> >
> > I seem to remember something ... not really sure.
>
> If such APs exist I think it might be possible to extend my patch to
> do something like the following. For the first time make 10 attempts
> to establish addba. If this fails - make no more attempts for the
> duration of the connection. If this succeeds - use logic I've added in
> the patch. This should handle broken APs and won't allow agg to be
> disable because of some random blackout.
Wouldn't that be equivalent to just bumping the number of tries?
> >> On the other hand looking at the code I've got an impression that
> >> connection doesn't stall while waiting for addba resp, i.e. packets
> >> still go though non-agg path. Did I miss something in this regards?
> >
> > We queue up packets in ieee80211_tx_prep_agg() when
> > HT_AGG_STATE_WANT_START is clear but HT_AGG_STATE_OPERATIONAL is not
> > set, this is the case after we send the addBA request frame and before
> > we get a response. So since the timeout is 1 second, the connection can
> > stall for quite a while.
>
> Is the any particular reason why packets are not being sent via
> non-agg path while agg path is being established? I'm not suggesting
> that it is wrong, I'm just curious. The delay in data transmission
> you've mentioned in regards to my patch is probably applicable here
> too, so won't it make sense to ignore agg until it is fully set up?
It's required because you tell the peer what the first aggregated
packet's sequence number is going to be.
johannes
Hi.
Thanks for your comments!
2011/11/29 Johannes Berg <[email protected]>:
> On Mon, 2011-11-28 at 21:27 -0500, Nikolay Martynov wrote:
>> Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
>> requests. When this limit is reached aggregation is turned off for
>> given TID permanently. This doesn't seem right: three requests is
>> not that much, some 'blackout' can happen, but effect of it affects
>> whole connection indefinitely.
>> ? This patch adds a period of time (1 minute) after which counter of
>> sent addba requests is reset so addba requests can be sent again.
>> ? The traffic impact should be negligible, but connection will be more
>> stable.
>
> Conceptually, this seems OK to me, although on broken APs it might mean
> connection stalls every minute, not sure how desirable that is?
Do APs broken is such way exist? I.e. APs which declare aggregation
support but do not respond to addba.
On the other hand looking at the code I've got an impression that
connection doesn't stall while waiting for addba resp, i.e. packets
still go though non-agg path. Did I miss something in this regards?
>
>> - ? ? /* we have tried too many times, receiver does not want A-MPDU */
>> ? ? ? if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_MAX_RETRIES) {
>> - ? ? ? ? ? ? ret = -EBUSY;
>> - ? ? ? ? ? ? goto err_unlock_sta;
>> + ? ? ? ? ? ? unsigned int timestamp = jiffies_to_msecs(jiffies);
>> + ? ? ? ? ? ? if (timestamp - sta->ampdu_mlme.last_addba_req_time[tid] >
>> + ? ? ? ? ? ? ? ? HT_AGG_RETRIES_PERIOD) {
>
> this logic is confusing -- why ms? jiffies should be absolutely OK for
> minute-long timeouts.
Sure, I'll change this to use jiffies.
Thanks.
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]
2011/11/30 Johannes Berg <[email protected]>:
> On Tue, 2011-11-29 at 17:46 -0500, Nikolay Martynov wrote:
>
>> > Maybe the better approach would be to bump it up, but also only allow an
>> > attempt once every say 15 seconds or so? It seems kinda useless to try 3
>> > (or 10/15) times in quick succession and then give up, vs. just trying
>> > more spaced out?
>>
>> ? Makes sense to me. Although I would left first 2-3 go quickly in
>> case first was lost due to random glitch there is no reason to wait 15
>> seconds. But if first 2-3 fail - it make sense split next apart, I
>> think.
>
> Seems ok to me, want to implement it? :)
Sure, I'll send patch for this.
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]
2011/11/29 Johannes Berg <[email protected]>:
> On Tue, 2011-11-29 at 17:28 -0500, Nikolay Martynov wrote:
>> Hi.
>>
>> 2011/11/29 Johannes Berg <[email protected]>:
>> > On Tue, 2011-11-29 at 17:01 -0500, Nikolay Martynov wrote:
>> >
>> >> >> ? Do APs broken is such way exist? I.e. APs which declare aggregation
>> >> >> support but do not respond to addba.
>> >> >
>> >> > I seem to remember something ... not really sure.
>> >>
>> >> ? If such APs exist I think it might be possible to extend my patch to
>> >> do something like the following. For the first time make 10 attempts
>> >> to establish addba. If this fails - make no more attempts for the
>> >> duration of the connection. If this succeeds - use logic I've added in
>> >> the patch. This should handle broken APs and won't allow agg to be
>> >> disable because of some random blackout.
>> >
>> > Wouldn't that be equivalent to just bumping the number of tries?
>>
>> ? Well, sort of if you are willing to bump it to 10-15, I guess it
>> would be kind of same thing.
>> ? I've seen couple of times when 3 wasn't enough and agg got disabled,
>> but 10-15 seems reasonably large to give up completely.
>
> Maybe the better approach would be to bump it up, but also only allow an
> attempt once every say 15 seconds or so? It seems kinda useless to try 3
> (or 10/15) times in quick succession and then give up, vs. just trying
> more spaced out?
Makes sense to me. Although I would left first 2-3 go quickly in
case first was lost due to random glitch there is no reason to wait 15
seconds. But if first 2-3 fail - it make sense split next apart, I
think.
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]
On Tue, 2011-11-29 at 17:46 -0500, Nikolay Martynov wrote:
> > Maybe the better approach would be to bump it up, but also only allow an
> > attempt once every say 15 seconds or so? It seems kinda useless to try 3
> > (or 10/15) times in quick succession and then give up, vs. just trying
> > more spaced out?
>
> Makes sense to me. Although I would left first 2-3 go quickly in
> case first was lost due to random glitch there is no reason to wait 15
> seconds. But if first 2-3 fail - it make sense split next apart, I
> think.
Seems ok to me, want to implement it? :)
johannes
On Thu, Dec 08, 2011 at 10:43:41PM -0500, Nikolay Martynov wrote:
> Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
> requests. When this limit is reached aggregation is turned off for
> given TID permanently. This doesn't seem right: three requests is
> not that much, some 'blackout' can happen, but effect of it affects
> whole connection indefinitely.
> This patch increases number of retries to 15. Also, when there have
> been 3 or more retries it splits further retries apart by 15 seconds
> instead of sending them in very short period of time.
>
> Signed-off-by: Nikolay Martynov <[email protected]>
> ---
> net/mac80211/agg-tx.c | 19 +++++++++++++++++++
> net/mac80211/sta_info.h | 6 +++++-
> 2 files changed, 24 insertions(+), 1 deletions(-)
>
> diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
> index 7380287..7060c8f 100644
> --- a/net/mac80211/agg-tx.c
> +++ b/net/mac80211/agg-tx.c
> @@ -390,6 +390,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
> #endif
>
> spin_lock_bh(&sta->lock);
> + sta->ampdu_mlme.last_addba_req_time[tid] = jiffies;
> sta->ampdu_mlme.addba_req_num[tid]++;
> spin_unlock_bh(&sta->lock);
>
> @@ -490,6 +491,24 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
> goto err_unlock_sta;
> }
>
> + /*
> + * if we have tried more than HT_AGG_BURST_RETRIES times we
> + * will spread our requests in time to avoid stalling connection
> + * for too long
> + */
> + if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_BURST_RETRIES &&
> + time_before(jiffies, sta->ampdu_mlme.last_addba_req_time[tid] +
> + HT_AGG_RETRIES_PERIOD)) {
> +#ifdef CONFIG_MAC80211_HT_DEBUG
> + printk(KERN_DEBUG "BA request denied - "
> + "waiting a grace period after %d failed requests "
> + "on tid %u\n",
> + sta->ampdu_mlme.addba_req_num[tid], tid);
> +#endif /* CONFIG_MAC80211_HT_DEBUG */
> + ret = -EBUSY;
> + goto err_unlock_sta;
> + }
> +
> tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
> /* check if the TID is not in aggregation flow already */
> if (tid_tx || sta->ampdu_mlme.tid_start_tx[tid]) {
CC [M] net/mac80211/agg-tx.o
net/mac80211/agg-tx.c: In function ‘ieee80211_start_tx_ba_session’:
net/mac80211/agg-tx.c:474:6: warning: comparison of distinct pointer types lacks a cast
No new warnings, please!
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 1a14fab..c8578eb 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -75,7 +75,9 @@ enum ieee80211_sta_info_flags {
>
> #define STA_TID_NUM 16
> #define ADDBA_RESP_INTERVAL HZ
> -#define HT_AGG_MAX_RETRIES 0x3
> +#define HT_AGG_MAX_RETRIES 15
> +#define HT_AGG_BURST_RETRIES 3
> +#define HT_AGG_RETRIES_PERIOD (15 * HZ)
>
> #define HT_AGG_STATE_DRV_READY 0
> #define HT_AGG_STATE_RESPONSE_RECEIVED 1
> @@ -171,6 +173,7 @@ struct tid_ampdu_rx {
> * @tid_tx: aggregation info for Tx per TID
> * @tid_start_tx: sessions where start was requested
> * @addba_req_num: number of times addBA request has been sent.
> + * @last_addba_req_time: timestamp of the last addBA request.
> * @dialog_token_allocator: dialog token enumerator for each new session;
> * @work: work struct for starting/stopping aggregation
> * @tid_rx_timer_expired: bitmap indicating on which TIDs the
> @@ -190,6 +193,7 @@ struct sta_ampdu_mlme {
> struct work_struct work;
> struct tid_ampdu_tx __rcu *tid_tx[STA_TID_NUM];
> struct tid_ampdu_tx *tid_start_tx[STA_TID_NUM];
> + unsigned int last_addba_req_time[STA_TID_NUM];
> u8 addba_req_num[STA_TID_NUM];
> u8 dialog_token_allocator;
> };
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Fri, Dec 9, 2011 at 4:43 AM, Nikolay Martynov <[email protected]> wrote:
> ?Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
> requests. When this limit is reached aggregation is turned off for
> given TID permanently. This doesn't seem right: three requests is
> not that much, some 'blackout' can happen, but effect of it affects
> whole connection indefinitely.
> ?This patch increases number of retries to 15. Also, when there have
> been 3 or more retries it splits further retries apart by 15 seconds
> instead of sending them in very short period of time.
This is not 100% related to your patch but would it also make sense to
disallow BA session setup when it was just torn down a second ago?
Helmut
Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
requests. When this limit is reached aggregation is turned off for
given TID permanently. This doesn't seem right: three requests is
not that much, some 'blackout' can happen, but effect of it affects
whole connection indefinitely.
This patch increases number of retries to 15. Also, when there have
been 3 or more retries it splits further retries apart by 15 seconds
instead of sending them in very short period of time.
Signed-off-by: Nikolay Martynov <[email protected]>
---
net/mac80211/agg-tx.c | 19 +++++++++++++++++++
net/mac80211/sta_info.h | 6 +++++-
2 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 2c2e951..8906d14 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -360,6 +360,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
#endif
spin_lock_bh(&sta->lock);
+ sta->ampdu_mlme.last_addba_req_time[tid] = jiffies;
sta->ampdu_mlme.addba_req_num[tid]++;
spin_unlock_bh(&sta->lock);
@@ -438,6 +439,24 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
goto err_unlock_sta;
}
+ /*
+ * if we have tried more than HT_AGG_BURST_RETRIES times we
+ * will spread our requests in time to avoid stalling connection
+ * for too long
+ */
+ if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_BURST_RETRIES &&
+ jiffies - sta->ampdu_mlme.last_addba_req_time[tid] <
+ HT_AGG_RETRIES_PERIOD) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+ printk(KERN_DEBUG "BA request denied - "
+ "waiting a grace period after %d failed requests "
+ "on tid %u\n",
+ sta->ampdu_mlme.addba_req_num[tid], tid);
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+ ret = -EBUSY;
+ goto err_unlock_sta;
+ }
+
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
/* check if the TID is not in aggregation flow already */
if (tid_tx || sta->ampdu_mlme.tid_start_tx[tid]) {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 1a14fab..c8578eb 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -75,7 +75,9 @@ enum ieee80211_sta_info_flags {
#define STA_TID_NUM 16
#define ADDBA_RESP_INTERVAL HZ
-#define HT_AGG_MAX_RETRIES 0x3
+#define HT_AGG_MAX_RETRIES 15
+#define HT_AGG_BURST_RETRIES 3
+#define HT_AGG_RETRIES_PERIOD (15 * HZ)
#define HT_AGG_STATE_DRV_READY 0
#define HT_AGG_STATE_RESPONSE_RECEIVED 1
@@ -171,6 +173,7 @@ struct tid_ampdu_rx {
* @tid_tx: aggregation info for Tx per TID
* @tid_start_tx: sessions where start was requested
* @addba_req_num: number of times addBA request has been sent.
+ * @last_addba_req_time: timestamp of the last addBA request.
* @dialog_token_allocator: dialog token enumerator for each new session;
* @work: work struct for starting/stopping aggregation
* @tid_rx_timer_expired: bitmap indicating on which TIDs the
@@ -190,6 +193,7 @@ struct sta_ampdu_mlme {
struct work_struct work;
struct tid_ampdu_tx __rcu *tid_tx[STA_TID_NUM];
struct tid_ampdu_tx *tid_start_tx[STA_TID_NUM];
+ unsigned int last_addba_req_time[STA_TID_NUM];
u8 addba_req_num[STA_TID_NUM];
u8 dialog_token_allocator;
};
--
1.7.4.1
On Mon, 2011-12-05 at 22:03 -0500, Nikolay Martynov wrote:
> Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
> requests. When this limit is reached aggregation is turned off for
> given TID permanently. This doesn't seem right: three requests is
> not that much, some 'blackout' can happen, but effect of it affects
> whole connection indefinitely.
> This patch increases number of retries to 15. Also, when there have
> been 3 or more retries it splits further retries apart by 15 seconds
> instead of sending them in very short period of time.
>
> Signed-off-by: Nikolay Martynov <[email protected]>
> + if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_BURST_RETRIES &&
> + jiffies - sta->ampdu_mlme.last_addba_req_time[tid] <
> + HT_AGG_RETRIES_PERIOD) {
You should use the time_after or time_before macros to avoid issues with
jiffies wrapping. Other than that looks good to me.
johannes
Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
requests. When this limit is reached aggregation is turned off for
given TID permanently. This doesn't seem right: three requests is
not that much, some 'blackout' can happen, but effect of it affects
whole connection indefinitely.
This patch increases number of retries to 15. Also, when there have
been 3 or more retries it splits further retries apart by 15 seconds
instead of sending them in very short period of time.
Signed-off-by: Nikolay Martynov <[email protected]>
---
net/mac80211/agg-tx.c | 19 +++++++++++++++++++
net/mac80211/sta_info.h | 6 +++++-
2 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index e92f98d..76be617 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -392,6 +392,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
#endif
spin_lock_bh(&sta->lock);
+ sta->ampdu_mlme.last_addba_req_time[tid] = jiffies;
sta->ampdu_mlme.addba_req_num[tid]++;
spin_unlock_bh(&sta->lock);
@@ -492,6 +493,24 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
goto err_unlock_sta;
}
+ /*
+ * if we have tried more than HT_AGG_BURST_RETRIES times we
+ * will spread our requests in time to avoid stalling connection
+ * for too long
+ */
+ if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_BURST_RETRIES &&
+ time_before(jiffies, sta->ampdu_mlme.last_addba_req_time[tid] +
+ HT_AGG_RETRIES_PERIOD)) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+ printk(KERN_DEBUG "BA request denied - "
+ "waiting a grace period after %d failed requests "
+ "on tid %u\n",
+ sta->ampdu_mlme.addba_req_num[tid], tid);
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+ ret = -EBUSY;
+ goto err_unlock_sta;
+ }
+
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
/* check if the TID is not in aggregation flow already */
if (tid_tx || sta->ampdu_mlme.tid_start_tx[tid]) {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 15b3bb7..dee2842 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -83,7 +83,9 @@ enum ieee80211_sta_state {
#define STA_TID_NUM 16
#define ADDBA_RESP_INTERVAL HZ
-#define HT_AGG_MAX_RETRIES 0x3
+#define HT_AGG_MAX_RETRIES 15
+#define HT_AGG_BURST_RETRIES 3
+#define HT_AGG_RETRIES_PERIOD (15 * HZ)
#define HT_AGG_STATE_DRV_READY 0
#define HT_AGG_STATE_RESPONSE_RECEIVED 1
@@ -179,6 +181,7 @@ struct tid_ampdu_rx {
* @tid_tx: aggregation info for Tx per TID
* @tid_start_tx: sessions where start was requested
* @addba_req_num: number of times addBA request has been sent.
+ * @last_addba_req_time: timestamp of the last addBA request.
* @dialog_token_allocator: dialog token enumerator for each new session;
* @work: work struct for starting/stopping aggregation
* @tid_rx_timer_expired: bitmap indicating on which TIDs the
@@ -198,6 +201,7 @@ struct sta_ampdu_mlme {
struct work_struct work;
struct tid_ampdu_tx __rcu *tid_tx[STA_TID_NUM];
struct tid_ampdu_tx *tid_start_tx[STA_TID_NUM];
+ unsigned long last_addba_req_time[STA_TID_NUM];
u8 addba_req_num[STA_TID_NUM];
u8 dialog_token_allocator;
};
--
1.7.4.1
Currently code allows three (HT_AGG_MAX_RETRIES) unanswered addba
requests. When this limit is reached aggregation is turned off for
given TID permanently. This doesn't seem right: three requests is
not that much, some 'blackout' can happen, but effect of it affects
whole connection indefinitely.
This patch increases number of retries to 15. Also, when there have
been 3 or more retries it splits further retries apart by 15 seconds
instead of sending them in very short period of time.
Signed-off-by: Nikolay Martynov <[email protected]>
---
net/mac80211/agg-tx.c | 19 +++++++++++++++++++
net/mac80211/sta_info.h | 6 +++++-
2 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 7380287..7060c8f 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -390,6 +390,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
#endif
spin_lock_bh(&sta->lock);
+ sta->ampdu_mlme.last_addba_req_time[tid] = jiffies;
sta->ampdu_mlme.addba_req_num[tid]++;
spin_unlock_bh(&sta->lock);
@@ -490,6 +491,24 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
goto err_unlock_sta;
}
+ /*
+ * if we have tried more than HT_AGG_BURST_RETRIES times we
+ * will spread our requests in time to avoid stalling connection
+ * for too long
+ */
+ if (sta->ampdu_mlme.addba_req_num[tid] > HT_AGG_BURST_RETRIES &&
+ time_before(jiffies, sta->ampdu_mlme.last_addba_req_time[tid] +
+ HT_AGG_RETRIES_PERIOD)) {
+#ifdef CONFIG_MAC80211_HT_DEBUG
+ printk(KERN_DEBUG "BA request denied - "
+ "waiting a grace period after %d failed requests "
+ "on tid %u\n",
+ sta->ampdu_mlme.addba_req_num[tid], tid);
+#endif /* CONFIG_MAC80211_HT_DEBUG */
+ ret = -EBUSY;
+ goto err_unlock_sta;
+ }
+
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
/* check if the TID is not in aggregation flow already */
if (tid_tx || sta->ampdu_mlme.tid_start_tx[tid]) {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 1a14fab..c8578eb 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -75,7 +75,9 @@ enum ieee80211_sta_info_flags {
#define STA_TID_NUM 16
#define ADDBA_RESP_INTERVAL HZ
-#define HT_AGG_MAX_RETRIES 0x3
+#define HT_AGG_MAX_RETRIES 15
+#define HT_AGG_BURST_RETRIES 3
+#define HT_AGG_RETRIES_PERIOD (15 * HZ)
#define HT_AGG_STATE_DRV_READY 0
#define HT_AGG_STATE_RESPONSE_RECEIVED 1
@@ -171,6 +173,7 @@ struct tid_ampdu_rx {
* @tid_tx: aggregation info for Tx per TID
* @tid_start_tx: sessions where start was requested
* @addba_req_num: number of times addBA request has been sent.
+ * @last_addba_req_time: timestamp of the last addBA request.
* @dialog_token_allocator: dialog token enumerator for each new session;
* @work: work struct for starting/stopping aggregation
* @tid_rx_timer_expired: bitmap indicating on which TIDs the
@@ -190,6 +193,7 @@ struct sta_ampdu_mlme {
struct work_struct work;
struct tid_ampdu_tx __rcu *tid_tx[STA_TID_NUM];
struct tid_ampdu_tx *tid_start_tx[STA_TID_NUM];
+ unsigned int last_addba_req_time[STA_TID_NUM];
u8 addba_req_num[STA_TID_NUM];
u8 dialog_token_allocator;
};
--
1.7.4.1
Am Freitag, 9. Dezember 2011, 23:32:55 schrieb Nikolay Martynov:
> Hi,
>
> > This is not 100% related to your patch but would it also make sense to
> > disallow BA session setup when it was just torn down a second ago?
>
> I do not think this makes much sense. BA session is created when
> connection is being used (i.e. there is something to aggregate). And
> this could happen at any time, even right after BA session was torn
> down after timeout.
Yep, sure. I thought more about a case where the receiving peer wants us
to tear down the BA session for whatever reason (some Windows clients seem
to do that in some cases) ...
Helmut