[Ecn-sane] Fwd: [PATCH net-next] tcp: avoid negotitating ECN for BBR

[Ecn-sane] Fwd: [PATCH net-next] tcp: avoid negotitating ECN for BBR

[Dave Taht]

Was this ever resolved?

---------- Forwarded message ---------
From: David Miller <davem@davemloft.net>
Date: Fri, Jan 19, 2018 at 2:33 PM
Subject: Re: [PATCH net-next] tcp: avoid negotitating ECN for BBR
To: <ycheng@google.com>
Cc: <netdev@vger.kernel.org>, <edumazet@google.com>,
<ysseung@google.com>, <ncardwell@google.com>


From: Yuchung Cheng <ycheng@google.com>
Date: Tue, 16 Jan 2018 17:57:26 -0800

> This patch keeps BBR from negotiating ECN if sysctl ECN is
> set. Prior to this patch, BBR negotiates ECN if enabled, sends
> CWR upon receiving ECE ACKs but does not react to them. This can
> cause confusion from the protocol perspective. Therefore this
> patch prevents the connection from negotiating ECN if BBR is the
> congestion control during the handshake.
>
> Note that after the handshake, the user can still switch to a
> different congestion control that supports or even requires ECN
> (e.g. DCTCP).  In that case, the connection can not re-negotiate
> ECN and has to go with the ECN-free mode in that congestion control.
>
> There are other cases BBR would still respond to ECE ACKs with CWR
> but does not react to it like the behavior before this patch. First,
> when the user switches to BBR congestion control but the connection
> has already negotiated ECN before. Second, the system has configured
> the ip route and/or uses eBPF to enable ECN on the connection that
> uses BBR congestion control.
>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Acked-by: Yousuk Seung <ysseung@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>

Well, this is a bit disappointing.  I'm having trouble justifying
applying this.

Why doesn't BBR react to ECN notifications?  Is it because BBR's
idea of congestion differs from the one ECN is likely indicating?

This is really unfortunate, because if BBR does become quite prominent
(that's what you want right? :-) then what little success there has
been deploying working ECN will be for almost nothing, and there
will be little incentive for further ECN deployment.

And the weird behavior you list in your last paragraph, about how if
the user switches to BBR then ECN will be active, is just a red flag
that shows perhaps this is a bad idea overall.

ECN behavior should not be so tightly bound to the congestion control
algorithm like this, it's a connection property independant of
congestion control algorithm.

I'm not applying this for now, sorry.  Maybe if you significantly
enhance the commit message and try to do something sane with the
algorithm switching case it is work a respin.

Thanks.


-- 
:( My old R&D campus is up for sale: https://tinyurl.com/yurtlab
Dave Täht CSO, LibreQos

Re: [Ecn-sane] Fwd: [PATCH net-next] tcp: avoid negotitating ECN for BBR

[Pete Heist]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

It doesn't look like the patch to set TCP_CONG_DONT_USE_ECN in BBRv1
was ever applied, but it also doesn't look like v1 was ever modified to
react to CE, whereas BBRv3 appears to:

https://github.com/google/bbr/blob/v3/README.md#enabling-ecn-support

I'll post something about ecn_low separately.

Pete

On Mon, 2023-11-27 at 19:12 -0500, Dave Taht via Ecn-sane wrote:
> Was this ever resolved?
> 
> ---------- Forwarded message ---------
> From: David Miller <davem@davemloft.net>
> Date: Fri, Jan 19, 2018 at 2:33 PM
> Subject: Re: [PATCH net-next] tcp: avoid negotitating ECN for BBR
> To: <ycheng@google.com>
> Cc: <netdev@vger.kernel.org>, <edumazet@google.com>,
> <ysseung@google.com>, <ncardwell@google.com>
> 
> 
> From: Yuchung Cheng <ycheng@google.com>
> Date: Tue, 16 Jan 2018 17:57:26 -0800
> 
> > This patch keeps BBR from negotiating ECN if sysctl ECN is
> > set. Prior to this patch, BBR negotiates ECN if enabled, sends
> > CWR upon receiving ECE ACKs but does not react to them. This can
> > cause confusion from the protocol perspective. Therefore this
> > patch prevents the connection from negotiating ECN if BBR is the
> > congestion control during the handshake.
> > 
> > Note that after the handshake, the user can still switch to a
> > different congestion control that supports or even requires ECN
> > (e.g. DCTCP).  In that case, the connection can not re-negotiate
> > ECN and has to go with the ECN-free mode in that congestion
> > control.
> > 
> > There are other cases BBR would still respond to ECE ACKs with CWR
> > but does not react to it like the behavior before this patch.
> > First,
> > when the user switches to BBR congestion control but the connection
> > has already negotiated ECN before. Second, the system has
> > configured
> > the ip route and/or uses eBPF to enable ECN on the connection that
> > uses BBR congestion control.
> > 
> > Signed-off-by: Yuchung Cheng <ycheng@google.com>
> > Signed-off-by: Neal Cardwell <ncardwell@google.com>
> > Acked-by: Yousuk Seung <ysseung@google.com>
> > Acked-by: Eric Dumazet <edumazet@google.com>
> 
> Well, this is a bit disappointing.  I'm having trouble justifying
> applying this.
> 
> Why doesn't BBR react to ECN notifications?  Is it because BBR's
> idea of congestion differs from the one ECN is likely indicating?
> 
> This is really unfortunate, because if BBR does become quite
> prominent
> (that's what you want right? :-) then what little success there has
> been deploying working ECN will be for almost nothing, and there
> will be little incentive for further ECN deployment.
> 
> And the weird behavior you list in your last paragraph, about how if
> the user switches to BBR then ECN will be active, is just a red flag
> that shows perhaps this is a bad idea overall.
> 
> ECN behavior should not be so tightly bound to the congestion control
> algorithm like this, it's a connection property independant of
> congestion control algorithm.
> 
> I'm not applying this for now, sorry.  Maybe if you significantly
> enhance the commit message and try to do something sane with the
> algorithm switching case it is work a respin.
> 
> Thanks.
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [Ecn-sane] [PATCH net-next] tcp: avoid negotitating ECN for BBR

[Sebastian Moeller]

Hi Pete,


> On Nov 28, 2023, at 09:43, Pete Heist via Ecn-sane <ecn-sane@lists.bufferbloat.net> wrote:
> 
> It doesn't look like the patch to set TCP_CONG_DONT_USE_ECN in BBRv1
> was ever applied, but it also doesn't look like v1 was ever modified to
> react to CE, whereas BBRv3 appears to:
> 
> https://github.com/google/bbr/blob/v3/README.md#enabling-ecn-support
> 
> I'll post something about ecn_low separately.

	The way I read this is that BBR still does ignore rfc3168 ECN, but allows to use DCTCP style ECN on a route by route base... this at least seems to acknowledge that currently no randomly selected path is likely to support DCTCP style ECN... this seems far more cautious than I had feared. I still think that BBR did itself a disservice to ignore rfc3168 style ECN, but that is water down the bridge...

Regards
	Sebastian



> 
> Pete
> 
> On Mon, 2023-11-27 at 19:12 -0500, Dave Taht via Ecn-sane wrote:
>> Was this ever resolved?
>> 
>> ---------- Forwarded message ---------
>> From: David Miller <davem@davemloft.net>
>> Date: Fri, Jan 19, 2018 at 2:33 PM
>> Subject: Re: [PATCH net-next] tcp: avoid negotitating ECN for BBR
>> To: <ycheng@google.com>
>> Cc: <netdev@vger.kernel.org>, <edumazet@google.com>,
>> <ysseung@google.com>, <ncardwell@google.com>
>> 
>> 
>> From: Yuchung Cheng <ycheng@google.com>
>> Date: Tue, 16 Jan 2018 17:57:26 -0800
>> 
>>> This patch keeps BBR from negotiating ECN if sysctl ECN is
>>> set. Prior to this patch, BBR negotiates ECN if enabled, sends
>>> CWR upon receiving ECE ACKs but does not react to them. This can
>>> cause confusion from the protocol perspective. Therefore this
>>> patch prevents the connection from negotiating ECN if BBR is the
>>> congestion control during the handshake.
>>> 
>>> Note that after the handshake, the user can still switch to a
>>> different congestion control that supports or even requires ECN
>>> (e.g. DCTCP).  In that case, the connection can not re-negotiate
>>> ECN and has to go with the ECN-free mode in that congestion
>>> control.
>>> 
>>> There are other cases BBR would still respond to ECE ACKs with CWR
>>> but does not react to it like the behavior before this patch.
>>> First,
>>> when the user switches to BBR congestion control but the connection
>>> has already negotiated ECN before. Second, the system has
>>> configured
>>> the ip route and/or uses eBPF to enable ECN on the connection that
>>> uses BBR congestion control.
>>> 
>>> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>>> Signed-off-by: Neal Cardwell <ncardwell@google.com>
>>> Acked-by: Yousuk Seung <ysseung@google.com>
>>> Acked-by: Eric Dumazet <edumazet@google.com>
>> 
>> Well, this is a bit disappointing.  I'm having trouble justifying
>> applying this.
>> 
>> Why doesn't BBR react to ECN notifications?  Is it because BBR's
>> idea of congestion differs from the one ECN is likely indicating?
>> 
>> This is really unfortunate, because if BBR does become quite
>> prominent
>> (that's what you want right? :-) then what little success there has
>> been deploying working ECN will be for almost nothing, and there
>> will be little incentive for further ECN deployment.
>> 
>> And the weird behavior you list in your last paragraph, about how if
>> the user switches to BBR then ECN will be active, is just a red flag
>> that shows perhaps this is a bad idea overall.
>> 
>> ECN behavior should not be so tightly bound to the congestion control
>> algorithm like this, it's a connection property independant of
>> congestion control algorithm.
>> 
>> I'm not applying this for now, sorry.  Maybe if you significantly
>> enhance the commit message and try to do something sane with the
>> algorithm switching case it is work a respin.
>> 
>> Thanks.
>> 
>> 
> 
> _______________________________________________
> Ecn-sane mailing list
> Ecn-sane@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/ecn-sane