[Ecn-sane] Fwd: [RFC PATCH 00/28]: Accurate ECN for TCP

[Ecn-sane] Fwd: [RFC PATCH 00/28]: Accurate ECN for TCP

[Dave Taht]

I have no comments to make, aside from my concerns with dealing
correctly with offload engines.

---------- Forwarded message ---------
From: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Date: Wed, Mar 18, 2020 at 2:45 AM
Subject: [RFC PATCH 00/28]: Accurate ECN for TCP
To: <netdev@vger.kernel.org>
Cc: Yuchung Cheng <ycheng@google.com>, Neal Cardwell
<ncardwell@google.com>, Eric Dumazet <eric.dumazet@gmail.com>, Olivier
Tilmans <olivier.tilmans@nokia-bell-labs.com>


Hi all,

Here's the full Accurate ECN implementation mostly based on
  https://tools.ietf.org/html/draft-ietf-tcpm-accurate-ecn-11

Comments would be highly appreciated. The GSO/TSO maze of bits
in particular is something I'm somewhat unsure if I got it
right (for a feature that has a software fallback).

There is an extensive set of packetdrill unit tests for most of
the functionality (I'll send separately to packetdrill).

Please note that this submission is not yet intented to be
included to net-next because some small changes seem still
possible to the spec.

 Documentation/networking/ip-sysctl.txt |  12 +-
 drivers/net/tun.c                      |   3 +-
 include/linux/netdev_features.h        |   3 +
 include/linux/skbuff.h                 |   2 +
 include/linux/tcp.h                    |  19 ++
 include/net/tcp.h                      | 221 ++++++++++---
 include/uapi/linux/tcp.h               |   9 +-
 net/ethtool/common.c                   |   1 +
 net/ipv4/bpf_tcp_ca.c                  |   2 +-
 net/ipv4/syncookies.c                  |  12 +
 net/ipv4/tcp.c                         |  10 +-
 net/ipv4/tcp_dctcp.c                   |   2 +-
 net/ipv4/tcp_dctcp.h                   |   2 +-
 net/ipv4/tcp_input.c                   | 558 ++++++++++++++++++++++++++++-----
 net/ipv4/tcp_ipv4.c                    |   8 +-
 net/ipv4/tcp_minisocks.c               |  84 ++++-
 net/ipv4/tcp_offload.c                 |  11 +-
 net/ipv4/tcp_output.c                  | 298 +++++++++++++++---
 net/ipv4/tcp_timer.c                   |   4 +-
 net/ipv6/syncookies.c                  |   1 +
 net/ipv6/tcp_ipv6.c                    |   4 +-
 net/netfilter/nf_log_common.c          |   4 +-

--
 i.

ps. My apologies if you got a duplicate copy of them. It seems that
answering "no" to git send-email asking "Send this email?" might
still have sent something out.



-- 
Make Music, Not War

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-435-0729

[Ecn-sane] Fwd: [RFC PATCH 09/28] gso: AccECN support

[Dave Taht]

---------- Forwarded message ---------
From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
Date: Thu, Mar 19, 2020 at 3:49 PM
Subject: Re: [RFC PATCH 09/28] gso: AccECN support
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Netdev <netdev@vger.kernel.org>, Yuchung Cheng
<ycheng@google.com>, Neal Cardwell <ncardwell@google.com>, Olivier
Tilmans <olivier.tilmans@nokia-bell-labs.com>


On Wed, 18 Mar 2020, Eric Dumazet wrote:
> On 3/18/20 2:43 AM, Ilpo Järvinen wrote:
> > From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> >
> > Handling the CWR flag differs between RFC 3168 ECN and AccECN.
> > Take it into account in GSO by not clearing the CWR bit.
> >
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> >
>
> Does it means TCP segmentation offload is disabled on all the NIC
> around ?

On general level, yes. HW TSO should be disabled with AccECN (or when CWR
flag is set for an incoming packet). The reason is how CWR flag is handled
by RFC 3168 ECN-aware TSO. It splits the segment such that CWR is cleared
starting from the 2nd segment which is incompatible how AccECN handles the
CWR flag. With AccECN, CWR flag (or more accurately, the ACE field that
also includes ECE & AE flags) changes only when new packet(s) with CE mark
arrives so the flag should not be changed within a super-skb. The new
feature flag is necessary to prevent such TSO engines happily clearing
CWRs (if the CWR handling feature cannot be turned off).

If NIC is completely unaware of RFC3168 ECN (doesn't support
NETIF_F_TSO_ECN) or its TSO engine can be set to not touch CWR flag
despite supporting also NETIF_F_TSO_ECN, TSO could be safely used with
AccECN on such NIC. I've little expertise on TSO HW so I don't know if
some/what NICs can do it. Nonetheless, there's nothing fundamental
preventing TSO being enabled with AccECN by NICs (if either of those
conditions is met).

In the cases, where TSO would fail to keep its hands off CWR flag, it
should fallback to GSO which has always on AccECN support (similar to
always on ECN support). I think that the current GSO changes are capable
of handling AccECN skbs. For the other parts of the idea above I'm not
so sure. There is this NETIF_F_GSO_SOFTWARE with comment that seems to
indicate it's doing what I want but I'm not fully sure if adding a flag
there is enough to achieve the desired effect?

On the rx side, supporting both RFC3168 and AccECN style CWR handling
is slightly more complicated (and possibly not worth the effort given
CWRs should be relatively rare with RFC3168-style ECN).

> Why tun driver is changed and not others ?

I think I didn't really understand why tun.c plays with the TSO_ECN flag
until now (after finding a related comment from tap.c) and so that change
now doesn't make much sense for me now any more. So I'll just remove that
part.

> I believe you need to give much more details in changelog in general,
> because many changes are not that obvious.

I'll try to.

Thanks.

--
 i.


-- 
Make Music, Not War

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-435-0729

[Ecn-sane] Fwd: [RFC PATCH 28/28] tcp: AccECN sysctl documentation

[Dave Taht]

On Fri, Mar 20, 2020 at 3:40 PM Ilpo Järvinen
<ilpo.jarvinen@cs.helsinki.fi> wrote:
>
> On Thu, 19 Mar 2020, Dave Taht wrote:
>
> > On Wed, Mar 18, 2020 at 2:44 AM Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
> > >
> > > From: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> > >
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> > > ---
> > >  Documentation/networking/ip-sysctl.txt | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
> > > index 5f53faff4e25..ecca6e1d6bea 100644
> > > --- a/Documentation/networking/ip-sysctl.txt
> > > +++ b/Documentation/networking/ip-sysctl.txt
> > > @@ -301,15 +301,21 @@ tcp_ecn - INTEGER
> > >                 0 Disable ECN.  Neither initiate nor accept ECN.
> > >                 1 Enable ECN when requested by incoming connections and
> > >                   also request ECN on outgoing connection attempts.
> > > -               2 Enable ECN when requested by incoming connections
> > > +               2 Enable ECN or AccECN when requested by incoming connections
> > >                   but do not request ECN on outgoing connections.
> >
> > Changing existing user-behavior for this default seems to be overly
> > optimistic. Useful for testing, but...
>
> I disagree.

If you would like me to make a more specific objection, from what I read
elsewhere in this patchset, an accecn enabled transaction can also arbitrarily



>
> The kernel default on ECN is/has been "do nothing" like forever. Yet,
> passively allowing ECN on servers is a low risk operation because nothing
> will change before client actively asks for it. However, it was obvious
> that the servers didn't do that. The servers could have set tcp_ecn to 1
> (before 2 was there) which is low risk for _servers_ (unlike for clients)
> but only very very few did. I don't believe servers would now
> intentionally pick 2 when they clearly didn't pick 1 earlier either.
>
> Adding 2 is/was an attempt to side-step the need for both ends to make
> conscious decision by setting the sysctl (which servers didn't want to
> do). That is, 2 gives decision on what to do into the hands of the client
> side which was the true intent of 2 (in case you don't know, I made that
> change).
>
> Allowing the client side to make the decision alone has proven successful
> approach. We now have significant passive RFC3168 ECN server deployment.
> It is wide-spread enough that Apple found it useful enough for their
> client side, experimented with it and worked to fix the issues where they
> discovered something in the network was incompatible with ECN. I don't
> believe it would have happened without leaving the decision into the hands
> of the clients.
>
> Similarly, passively allowing the client to decide to use AccECN is
> low risk thing. ...As with RFC 3168 ECN, "do nothing" applies also for
> Accurate ECN here unless the client asks for it.
>
> > > +               3 Enable AccECN when requested by incoming connections and
> > > +                 also request AccECN on outgoing connection attempts.
> > > +           0x102 Enable AccECN in optionless mode for incoming connections.
> > > +           0x103 Enable AccECN in optionless mode for incoming and outgoing
> > > +                 connections.
> >
> > In terms of the logic bits here, it might make more sense
> >
> > 0: disable ecn
> > 1: enable std ecn on in or out
> > 2: enable std ecn when requested on in (the default)
> > 3: essentially unused
> > 4: enable accecn when requested on in
> > 5: enable std ecn and accecn on in or out
> > 6: enable accecn and ecn on in but not out
>
> If "full control" is the way to go, I think it should be made using flags
> instead, along these lines:
>
> 1: Enable RFC 3168 ECN in+out
> 2: Enable RFC 3168 ECN in (default on)
> 4: Enable Accurate ECN in (default on)
> 8: Enable Accurate ECN in+out
>
> Note that I intentionally reversed the in and in/out order for 4&8
> (something that couldn't be done with 1&2 to preserve meaning of 1).
>
> I think it's a bit complicated though but if this is what most people
> want, I can of course change it to flags.
>
> > Do we have any data on how often the tcp ns bit is a source of
> > firewalling problems yet?
> >
> > 0x102 strikes me as a bit more magical than required
>
> To me it compares to some fast open cookie things that are similarly using
> higher order bits in flag like manner.
>
> > and I don't know
> > what optionless means in this context.
>
> Do you mean that "optionless" is not a good word to use here? (I'm not
> a native speaker but I can imagine it might sound like "futureless"?)
> I meant that AccECN operates then w/o sending any AccECN option (rx side
> still processes the options if the peer chooses to send them despite not
> getting any back).
>
> Thanks.
>
> --
>  i.



--
Make Music, Not War

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-435-0729