mac80211 aggregation (was: iwlwifi aggregation info)

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 4149 bytes --]

On Thu, 2008-08-07 at 01:31 +0300, Tomas Winkler wrote:
> On Fri, Aug 1, 2008 at 3:54 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Fri, 2008-08-01 at 15:40 +0300, Tomas Winkler wrote:
> >> > Right. So why are you saying we should have a separate qdisc for it?
> >>
> >> I need a sw queue for it.
> >
> > Sure, you need to buffer packets, but that's a whole different beast.
> 
> So this is the most important point of this conversation... What do
> you call this beast!

Ok I see where we're going. You're thinking of "struct
sk_buff_head" (which is part of most qdiscs) while I always thought you
actually wanted a qdisc.

> What I'm trying to say all the way along I don't need qdisc per se I
> need like 80% of it.

Right.

> So it was already implemented in qdisc so it was utilized.
> I'm not aware of anything else that is already implemented but you can
> always open my eyes and I'm not trying to be cynical.

Well there's struct sk_buff_head that allows you to queue up frames
anywhere you want them to be, we use it for example for PS-buffered
frames. Of course that means we need to implement it ourselves.

> This is really theoretical question. Wireless has defined 4 AC stream
> for simplicity
> so you map 2 TIDs out of 8 into 4 AC stream. One possible view
> of aggregation you don't map TID into AC stream but run it along to
> get more fine grained.
> Otherwise why don't we do <AC, RA> but <TID,RA> . Just a possible
> view. after all it's mapped back to AC.

Well 802.11 isn't really clear about this, it talks about QSTAs having
no more than one frame per <TID,RA,SA> outstanding at a time. I don't
have access to wi-fi documents but I suspect they add further
constraints because it's not feasible to have that many packets buffered
on the hardware.

> >> Not scheduling mean not string to prioritize streams in SW. I guess it means RR.
> >
> > But that's entirely user dependent if we allow actual qdiscs.
> 
> True but there is s always some sane default.

So we should disallow changing qdiscs? I'm not entirely against that,
but I think if we go that way we should force it in the code.

> I suggest to start at some more productive point.
> So what is important is that we agree on one thing that we need sw
> queue/buffer (forget qdisc for now) for aggregation queue and the
> question is how to implement it efficiently  and how to resolve
> cleanly the lack of requeueing in the new MQ. .

Well we can clearly do requeuing as much as we like, but we lose the
skb->cb over it. In fact, the master_start_xmit now makes that explicit
by memsetting it to 0.

I've looked into the Atheros driver, they way the implement it is that
when frames for an aggregation session are passed to the ->tx() call
they simply buffer them in software and when they have enough they
enqueue all of them to the hardware queue for the particular AC they map
into. This scales much better, with 16 TIDs and many stations you can
that way possibly support a lot of concurrent aggregation sessions.

Anyway, I'm confident you can work it out, I don't have time any more
for it right now, as I'll announce in a separate email. I had a patch
that mostly went the "just have queues per AC" route, but it was
incomplete, if anybody wants to take a look it's at
http://johannes.sipsolutions.net/patches/kernel/all/2008-08-04-12%3a26/006-fix-skb-cb-agg.patch

Here's a recap of the current issues that I'm aware of:

      * select_queue cannot really look into any state because due to
        the lack of a single TX lock there's nothing to prevent it
        racing against the code starting/stopping the aggregation
      * you cannot re-queue and assume that skb->cb will be left intact,
        but that's codified now by clearing skb->cb on master_start_xmit
      * the current implementation of ieee80211_requeue is racy against
        select_queue, select_queue is called under RCU lock though so
        you can use synchronize_rcu to avoid the race, this requires
        moving ieee80211_requeue into process context though

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]