Tomas Winkler wrote: > On Tue, Sep 16, 2008 at 11:07 AM, Johannes Berg > <johannes@xxxxxxxxxxxxxxxx> wrote: >> On Tue, 2008-09-16 at 10:35 +0300, Tomas Winkler wrote: >>> Re-enable aggregation by addressing skb->cb overwrites >>> after insertion into the qdisc. Aggregation was disabled >>> after the new TX multiqueue changes were introduced. Instead >>> of relying on the skb->cb we use two flags on the skb. >>> >>> Signed-off-by: Luis R. Rodriguez <lrodriguez@xxxxxxxxxxx> >>> --- >>> >>> Users have been reporting low rates on 2.6.27 with 11n drivers, the >>> problem is been 11n aggregation was disabled due to the new TX multique >>> changes. We should have addressed this sooner but we just got to it >>> now. >>> >>> Without addressing this we won't get 11n aggregation on 2.6.27. I tried >>> to minimize the changes required. I'm about to test this, it compiles. >>> >>> If this doesn't get upstream for 27 perhaps distributions are willing >>> to >>> carry it around then and if so oh well (grr...). >>> >>> V2: >>> 1. removed skbuff ->is_part_ampdu >>> 2. fixed compilation warnings >> >> Looks better. However, still problems remain. >> >> ieee80211_start_tx_ba_session has a comment saying >> >> /* No need to requeue the packets in the agg queue, since >> we >> * held the tx lock: no packet could be enqueued to the >> newly >> * allocated queue */ >> >> which is clearly no longer true since there is no TX lock. You should >> maintain a similar invariant instead, namely that sta->tid_to_tx_q[tid] >> isn't updated until after ampdu_action(), in which case also no frames >> will go to the aggregation queue. >> >> However, to avoid reordering issues at aggregation start, you also need >> to stop the AC queue the TID falls into, to restart it only after >> ieee80211_requeue. This is tricky as it might be stopped due to a scan >> starting at the same time, so additional bookkeeping about who stopped a >> queue for what reason will be needed so you don't enable a queue that >> the scan has stopped or vice versa. >> >> As for ieee80211_requeue, it will need to be run from process context to >> be able to synchronize_rcu() at the beginning of the function to make >> sure any concurrent select_queue() has finished. That way, it can be >> sure that all frames on the queues will be properly classified: when >> aggregation was just _enabled_ frames after synchronize_rcu() will go to >> the aggregation queue and we can safely requeue the frames on the AC >> queue; when aggregation was just _disabled_ after synchronize_rcu() no >> frames will go to the aggregation queue any more. >> >> Note that for both cases both queues have to be stopped, the aggregation >> queue is easy to handle since it can just start out in stopped until >> ieee80211_requeue is run from the workqueue, but for the AC queue as I >> mentioned this requires extra bookkeeping to not collide with the driver >> or the scan stopping queues. Incidentally, it looks as though the scan >> might collide with the driver here, the fact that we allow pending >> packets probably rescues us here, but we cannot rely on that for >> aggregation because there we need it for correct ordering. >> >> I think that covers it all, but I might be wrong. > > Try to address these. Scan is no issue if HW scan is enabled, haven't > look if athk9 is using sw or hw scan. ath9k is sw scan. I just had another idea, it might be possible to do everything under the sta spinlock to avoid having to stop queues, but I'm not sure that can work out. johannes -- 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
