From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ob0-x22b.google.com (mail-ob0-x22b.google.com [IPv6:2607:f8b0:4003:c01::22b]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by huchra.bufferbloat.net (Postfix) with ESMTPS id CA70E21F663 for ; Fri, 27 Nov 2015 07:16:07 -0800 (PST) Received: by obdgf3 with SMTP id gf3so85409431obd.3 for ; Fri, 27 Nov 2015 07:16:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=6Npn6pPbQDiRoHNm2GN6kqs9zy2d4dx8sq6lnNqMPqo=; b=J1esBwYGFJp98IjqzEwCuermovLl+h5EOBk2QEvMMoGzYYz1WSdb+J+gwCWwd4ruHD fq22CAEu/oNNlpWOWUp23ZffXNLwcachpU6oCLeynvk4Nauagc16zNkxNQ9JUsnpVsnK pm1Mf9ZmWiW0u2rBI9d0yLXYpOuMFZIev34pL3PF/UfmlcQJXMY4dYMsbilD/0dWifkX 9oNNqFSgkcJ9FXzJWCJ+vtpAfASLW6L/G3r6WZmrbfQNWy0sYqb5c0TbOPks6lRqpnKS acdu+e/NmRo7n+HkgsoUHXRyv3hExtp7l1Mgi4PJ+K7ParVooSPbYxtYr+WE0Eo0aja3 vugw== MIME-Version: 1.0 X-Received: by 10.60.39.200 with SMTP id r8mr19678312oek.81.1448637365916; Fri, 27 Nov 2015 07:16:05 -0800 (PST) Received: by 10.202.187.3 with HTTP; Fri, 27 Nov 2015 07:16:05 -0800 (PST) Date: Fri, 27 Nov 2015 16:16:05 +0100 Message-ID: From: Dave Taht To: Kevin Darbyshire-Bryant Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: cake@lists.bufferbloat.net Subject: [Cake] NET_XMIT_CN question X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.13 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Nov 2015 15:16:30 -0000 We decreased the size of the queue, regardless of what flow we dropped from... so why do we not need to decrease the queue size when using NET_XMIT_CN? Where is that done by the caller? I have grepped and googled... As best as I recall the semantics of NET_XMIT_CN changed over time, it used= to only work on enqueue on a tail drop queue, where it would push back on the stack to not, actually, drop the packet, but to halve the window and requeue. Staring at the code now, I don't see how it could be right... here OR in the existing fq_codel code in the mainline. /* Return Congestion Notification only if we dropped a packet * from this flow. */ if (fq_codel_drop(sch) =3D=3D idx) return NET_XMIT_CN; /* As we dropped a packet, better let upper stack know this */ qdisc_tree_decrease_qlen(sch, 1); return NET_XMIT_SUCCESS; (I am not sure NET_XMIT_CN works right when there is a hash collision, for example, although the updated doc does seem to indicate that, ... still, then, where does (an equivalent for) qdisc_tree_decrease_qlen kick in?) So I ripped it out. Otherwise... the code does look correct, now that I've reviewed it further. But does putting it back in have any measurable effect? ... Also I note there was a bunch of commits to sch_fq_codel mainline since jul= y, notably this one. c0afd9ce4d6a646fb6433536f95a418bb348fab1 Dave T=C3=A4ht Let's go make home routers and wifi faster! With better software! https://www.gofundme.com/savewifi On Fri, Nov 27, 2015 at 3:33 PM, Kevin Darbyshire-Bryant wrote: > I've been pondering this commit and wonder if we're throwing out baby > with bathwater: > > https://github.com/dtaht/sch_cake/commit/90e792115f10fa2a31327208302e66c0= ab3f2251 > > The original code, to paraphrase in something resembling English went: > > 1. I've just enqueud a packet & Oh sh*t we're over our q_disc buffer > limit, we'd better do something about it, hmmm > 2. Whilst we're over this limit, go look through our flows (cake_drop) > and kill something, if that something was in the same flow as the thing > we just enqueued then set a flag 'cos these guys are part of the > problem! Keep track of how many things we killed. > 3 Now we're below the limit and finished killing packets, return back to > our caller - if we killed stuff in the same flow the caller is sending, > then return 'NET_XMIT_CN' (we're congested), else just say 'happy as Larr= y' > > *//* NET_XMIT_CN is special. It does not guarantee that this packet is > lost. It/* > */* indicates that the device will soon be dropping packets, or already > drops/* > */* some packets of the same priority; prompting us to send less > aggressively. *//* > > What I do agree looked odd in the original code was the 'dropped - > same_flow': We haven't dropped any more or fewer packets just 'cos we > found one or more in 'the same flow'. > > - qdisc_tree_decrease_qlen(sch, dropped - same_flow); <--???????? > + qdisc_tree_decrease_qlen(sch, dropped); > > Incidentally, a search of 'NET_XMIT_CN' turned up > http://www.bufferbloat.net/issues/398.pdf > > > _______________________________________________ > Cake mailing list > Cake@lists.bufferbloat.net > https://lists.bufferbloat.net/listinfo/cake >