Hi, Thanks for spending your time looking at my patch and providing feedback! 2011/11/24 Johannes Berg <johannes@xxxxxxxxxxxxxxxx>: > On Tue, 2011-11-22 at 21:50 -0500, Nikolay Martynov wrote: >> Currently tx aggregation is not being timed out even if timeout is >> specified when aggregation is opened. Tx tid stays active until delba >> arrives from recipient (i.e. recipient times out tid when it is >> inactive). >> The problem with this approach is that delba can get lost in the air >> and tx tid will stay perpetually opened on the originator while closed >> on recipient thus all data sent via this tid will be lost. >> The problem manifests itself with connection becoming slow/unusable >> with ping times jumping to 4s. At such time opened tx tid can be seen >> on one side of the connection without corresponding rx tid one the >> other side. This seems to be happening quite often soon after >> connection on ar9102 I have. >> This patch implements tx tid timeouting in way very similar to rx tid >> timeouting. >> >> All comments and suggestions are appreciated. > > Looks OK to me. Did you run it through sparse too? :-) > > johannes > > Please forgive my lack of experience, but could you please point me to some docs where I can read about sparse and how to use it? I'd really appreciate this. Also, I one more thing I forgot to mention in original cover letter. Some time before this patch I've posted patch for ath9k: "[PATCH v3] ath9k: improve ath_tx_aggr_stop to avoid TID stuck in cleanup state". I've noticed that bug fixed in ath9k patch is triggered much more often when this patch to mac80211 applied. Probably because of some timing issues when TID is being closed from both ends at about same time. What I mean to say is that this patch to mac80211 should probably be applied after that patch to ath9k to avoid making ath9k less stable. And one more thing, if you do not mind. 'ieee80211_stop_tx_ba_session' tests tx_tid->state for HT_AGG_STATE_STOPPING holding only spinlock on sta->lock. '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on sta->lock is not being held. It seems to be that this could lead to problems if these two functions get called at about same time (which could easily happen when this patch is applied). Although actual window when lock is not help is really small. But I think the way it is currently done could still lead to problems. I think 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up to be covered by spin lock. I can include this as a part of next version of my patch. I'd really appreciate your thoughts about this. Thanks! -- Truthfully yours, Martynov Nikolay. Email: mar.kolya@xxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
