Re: Another fragmentation multiqueue kludge

[Johannes Berg <johannes@sipsolutions.net>]

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

On Thu, 2008-07-24 at 17:37 +0300, Tomas Winkler wrote:

> >> -       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> >> +       struct ieee80211_tx_info *info;
> >>         int ret, i;
> >>
> >> -       if (netif_subqueue_stopped(local->mdev, skb))
> >> -               return IEEE80211_TX_AGAIN;
> >>
> >>         if (skb) {
> >> +               if (netif_subqueue_stopped(local->mdev, skb))
> >> +                       return IEEE80211_TX_AGAIN;
> >> +               info = IEEE80211_SKB_CB(skb);
> >> +
> >
> > This isn't right. It means that if you have a stopped queue and pending
> > fragments, you still transmit them, which is not something the driver
> > should have to expect.
> 
> This logic was wrong you cannot access skb and then ask if'ts not
> NULL. (it actually creates nice panic in fragmentation)

Oh, indeed, and since the same check is actually in the if
(tx->extra_frag) part, this is fine.

> >>         if (test_bit(queue, local->queues_pending)) {
> >> +               /* don't call scheduler just open the queue */
> >> +               if (ieee80211_is_multiqueue(local)) {
> >> +                       netif_start_subqueue(local->mdev, queue);
> >> +               } else {
> >> +                       WARN_ON(queue != 0);
> >> +                       netif_start_queue(local->mdev);
> >> +               }
> >>                 tasklet_schedule(&local->tx_pending_tasklet);
> >>         } else {
> >>                 if (ieee80211_is_multiqueue(local)) {
> >
> > That's not right either, if you have pending fragments/packets then you
> > cannot start the queue.
> 
> Exactly this what I wrote are my concern on the other hand
> that's the kludge because the __ieee80211_tx checks whether the queue
> is started so there is no way
> the pending packet is transmitted if you don't start the queue. I have
> new version of the patch that starts
> the queue only in the tasklet. Tasklet is locks tx so the next packet
> will come only after the pending packet is transmitted.

What happens if you just invert the
                if (__netif_subqueue_stopped(local->mdev, i))
                        continue;

check in ieee80211_tx_pending to read
                if (!__netif_subqueue_stopped(local->mdev, i))
                        continue;

as I suggested yesterday?

> I actually can do. I can leave extra 16 TFDs (4 bits for frag num)  in
> the queue that wouldn't be a problem, I'm just not sure that iwlwifi
> is the only driers that does it.  I didn't encountered such scenario
> but this code looks like also non fragments can be put to pending. Am
> I wrong?

non-fragments can only be put in there if your driver is buggy, because
if you don't have fragmentation turned on then you should know when your
queue is filling up and be able to stop it before you get another frame.

The problem with fragmentation is that mac80211 creates multiple frames
from a single one, while if you don't have fragmentation that doesn't
happen since once you stop the queue there are no pending packets.

If we require drivers to always keep space for 8 or 9 fragments then we
can remove all this requeuing stuff, the only trouble with that may be
in adm8211 since that has to send frames one by one, but we can fix it
in that driver, it only has a single queue so it's much easier.

johannes

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