Message-Id: <1215769627.3483.107.camel@johannes.berg>
Date:	Fri, 11 Jul 2008 11:47:06 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	"David S. Miller" <davem@...emloft.net>
Cc:	netdev <netdev@...r.kernel.org>,
	linux-wireless <linux-wireless@...r.kernel.org>
Subject: TXQ real_num_tx_queues comments/questions

Sorry for not replying in a proper thread, I didn't have a copy at hand
and didn't want to bump the old version. Apologies also for the long
mail, I'm staring at the code at the moment and it may not be the most
cohesive I've ever written.

One thing I just noticed that caused me to think about it a bit more is
that you didn't change netif_tx_{start,stop,wake}_all_queues to use
real_num_tx_queues instead of num_tx_queues. However, since those queues
don't actually get used, it doesn't actually matter whether they're
started or not.

Also related to real_num_tx_queues, you mentioned in the email (if that
restriction is kept it should be explained in a comment somewhere I
think) that it must not be changed while the device is operating. While
that restriction makes it obviously safe, mac80211 is already
effectively violating it, although it isn't setting real_num_tx_queues
at all. That makes me question that restriction a bit.

However, since real_num_tx_queues is only used by the TX hashing, it
would be completely safe to change real_num_tx_queues in an RCU-like
fashion if dev_pick_tx() was under the RCU lock in dev_queue_xmit(),
i.e. just a few lines later.

Back to mac80211. There, we are clearly not using real_num_tx_queues at
all, but we are using select_queue which has the same effect. Our
select_queue, however, isn't static, and it can decide to fill a
different number of queues depending on the aggregation states.

The only interesting situation is when we need to remove a queue since
for adding it we can prepare for that before we make select_queue use
it. Removing it is however, I think, still buggy. You removed most of
the queue locking and now just lock the single txq in ieee80211_requeue,
which seems fine, but when you look at ieee80211_ht_agg_queue_remove it
seems still racy:

select_queue uses the queue_pool bit, which is only cleared in
ieee80211_ht_agg_queue_remove right before ieee80211_requeue.
ieee80211_requeue then proceeds to take all packets out of the queue and
for that of course locks the queue.

But, as far as I can tell, it would be possible for another CPU to be in
select_queue at the same time, after having tested the queue_pool bit to
be set, but not having returned yet. Now, this CPU proceeds to enqueue
the packet to the stopped queue and tries to lock the queue lock for
that, having to wait because the first CPU is in ieee80211_requeue
already holding it. mac80211 finishes requeuing and finally assumes
nothing will come in on that queue any more, but the other CPU, now
getting the lock, will enqueue a frame to that queue.

I'm not entirely sure that my analsysis is correct because the mac80211
code is running in a tasklet, but dev_queue_xmit() only disables bhs
after using select_queue, and even that is only local bhs.

To fix it, I think we should move the queue selection under the rcu
lock. Then we can, in mac80211, instead of deferring the requeuing to
the tasklet, defer it to a work struct (i.e. process context) and
synchronize_rcu() between making select_queue no longer choose the queue
and requeuing from it.

Or we can do more complicated things with the queue_pool bits or an
extra spinlock for select_queue.

I like the RCU variant better, as it means we don't need a "central"
lock that is taken for all tx queues, and it also allows other drivers
to actually change real_num_tx_queues in a similar fashion, should that
ever be required.

johannes

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists