Re: [Ecn-sane] [PATCH net-next] fq_codel: avoid under-utilization with ce_threshold at low link rates

[Sebastian Moeller <moeller0@gmx.de>]

How is that different from setting the threshold to be equivalent to one packets transmission time (or slightly above). I guess for smaller than MTU packets this might make a difference, but which source is going to produce them back to back so that paying attention actually has some benefit?

My gut-feeling is that this just helps to keep a nice feeling because the threshold is nice and small, even though effectively it is not... What am I missing here?

Regards
	Sebastian


> On Oct 29, 2021, at 17:22, Dave Taht <dave.taht@gmail.com> wrote:
> 
> ---------- Forwarded message ---------
> From: Eric Dumazet <edumazet@google.com>
> Date: Fri, Oct 29, 2021 at 7:53 AM
> Subject: Re: [PATCH net-next] fq_codel: avoid under-utilization with
> ce_threshold at low link rates
> To: Neal Cardwell <ncardwell@google.com>
> Cc: Asad Sajjad Ahmed <asadsa@ifi.uio.no>, David S. Miller
> <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, netdev
> <netdev@vger.kernel.org>, Toke Høiland-Jørgensen <toke@redhat.com>,
> Ingemar Johansson S <ingemar.s.johansson@ericsson.com>, Tom Henderson
> <tomh@tomh.org>, Bob Briscoe <research@bobbriscoe.net>, Olga Albisser
> <olga@albisser.org>
> 
> 
> On Fri, Oct 29, 2021 at 6:54 AM Neal Cardwell <ncardwell@google.com> wrote:
>> 
>> On Thu, Oct 28, 2021 at 3:15 PM Asad Sajjad Ahmed <asadsa@ifi.uio.no> wrote:
>>> 
>>> Commit "fq_codel: generalise ce_threshold marking for subset of traffic"
>>> [1] enables ce_threshold to be used in the Internet, not just in data
>>> centres.
>>> 
>>> Because ce_threshold is in time units, it can cause poor utilization at
>>> low link rates when it represents <1 packet.
>>> E.g., if link rate <12Mb/s ce_threshold=1ms is <1500B packet.
>>> 
>>> So, suppress ECN marking unless the backlog is also > 1 MTU.
>>> 
>>> A similar patch to [1] was tested on an earlier kernel, and a similar
>>> one-packet check prevented poor utilization at low link rates [2].
>>> 
>>> [1] commit dfcb63ce1de6 ("fq_codel: generalise ce_threshold marking for subset of traffic")
>>> 
>>> [2] See right hand column of plots at the end of:
>>> https://bobbriscoe.net/projects/latency/dctth_journal_draft20190726.pdf
>>> 
>>> Signed-off-by: Asad Sajjad Ahmed <asadsa@ifi.uio.no>
>>> Signed-off-by: Olga Albisser <olga@albisser.org>
>>> ---
>>> include/net/codel_impl.h | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/include/net/codel_impl.h b/include/net/codel_impl.h
>>> index 137d40d8cbeb..4e3e8473e776 100644
>>> --- a/include/net/codel_impl.h
>>> +++ b/include/net/codel_impl.h
>>> @@ -248,7 +248,8 @@ static struct sk_buff *codel_dequeue(void *ctx,
>>>                                                    vars->rec_inv_sqrt);
>>>        }
>>> end:
>>> -       if (skb && codel_time_after(vars->ldelay, params->ce_threshold)) {
>>> +       if (skb && codel_time_after(vars->ldelay, params->ce_threshold) &&
>>> +           *backlog > params->mtu) {
> 
> I think this idea would apply to codel quite well.  (This helper is
> common to codel and fq_codel)
> 
> But with fq_codel my thoughts are:
> 
> *backlog is the backlog of the qdisc, not the backlog for the flow,
> and it includes the packet currently being removed from the queue.
> 
> Setting ce_threshold to 1ms while the link rate is 12Mbs sounds
> misconfiguration to me.
> 
> Even if this flow has to transmit one tiny packet every minute, it
> will get CE mark
> just because at least one packet from an elephant flow is currently
> being sent to the wire.
> 
> BQL won't prevent that at least one packet is being processed while
> the tiny packet
> is coming into fq_codel qdisc.
> 
> vars->ldelay = now - skb_time_func(skb);
> 
> For tight ce_threshold, vars->ldelay would need to be replaced by
> 
> now - (time of first codel_dequeue() after this skb has been queued).
> This seems a bit hard to implement cheaply.
> 
> 
> 
> 
>>>                bool set_ce = true;
>>> 
>>>                if (params->ce_threshold_mask) {
>>> --
>> 
>> Sounds like a good idea, and looks good to me.
>> 
>> Acked-by: Neal Cardwell <ncardwell@google.com>
>> 
>> Eric, what do you think?
>> 
>> neal
> 
> 
> -- 
> Fixing Starlink's Latencies: https://www.youtube.com/watch?v=c9gLo6Xrwgw
> 
> Dave Täht CEO, TekLibre, LLC
> _______________________________________________
> Ecn-sane mailing list
> Ecn-sane@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/ecn-sane