On Sat, Feb 19, 2011 at 04:37:00PM -0800, Nathaniel Smith wrote:
> Actually, a few more comments just occurred to me...
>
> On Fri, Feb 18, 2011 at 1:21 PM, John W. Linville
> <linville@xxxxxxxxxxxxx> wrote:
> > Johannes' comment about tx status reporting being unreliable (and what
> > he was really saying) finally sunk-in. So, this version uses
> > skb->destructor to track in-flight fragments. That should handle
> > fragments that get silently dropped in the driver for whatever reason
> > without leaking queue capacity. Correct me if I'm wrong!
>
> Should we be somehow filtering out and ignoring the packets that get
> dropped, when we're calculating the average packet transmission rate?
> Presumably they're getting dropped almost instantly, so they don't
> really take up queue space and they have abnormally fast transmission
> times, which will tend to cause us to overestimate max_enqueued? They
> should be rare, though, at least. (And presumably we *should* include
> packets that get dropped because their retry timer ran out, since they
> were sitting in the queue for that long.) Possibly we should just
> ignore any packet that was handled in less than, oh, say, a few
> microseconds?
If you look, I only do the timing measurement for frames that
result in a tx status report. Frames that are dropped will only hit
ieee80211_tx_skb_free (which reduces the enqueued count but doesn't
recalculate max_enqueue).
> Alternatively, if we aren't worried about those packets, then is there
> any reason this patch should be mac80211 specific?
No, in fact I was thinking the same thing. Some thought needs to be
put to whether or not we can ignore the effects of letting dropped
packets effect the latency estimate...
> > +static void ieee80211_tx_skb_free(struct sk_buff *skb)
> > +{
> > + struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(skb->dev);
> > + struct ieee80211_local *local = sdata->local;
> > + int q = skb_get_queue_mapping(skb);
> > +
> > + /* decrement enqueued count */
> > + atomic_dec(&sdata->qdata[q].enqueued);
> > +
> > + /* if queue stopped, wake it */
> > + if (ieee80211_queue_stopped(&local->hw, q))
> > + ieee80211_wake_queue(&local->hw, q);
> > +}
>
> I think you need to check that .enqueued is < max_enqueued here,
> instead of waking the queue unconditionally.
>
> Suppose the data rate drops while there's a steady flow -- our
> max_enqueued value will drop below the current queue size, but because
> we wake the queue unconditionally after each packet goes out, and then
> only stop it again after we've queued at least one new packet, we
> might get 'stuck' with an over-large queue.
Yes, thanks for pointing that out. My non-thorough tests seem to do
a better job at holding the latency lower with that change.
Thanks for taking a look!
John
--
John W. Linville Someday the world will need a hero, and you
linville@xxxxxxxxxxxxx might be all we have. Be ready.
--
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
