From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-22-ewr.dyndns.com (mxout-245-ewr.mailhop.org [216.146.33.245]) by lists.bufferbloat.net (Postfix) with ESMTP id 5C3442E016E for ; Mon, 21 Feb 2011 11:00:42 -0800 (PST) Received: from scan-22-ewr.mailhop.org (scan-22-ewr.local [10.0.141.244]) by mail-22-ewr.dyndns.com (Postfix) with ESMTP id 600CB2E6A7 for ; Mon, 21 Feb 2011 19:00:41 +0000 (UTC) X-Spam-Score: 0.0 () X-Mail-Handler: MailHop by DynDNS X-Originating-IP: 70.61.120.58 Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by mail-22-ewr.dyndns.com (Postfix) with ESMTP id F2D8C3F581 for ; Mon, 21 Feb 2011 19:00:35 +0000 (UTC) Received: from uucp by smtp.tuxdriver.com with local-rmail (Exim 4.63) (envelope-from ) id 1Prazn-0006eM-9U; Mon, 21 Feb 2011 14:00:31 -0500 Received: from linville-8530p.local (localhost.localdomain [127.0.0.1]) by linville-8530p.local (8.14.4/8.14.4) with ESMTP id p1LIqL16013866; Mon, 21 Feb 2011 13:52:22 -0500 Received: (from linville@localhost) by linville-8530p.local (8.14.4/8.14.4/Submit) id p1LIqLf4013864; Mon, 21 Feb 2011 13:52:21 -0500 Date: Mon, 21 Feb 2011 13:52:21 -0500 From: "John W. Linville" To: Nathaniel Smith Subject: Re: [RFC v2] mac80211: implement eBDP algorithm to fight bufferbloat Message-ID: <20110221185221.GE9650@tuxdriver.com> References: <1297907356-3214-1-git-send-email-linville@tuxdriver.com> <1298064074-8108-1-git-send-email-linville@tuxdriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Cc: bloat-devel@lists.bufferbloat.net, johannes@sipsolutions.net, linux-wireless@vger.kernel.org X-BeenThere: bloat-devel@lists.bufferbloat.net X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Developers working on AQM, device drivers, and networking stacks" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Feb 2011 19:00:42 -0000 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 > 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@tuxdriver.com might be all we have. Be ready.