netdev - Re: [PATCH net-next 2/2] fq_codel: implement L4S style ce_threshold

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Oct 2021 04:42:20 -0700
From:   Dave Taht <dave.taht@...il.com>
To:     Bob Briscoe <ietf@...briscoe.net>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Ingemar Johansson S <ingemar.s.johansson@...csson.com>,
        Tom Henderson <tomh@...h.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 2/2] fq_codel: implement L4S style
 ce_threshold_ect1 marking

Normally I would comment in-line on the patches but was somehow
unsubscribed to netdev as of oct 11. I'm glad more eyeballs
are on this, finally, however.

1) I would prefer this series bake in the l4s and bbrv2 trees a while,
be tested on vms, containers, on cloudy substrates, and home routers.

Note, I just said series. pie, fq_pie, and cake all can use
rfc3168-style ecn. Worse, fq_codel is also used in the wifi stack, but
not as a qdisc.

I think a safer and simpler path forward for existing assumptions and
callers for rfc3168 -style is that the existing INET_ECN_set_ce and
related calls be reworked to exclude ect1.

(as seen here) https://code.woboq.org/linux/linux/include/net/codel_impl.h.html#182

and the existing ce_threshold parameter enabled by default (at some
acceptible threshold) but only on detecting ect_1. That eliminates
the change to the uapi, and more correctly supports both standards.

Not having the ce_threshold param exposed currently in wifi and not
knowing how to make l4s-style signalling (at least vs tcp prague,
maybe not against bbrv2), it seems safest to make ect_1 - > drop reno
style on wifi, presently.

2) I don't know what is supposed to happen with GRO/GSO packets. This
is actually a long standing confusion of mine in the present day
linux stack - if a GRO packet (say an IW10 burst) is marked - do all
the packets get the marking? or just one? Is it driver or hardware
dependent? (software GRO is the bane of my existence)

3) My other long standing confusion is "triggered" by how we do
statistics keeping for packets - ce_mark gets overloaded by this
patch,
and we end up tracking two very different instances of the same idea
in the kernel statistics (much like the confusion along the path).
Given the frequency of marking in the l4s case is much higher than the
rfc3168 case, I'd advocate for a separate, 64 bit statistic.

Please note that in general I find the "packets" stat in the kernel,
given gso/gro, kind of hard to deal with in the first place,
"bytes_marked"
would be a better statistic to track than packets.