On Wed, Nov 18, 2009 at 12:34:39PM -0800, Johannes Berg wrote:
> On Wed, 2009-11-18 at 12:23 -0800, Luis R. Rodriguez wrote:
>
> > > > > + /* if we got over the limit with this frame, try to make room */
> > > > > + if (num > IEEE80211_TX_STATUS_QUEUE_LIMIT &&
> > > > > + (skb = skb_dequeue(&local->tx_status_unreliable))) {
> > > > > dev_kfree_skb_irq(skb);
> > > >
> > > > Note before there was a while loop and now just a if branch. Why can we
> > > > get away with freeing now just one buffer? It would be nice to see
> > > > this explained in the commit log entry as it is not obvious to me.
> > >
> > > Ah yes, I meant to explain that. We only use the status queues for TX
> > > status now and not for RX too as before, so before it could actually
> > > happen that we could free more than one off the unreliable queue, while
> > > now it really can only be one.
> >
> > Ah, thanks, so, that skb_queue can go and we can just have one
> > unreliable sk_buff ?
>
> No, you misunderstood. Because I'm separating the queue for the tasklet
> (tasklet_queue) and for TX status (tx_status/tx_status_unreliable) we
> can only possibly free one since we only added one -- the count cannot
> increase over that. The loop would only loop once at most.
OK, but can't you still have a driver spam mac80211 with a lot of
ieee80211_tx_status_irqsafe() calls in soft irq context with the final
skb requiring the tx complete, in that case the queue *will* get stuffed
and you could potentially free more if so desired.
Also, if our goal is to just avoid adding the skb if it does not require
a tx complete and our queue size is too large
if (!(info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) &&
num + 1 > IEEE80211_TX_STATUS_QUEUE_LIMIT)
dev_kfree_skb_irq(skb);
else
skb_queue_tail(&local->skb_queue)
> However ... right now we never use _any_ unreliable at all, but I
> suspect we will want to change that again at some point.
Just curious -- what would be a use case for that?
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
