On Mon, Mar 23, 2009 at 05:28:37PM +0100, Johannes Berg wrote:
> The pending packets code is quite incomprehensible, uses memory barriers
> nobody really understands, etc. This patch reworks it entirely, using
> the queue spinlock, proper stop bits and the skb queues themselves to
> indicate whether packets are pending or not (rather than a separate
> variable like before).
>
> Signed-off-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
<-- snip -->
> /*
> @@ -1830,40 +1829,57 @@ void ieee80211_tx_pending(unsigned long
> {
> struct ieee80211_local *local = (struct ieee80211_local *)data;
> struct net_device *dev = local->mdev;
> - struct ieee80211_tx_stored_packet *store;
> struct ieee80211_hdr *hdr;
> + unsigned long flags;
> struct ieee80211_tx_data tx;
> int i, ret;
> + bool next;
>
> rcu_read_lock();
> netif_tx_lock_bh(dev);
> +
> for (i = 0; i < local->hw.queues; i++) {
> - /* Check that this queue is ok */
> - if (__netif_subqueue_stopped(local->mdev, i) &&
> - !test_bit(i, local->queues_pending_run))
> - continue;
> + /*
> + * If queue is stopped by something other than due to pending
> + * frames, or we have no pending frames, proceed to next queue.
> + */
> + spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> + next = false;
> + if (local->queue_stop_reasons[i] !=
> + BIT(IEEE80211_QUEUE_STOP_REASON_PENDING) ||
> + skb_queue_empty(&local->pending[i]))
> + next = true;
> + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>
> - if (!test_bit(i, local->queues_pending)) {
> - clear_bit(i, local->queues_pending_run);
> - ieee80211_wake_queue(&local->hw, i);
> + if (next)
> continue;
> - }
>
> - clear_bit(i, local->queues_pending_run);
> + /*
> + * start the queue now to allow processing our packets,
> + * we're under the tx lock here anyway so nothing will
> + * happen as a result of this
> + */
> netif_start_subqueue(local->mdev, i);
>
> - store = &local->pending_packet[i];
> - tx.flags = 0;
> - tx.skb = store->skb;
> - hdr = (struct ieee80211_hdr *)tx.skb->data;
> - tx.sta = sta_info_get(local, hdr->addr1);
> - ret = __ieee80211_tx(local, &tx);
> - store->skb = tx.skb;
> - if (!ret) {
> - clear_bit(i, local->queues_pending);
> - ieee80211_wake_queue(&local->hw, i);
> + while (!skb_queue_empty(&local->pending[i])) {
> + tx.flags = 0;
> + tx.skb = skb_dequeue(&local->pending[i]);
> + hdr = (struct ieee80211_hdr *)tx.skb->data;
> + tx.sta = sta_info_get(local, hdr->addr1);
> +
> + ret = __ieee80211_tx(local, &tx);
> + if (ret != IEEE80211_TX_OK) {
> + skb_queue_head(&local->pending[i], tx.skb);
> + break;
> + }
> }
So this is good functional change, might be worth mentioning in the
commit log, that is, we now requeue onto the pending queue the skb
when the driver's tx() didn't return NETDEV_TX_OK or when __ieee80211_tx()
returns IEEE80211_TX_PENDING (which happens when the queue is stopped).
Maybe even better as a seperate patch. Before this we were just dropping
the skbs in the pending queue under those same conditions.
Other than that:
Reviewed-by: Luis R. Rodriguez <lrodriguez@xxxxxxxxxxx>
Luis
--
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
