Date:   Thu, 12 Mar 2020 20:05:01 +0200
From:   Petr Machata <petrm@...lanox.com>
To:     netdev@...r.kernel.org
Cc:     Petr Machata <petrm@...lanox.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Roman Mashak <mrv@...atatu.com>,
        Eric Dumazet <eric.dumazet@...il.com>, jhs@...atatu.com,
        xiyou.wangcong@...il.com, davem@...emloft.net, idosch@...lanox.com,
        mlxsw@...lanox.com
Subject: [PATCH net-next v3 0/6] RED: Introduce an ECN nodrop mode

When the RED qdisc is currently configured to enable ECN, the RED algorithm
is used to decide whether a certain SKB should be marked. If that SKB is
not ECN-capable, it is early-dropped.

It is also possible to keep all traffic in the queue, and just mark the
ECN-capable subset of it, as appropriate under the RED algorithm. Some
switches support this mode, and some installations make use of it.
There is currently no way to put the RED qdiscs to this mode.

Therefore this patchset adds a new RED flag, TC_RED_NODROP. When the qdisc
is configured with this flag, non-ECT traffic is enqueued instead of being
early-dropped.

Unfortunately, adding a new RED flag is not as simple as it sounds. RED
flags are passed in tc_red_qopt.flags. However RED neglects to validate the
flag field, and just copies it over wholesale to its internal structure,
and later dumps it back.

A broken userspace can therefore configure a RED qdisc with arbitrary
unsupported flags, and later expect to see the flags on qdisc dump. The
current ABI thus allows storage of 5 bits of custom data along with the
qdisc instance. With the new flag in place, storing 1 to this area would
gain the meaning of "nodrop mode", which is a change of behavior and ABI
breakage.

GRED, SFQ and CHOKE qdiscs are in the same situation. (GRED validates VQ
flags, but not the flags for the main table.) E.g. if SFQ ever needs to
support TC_RED_ADAPTATIVE, it needs another way of doing it, and at the
same time it needs to retain the possibility to store 6 bits of
uninterpreted data.

For RED, this problem is resolved in patch #2, which adds a new attribute,
and a way to separate flags from userbits. This can be reused by other
qdiscs. The flag itself and related behavioral changes are added in patch
#3. In patch #4, the new mode is offloaded by mlxsw.

To test the new feature, patch #1 first introduces a TDC testsuite that
covers the existing RED flags. Patch #5 later extends it with nodrop
coverage. Patch #6 contains a forwarding selftest for the offloaded
datapath.

To test the SW datapath, I took the mlxsw selftest and adapted it in mostly
obvious ways. The test is stable enough to verify that RED, ECN and ECN
nodrop actually work. However, I have no confidence in its portability to
other people's machines or mildly different configurations. I therefore do
not find it suitable for upstreaming.

GRED and CHOKE can use the same method as RED if they ever need to support
extra flags. SFQ uses the length of TCA_OPTIONS to dispatch on binary
control structure version, and would therefore need a different approach.

v3:
- Patch #2:
    - Change TCA_RED_FLAGS from NLA_U32 to NLA_BITFIELD32. Change
      RED_SUPPORTED_FLAGS the macro to red_supported_flags the constant
      and use as .validation_data.
    - Set policy's .strict_start_type to TCA_RED_FLAGS
    - red_get_flags(): Don't modify the passed-in flags until the end of
      the function. Return errno instead of bool.
    - Keep red_sched_data.flags as unsigned char.
    - Because bitfield32 allows only a subset of flags to be set, move the
      validation of the resulting configuration in red_change() into the
      critical section. Add a function red_validate_flags() specifically
      for the validation.
    - Remove braces when setting tc_red_qopt.flags in red_dump().
    - Check nla_put()'s return code when dumping TCA_RED_FLAGS.
    - Always dump TCA_RED_FLAGS, even if only old flags are active.
      The BITFIELD32 interface is richer and this way we can communicate
      to the client which flags are actually supported.
- Patch #3:
    - Rename "taildrop" to "nodrop"
    - Make red_use_nodrop() static instead of static inline
- Patch #4:
    - Adjust for the rename from is_taildrop to is_nodrop.
- Patch #5:
    - Rename "taildrop" to "nodrop"
- Patch #6:
    - Rename "taildrop" to "nodrop"

v2:
- Patch #1
    - Require nsPlugin in each RED test
    - Match end-of-line to catch cases of more flags reported than
      requested
- Patch #2:
    - Replaced with another patch.
- Patch #3:
    - Fix red_use_taildrop() condition in red_enqueue switch for
      probabilistic case.
- Patch #5:
    - Require nsPlugin in each RED test
    - Match end-of-line to catch cases of more flags reported than
      requested
    - Add a test for creation of non-ECN taildrop, which should fail

Petr Machata (6):
  selftests: qdiscs: Add TDC test for RED
  net: sched: Allow extending set of supported RED flags
  net: sched: RED: Introduce an ECN nodrop mode
  mlxsw: spectrum_qdisc: Offload RED ECN nodrop mode
  selftests: qdiscs: RED: Add nodrop tests
  selftests: mlxsw: RED: Test RED ECN nodrop offload

 .../ethernet/mellanox/mlxsw/spectrum_qdisc.c  |   9 +-
 include/net/pkt_cls.h                         |   1 +
 include/net/red.h                             |  38 ++++
 include/uapi/linux/pkt_sched.h                |  17 ++
 net/sched/sch_red.c                           |  74 ++++++-
 .../drivers/net/mlxsw/sch_red_core.sh         |  50 ++++-
 .../drivers/net/mlxsw/sch_red_ets.sh          |  11 ++
 .../drivers/net/mlxsw/sch_red_root.sh         |   8 +
 .../tc-testing/tc-tests/qdiscs/red.json       | 185 ++++++++++++++++++
 9 files changed, 372 insertions(+), 21 deletions(-)
 create mode 100644 tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json

-- 
2.20.1

Powered by blists - more mailing lists