* Re: [Ecn-sane] [PATCH net-next] fq_codel: avoid under-utilization with ce_threshold at low link rates
2021-10-29 15:22 ` [Ecn-sane] Fwd: [PATCH net-next] fq_codel: avoid under-utilization with ce_threshold at low link rates Dave Taht
@ 2021-10-29 15:41 ` Sebastian Moeller
0 siblings, 0 replies; 2+ messages in thread
From: Sebastian Moeller @ 2021-10-29 15:41 UTC (permalink / raw)
To: Dave Täht; +Cc: ECN-Sane
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
^ permalink raw reply [flat|nested] 2+ messages in thread