On Tue, Sep 16, 2008 at 12:08 PM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > 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. Are you cooking any code? > 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
