[PATCH net-next 0/6] RED: Introduce an ECN tail-dropping mode

[PATCH net-next 0/6] RED: Introduce an ECN tail-dropping mode

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

Petr says:

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.

To that end, add a new RED flag, TC_RED_TAILDROP. When the Qdisc is
configured with this flag, non-ECT traffic is enqueued (and tail-dropped
when the queue size is exhausted) instead of being early-dropped.

- The patchset starts with adding in patch #1 a TDC testsuite that covers
  the existing RED flags. This test is extended with the new flag later
  in the patchset.

- Patches #2 and #3 add the taildrop support to the RED qdisc itself resp.
  mlxsw.

- Patches #4 and #5 add tests to, respectively, the newly-introduced TDC
  suite, and the mlxsw-specific RED selftests.

To test the qdisc itself (apart from offloading or configuration, which are
covered above), I took the mlxsw selftest and adapted it for SW datapath in
mostly obvious ways. The test is stable enough to verify that RED, ECN and
ECN taildrop 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.

Petr Machata (6):
  selftests: qdiscs: Add TDC test for RED
  net: sched: Add centralized RED flag checking
  net: sched: RED: Introduce an ECN tail-dropping mode
  mlxsw: spectrum_qdisc: Offload RED ECN tail-dropping mode
  selftests: qdiscs: RED: Add taildrop tests
  selftests: mlxsw: RED: Test RED ECN taildrop offload

 .../ethernet/mellanox/mlxsw/spectrum_qdisc.c  |   9 +-
 include/net/pkt_cls.h                         |   1 +
 include/net/red.h                             |  17 +++
 include/uapi/linux/pkt_sched.h                |   1 +
 net/sched/sch_choke.c                         |   5 +
 net/sched/sch_gred.c                          |   7 +-
 net/sched/sch_red.c                           |  35 ++++-
 net/sched/sch_sfq.c                           |  10 +-
 .../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       | 142 ++++++++++++++++++
 12 files changed, 273 insertions(+), 23 deletions(-)
 create mode 100644 tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json

-- 
2.24.1

[PATCH net-next 1/6] selftests: qdiscs: Add TDC test for RED

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

Add a handful of tests for creating RED with different flags.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 .../tc-testing/tc-tests/qdiscs/red.json       | 102 ++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
new file mode 100644
index 000000000000..6c5a4c4e0a45
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
@@ -0,0 +1,102 @@
+[
+    {
+        "id": "8b6e",
+        "name": "RED",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
+    {
+        "id": "342e",
+        "name": "RED adaptive",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red adaptive limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb adaptive",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
+    {
+        "id": "2d4b",
+        "name": "ECN",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red ecn limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb ecn",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
+    {
+        "id": "650f",
+        "name": "ECN adaptive",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red ecn adaptive limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb ecn adaptive",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
+    {
+        "id": "5f15",
+        "name": "ECN harddrop",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red ecn harddrop limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb ecn harddrop",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    }
+]
-- 
2.24.1

[PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

The qdiscs RED, GRED, SFQ and CHOKE use different subsets of the same pool
of global RED flags. Add a common function for all of these to validate
that only supported flags are passed. In later patches this function will
be extended with a check for flag compatibility / meaningfulness.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/red.h     | 12 ++++++++++++
 net/sched/sch_choke.c |  5 +++++
 net/sched/sch_gred.c  |  7 +++----
 net/sched/sch_red.c   |  5 +++++
 net/sched/sch_sfq.c   | 10 ++++++++--
 5 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/include/net/red.h b/include/net/red.h
index 9665582c4687..bb7bac52c365 100644
--- a/include/net/red.h
+++ b/include/net/red.h
@@ -179,6 +179,18 @@ static inline bool red_check_params(u32 qth_min, u32 qth_max, u8 Wlog)
 	return true;
 }
 
+static inline bool red_check_flags(unsigned int flags,
+				   unsigned int supported_flags,
+				   struct netlink_ext_ack *extack)
+{
+	if (flags & ~supported_flags) {
+		NL_SET_ERR_MSG_MOD(extack, "unsupported RED flags specified");
+		return false;
+	}
+
+	return true;
+}
+
 static inline void red_set_parms(struct red_parms *p,
 				 u32 qth_min, u32 qth_max, u8 Wlog, u8 Plog,
 				 u8 Scell_log, u8 *stab, u32 max_P)
diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index a36974e9c601..c0e0c9f1ace3 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -78,6 +78,8 @@ static unsigned int choke_len(const struct choke_sched_data *q)
 	return (q->tail - q->head) & q->tab_mask;
 }
 
+#define CHOKE_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP)
+
 /* Is ECN parameter configured */
 static int use_ecn(const struct choke_sched_data *q)
 {
@@ -370,6 +372,9 @@ static int choke_change(struct Qdisc *sch, struct nlattr *opt,
 	if (!red_check_params(ctl->qth_min, ctl->qth_max, ctl->Wlog))
 		return -EINVAL;
 
+	if (!red_check_flags(ctl->flags, CHOKE_SUPPORTED_FLAGS, extack))
+		return -EINVAL;
+
 	if (ctl->limit > CHOKE_MAX_QUEUE)
 		return -EINVAL;
 
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index 8599c6f31b05..5e1cb4b243ce 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -428,6 +428,8 @@ static int gred_change_table_def(struct Qdisc *sch, struct nlattr *dps,
 		NL_SET_ERR_MSG_MOD(extack, "can't set per-Qdisc RED flags when per-virtual queue flags are used");
 		return -EINVAL;
 	}
+	if (!red_check_flags(sopt->flags, GRED_VQ_RED_FLAGS, extack))
+		return -EINVAL;
 
 	sch_tree_lock(sch);
 	table->DPs = sopt->DPs;
@@ -590,11 +592,8 @@ static int gred_vq_validate(struct gred_sched *table, u32 cdp,
 			NL_SET_ERR_MSG_MOD(extack, "can't change per-virtual queue RED flags when per-Qdisc flags are used");
 			return -EINVAL;
 		}
-		if (red_flags & ~GRED_VQ_RED_FLAGS) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "invalid RED flags specified");
+		if (!red_check_flags(red_flags, GRED_VQ_RED_FLAGS, extack))
 			return -EINVAL;
-		}
 	}
 
 	return 0;
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 1695421333e3..f9839d68b811 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -44,6 +44,8 @@ struct red_sched_data {
 	struct Qdisc		*qdisc;
 };
 
+#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE)
+
 static inline int red_use_ecn(struct red_sched_data *q)
 {
 	return q->flags & TC_RED_ECN;
@@ -216,6 +218,9 @@ static int red_change(struct Qdisc *sch, struct nlattr *opt,
 	if (!red_check_params(ctl->qth_min, ctl->qth_max, ctl->Wlog))
 		return -EINVAL;
 
+	if (!red_check_flags(ctl->flags, RED_SUPPORTED_FLAGS, extack))
+		return -EINVAL;
+
 	if (ctl->limit > 0) {
 		child = fifo_create_dflt(sch, &bfifo_qdisc_ops, ctl->limit,
 					 extack);
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index c787d4d46017..28949e0ec075 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -325,6 +325,8 @@ static unsigned int sfq_drop(struct Qdisc *sch, struct sk_buff **to_free)
 	return 0;
 }
 
+#define SFQ_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP)
+
 /* Is ECN parameter configured */
 static int sfq_prob_mark(const struct sfq_sched_data *q)
 {
@@ -620,7 +622,8 @@ static void sfq_perturbation(struct timer_list *t)
 		mod_timer(&q->perturb_timer, jiffies + q->perturb_period);
 }
 
-static int sfq_change(struct Qdisc *sch, struct nlattr *opt)
+static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
+		      struct netlink_ext_ack *extack)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
 	struct tc_sfq_qopt *ctl = nla_data(opt);
@@ -640,6 +643,9 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt)
 	if (ctl_v1 && !red_check_params(ctl_v1->qth_min, ctl_v1->qth_max,
 					ctl_v1->Wlog))
 		return -EINVAL;
+	if (ctl_v1 && !red_check_flags(ctl_v1->flags, SFQ_SUPPORTED_FLAGS,
+				       extack))
+		return -EINVAL;
 	if (ctl_v1 && ctl_v1->qth_min) {
 		p = kmalloc(sizeof(*p), GFP_KERNEL);
 		if (!p)
@@ -750,7 +756,7 @@ static int sfq_init(struct Qdisc *sch, struct nlattr *opt,
 	get_random_bytes(&q->perturbation, sizeof(q->perturbation));
 
 	if (opt) {
-		int err = sfq_change(sch, opt);
+		int err = sfq_change(sch, opt, extack);
 		if (err)
 			return err;
 	}
-- 
2.24.1

[PATCH net-next 3/6] net: sched: RED: Introduce an ECN tail-dropping mode

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

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.

To that end, add a new RED flag, TC_RED_TAILDROP. When the Qdisc is
configured with this flag, non-ECT traffic is enqueued (and tail-dropped
when the queue size is exhausted) instead of being early-dropped.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/pkt_cls.h          |  1 +
 include/net/red.h              |  5 +++++
 include/uapi/linux/pkt_sched.h |  1 +
 net/sched/sch_red.c            | 32 ++++++++++++++++++++++++++------
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 341a66af8d59..9ad369aba678 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -727,6 +727,7 @@ struct tc_red_qopt_offload_params {
 	u32 limit;
 	bool is_ecn;
 	bool is_harddrop;
+	bool is_taildrop;
 	struct gnet_stats_queue *qstats;
 };
 
diff --git a/include/net/red.h b/include/net/red.h
index bb7bac52c365..5f018205e57a 100644
--- a/include/net/red.h
+++ b/include/net/red.h
@@ -188,6 +188,11 @@ static inline bool red_check_flags(unsigned int flags,
 		return false;
 	}
 
+	if ((flags & TC_RED_TAILDROP) && !(flags & TC_RED_ECN)) {
+		NL_SET_ERR_MSG_MOD(extack, "taildrop mode is only meaningful with ECN");
+		return false;
+	}
+
 	return true;
 }
 
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index bbe791b24168..7293085ff157 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -272,6 +272,7 @@ struct tc_red_qopt {
 #define TC_RED_ECN		1
 #define TC_RED_HARDDROP		2
 #define TC_RED_ADAPTATIVE	4
+#define TC_RED_TAILDROP		8
 };
 
 struct tc_red_xstats {
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index f9839d68b811..d72db7643a37 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -44,7 +44,8 @@ struct red_sched_data {
 	struct Qdisc		*qdisc;
 };
 
-#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE)
+#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | \
+			     TC_RED_ADAPTATIVE | TC_RED_TAILDROP)
 
 static inline int red_use_ecn(struct red_sched_data *q)
 {
@@ -56,6 +57,11 @@ static inline int red_use_harddrop(struct red_sched_data *q)
 	return q->flags & TC_RED_HARDDROP;
 }
 
+static inline int red_use_taildrop(struct red_sched_data *q)
+{
+	return q->flags & TC_RED_TAILDROP;
+}
+
 static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		       struct sk_buff **to_free)
 {
@@ -76,23 +82,36 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 	case RED_PROB_MARK:
 		qdisc_qstats_overlimit(sch);
-		if (!red_use_ecn(q) || !INET_ECN_set_ce(skb)) {
+		if (!red_use_ecn(q)) {
 			q->stats.prob_drop++;
 			goto congestion_drop;
 		}
 
-		q->stats.prob_mark++;
+		if (INET_ECN_set_ce(skb)) {
+			q->stats.prob_mark++;
+		} else if (red_use_taildrop(q)) {
+			q->stats.prob_drop++;
+			goto congestion_drop;
+		}
+
+		/* Non-ECT packet in ECN taildrop mode: queue it. */
 		break;
 
 	case RED_HARD_MARK:
 		qdisc_qstats_overlimit(sch);
-		if (red_use_harddrop(q) || !red_use_ecn(q) ||
-		    !INET_ECN_set_ce(skb)) {
+		if (red_use_harddrop(q) || !red_use_ecn(q)) {
+			q->stats.forced_drop++;
+			goto congestion_drop;
+		}
+
+		if (INET_ECN_set_ce(skb)) {
+			q->stats.forced_mark++;
+		} else if (!red_use_taildrop(q)) {
 			q->stats.forced_drop++;
 			goto congestion_drop;
 		}
 
-		q->stats.forced_mark++;
+		/* Non-ECT packet in ECN taildrop mode: queue it. */
 		break;
 	}
 
@@ -167,6 +186,7 @@ static int red_offload(struct Qdisc *sch, bool enable)
 		opt.set.limit = q->limit;
 		opt.set.is_ecn = red_use_ecn(q);
 		opt.set.is_harddrop = red_use_harddrop(q);
+		opt.set.is_taildrop = red_use_taildrop(q);
 		opt.set.qstats = &sch->qstats;
 	} else {
 		opt.command = TC_RED_DESTROY;
-- 
2.24.1

[PATCH net-next 4/6] mlxsw: spectrum_qdisc: Offload RED ECN tail-dropping mode

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

RED ECN tail-dropping mode means that non-ECT traffic should not be
early-dropped, but enqueued normally instead. In Spectrum systems, this is
achieved by disabling CWTPM.ew (enable WRED) for a given traffic class.

So far CWTPM.ew was unconditionally enabled. Instead disable it when the
RED qdisc is in taildrop mode.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
index b9f429ec0db4..dee89298122c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
@@ -323,7 +323,7 @@ mlxsw_sp_qdisc_get_tc_stats(struct mlxsw_sp_port *mlxsw_sp_port,
 static int
 mlxsw_sp_tclass_congestion_enable(struct mlxsw_sp_port *mlxsw_sp_port,
 				  int tclass_num, u32 min, u32 max,
-				  u32 probability, bool is_ecn)
+				  u32 probability, bool is_wred, bool is_ecn)
 {
 	char cwtpm_cmd[MLXSW_REG_CWTPM_LEN];
 	char cwtp_cmd[MLXSW_REG_CWTP_LEN];
@@ -341,7 +341,7 @@ mlxsw_sp_tclass_congestion_enable(struct mlxsw_sp_port *mlxsw_sp_port,
 		return err;
 
 	mlxsw_reg_cwtpm_pack(cwtpm_cmd, mlxsw_sp_port->local_port, tclass_num,
-			     MLXSW_REG_CWTP_DEFAULT_PROFILE, true, is_ecn);
+			     MLXSW_REG_CWTP_DEFAULT_PROFILE, is_wred, is_ecn);
 
 	return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(cwtpm), cwtpm_cmd);
 }
@@ -445,8 +445,9 @@ mlxsw_sp_qdisc_red_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
 	prob = DIV_ROUND_UP(prob, 1 << 16);
 	min = mlxsw_sp_bytes_cells(mlxsw_sp, p->min);
 	max = mlxsw_sp_bytes_cells(mlxsw_sp, p->max);
-	return mlxsw_sp_tclass_congestion_enable(mlxsw_sp_port, tclass_num, min,
-						 max, prob, p->is_ecn);
+	return mlxsw_sp_tclass_congestion_enable(mlxsw_sp_port, tclass_num,
+						 min, max, prob,
+						 !p->is_taildrop, p->is_ecn);
 }
 
 static void
-- 
2.24.1

[PATCH net-next 5/6] selftests: qdiscs: RED: Add taildrop tests

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

Add tests for the new "taildrop" flag.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 .../tc-testing/tc-tests/qdiscs/red.json       | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
index 6c5a4c4e0a45..00d25db7e5f7 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
@@ -98,5 +98,45 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP link del dev $DUMMY type dummy"
         ]
+    },
+    {
+        "id": "53e8",
+        "name": "ECN taildrop",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red ecn taildrop limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb ecn taildrop",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
+    {
+        "id": "af8e",
+        "name": "ECN harddrop taildrop",
+        "category": [
+            "qdisc",
+            "red"
+        ],
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red ecn harddrop taildrop limit 1M avpkt 1500 min 100K max 300K",
+        "expExitCode": "0",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb ecn harddrop taildrop",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
     }
 ]
-- 
2.24.1

[PATCH net-next 6/6] selftests: mlxsw: RED: Test RED ECN taildrop offload

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

Extend RED testsuite to cover the new "taildropping" mode of RED-ECN. This
test is really similar to ECN test, diverging only in the last step, where
UDP traffic should go to backlog instead of being dropped. Thus extract a
common helper, ecn_test_common(), make do_ecn_test() into a relatively
simple wrapper, and add another one, do_ecn_taildrop_test().

Signed-off-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 .../drivers/net/mlxsw/sch_red_core.sh         | 50 ++++++++++++++++---
 .../drivers/net/mlxsw/sch_red_ets.sh          | 11 ++++
 .../drivers/net/mlxsw/sch_red_root.sh         |  8 +++
 3 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
index 8f833678ac4d..fc7986db3fe0 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
@@ -389,17 +389,14 @@ check_marking()
 	((pct $cond))
 }
 
-do_ecn_test()
+ecn_test_common()
 {
+	local name=$1; shift
 	local vlan=$1; shift
 	local limit=$1; shift
 	local backlog
 	local pct
 
-	# Main stream.
-	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
-			  $h3_mac tos=0x01
-
 	# Build the below-the-limit backlog using UDP. We could use TCP just
 	# fine, but this way we get a proof that UDP is accepted when queue
 	# length is below the limit. The main stream is using TCP, and if the
@@ -409,7 +406,7 @@ do_ecn_test()
 	check_err $? "Could not build the requested backlog"
 	pct=$(check_marking $vlan "== 0")
 	check_err $? "backlog $backlog / $limit Got $pct% marked packets, expected == 0."
-	log_test "TC $((vlan - 10)): ECN backlog < limit"
+	log_test "TC $((vlan - 10)): $name backlog < limit"
 
 	# Now push TCP, because non-TCP traffic would be early-dropped after the
 	# backlog crosses the limit, and we want to make sure that the backlog
@@ -419,7 +416,20 @@ do_ecn_test()
 	check_err $? "Could not build the requested backlog"
 	pct=$(check_marking $vlan ">= 95")
 	check_err $? "backlog $backlog / $limit Got $pct% marked packets, expected >= 95."
-	log_test "TC $((vlan - 10)): ECN backlog > limit"
+	log_test "TC $((vlan - 10)): $name backlog > limit"
+}
+
+do_ecn_test()
+{
+	local vlan=$1; shift
+	local limit=$1; shift
+	local name=ECN
+
+	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
+			  $h3_mac tos=0x01
+	sleep 1
+
+	ecn_test_common "$name" $vlan $limit
 
 	# Up there we saw that UDP gets accepted when backlog is below the
 	# limit. Now that it is above, it should all get dropped, and backlog
@@ -427,7 +437,31 @@ do_ecn_test()
 	RET=0
 	build_backlog $vlan $((2 * limit)) udp >/dev/null
 	check_fail $? "UDP traffic went into backlog instead of being early-dropped"
-	log_test "TC $((vlan - 10)): ECN backlog > limit: UDP early-dropped"
+	log_test "TC $((vlan - 10)): $name backlog > limit: UDP early-dropped"
+
+	stop_traffic
+	sleep 1
+}
+
+do_ecn_taildrop_test()
+{
+	local vlan=$1; shift
+	local limit=$1; shift
+	local name="ECN taildrop"
+
+	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
+			  $h3_mac tos=0x01
+	sleep 1
+
+	ecn_test_common "$name" $vlan $limit
+
+	# Up there we saw that UDP gets accepted when backlog is below the
+	# limit. Now that it is above, in taildrop mode, make sure it goes to
+	# backlog as well.
+	RET=0
+	build_backlog $vlan $((2 * limit)) udp >/dev/null
+	check_err $? "UDP traffic was early-dropped instead of getting into backlog"
+	log_test "TC $((vlan - 10)): $name backlog > limit: UDP tail-dropped"
 
 	stop_traffic
 	sleep 1
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
index af83efe9ccf1..042a33cc13f4 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
@@ -4,6 +4,7 @@
 ALL_TESTS="
 	ping_ipv4
 	ecn_test
+	ecn_taildrop_test
 	red_test
 	mc_backlog_test
 "
@@ -50,6 +51,16 @@ ecn_test()
 	uninstall_qdisc
 }
 
+ecn_taildrop_test()
+{
+	install_qdisc ecn taildrop
+
+	do_ecn_taildrop_test 10 $BACKLOG1
+	do_ecn_taildrop_test 11 $BACKLOG2
+
+	uninstall_qdisc
+}
+
 red_test()
 {
 	install_qdisc
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
index b2217493a88e..af55672dc335 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
@@ -4,6 +4,7 @@
 ALL_TESTS="
 	ping_ipv4
 	ecn_test
+	ecn_taildrop_test
 	red_test
 	mc_backlog_test
 "
@@ -33,6 +34,13 @@ ecn_test()
 	uninstall_qdisc
 }
 
+ecn_taildrop_test()
+{
+	install_qdisc ecn taildrop
+	do_ecn_taildrop_test 10 $BACKLOG
+	uninstall_qdisc
+}
+
 red_test()
 {
 	install_qdisc
-- 
2.24.1

Re: [PATCH net-next 3/6] net: sched: RED: Introduce an ECN tail-dropping mode

[Jakub Kicinski]

On Mon,  9 Mar 2020 20:35:00 +0200 Ido Schimmel wrote:
> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> index f9839d68b811..d72db7643a37 100644
> --- a/net/sched/sch_red.c
> +++ b/net/sched/sch_red.c
> @@ -44,7 +44,8 @@ struct red_sched_data {
>  	struct Qdisc		*qdisc;
>  };
>  
> -#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE)
> +#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | \
> +			     TC_RED_ADAPTATIVE | TC_RED_TAILDROP)
>  
>  static inline int red_use_ecn(struct red_sched_data *q)
>  {
> @@ -56,6 +57,11 @@ static inline int red_use_harddrop(struct red_sched_data *q)
>  	return q->flags & TC_RED_HARDDROP;
>  }
>  
> +static inline int red_use_taildrop(struct red_sched_data *q)

Please don't do static inlines in C code, even if the neighboring code
does.

> +{
> +	return q->flags & TC_RED_TAILDROP;
> +}
> +
>  static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		       struct sk_buff **to_free)
>  {
> @@ -76,23 +82,36 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  
>  	case RED_PROB_MARK:
>  		qdisc_qstats_overlimit(sch);
> -		if (!red_use_ecn(q) || !INET_ECN_set_ce(skb)) {
> +		if (!red_use_ecn(q)) {
>  			q->stats.prob_drop++;
>  			goto congestion_drop;
>  		}
>  
> -		q->stats.prob_mark++;
> +		if (INET_ECN_set_ce(skb)) {
> +			q->stats.prob_mark++;
> +		} else if (red_use_taildrop(q)) {

This condition is inverted, no?

If user requested taildrop the packet should be queued.

> +			q->stats.prob_drop++;
> +			goto congestion_drop;
> +		}
> +
> +		/* Non-ECT packet in ECN taildrop mode: queue it. */
>  		break;
>  
>  	case RED_HARD_MARK:
>  		qdisc_qstats_overlimit(sch);
> -		if (red_use_harddrop(q) || !red_use_ecn(q) ||
> -		    !INET_ECN_set_ce(skb)) {
> +		if (red_use_harddrop(q) || !red_use_ecn(q)) {
> +			q->stats.forced_drop++;
> +			goto congestion_drop;
> +		}
> +
> +		if (INET_ECN_set_ce(skb)) {
> +			q->stats.forced_mark++;
> +		} else if (!red_use_taildrop(q)) {

This one looks correct.

>  			q->stats.forced_drop++;
>  			goto congestion_drop;
>  		}
>  
> -		q->stats.forced_mark++;
> +		/* Non-ECT packet in ECN taildrop mode: queue it. */
>  		break;
>  	}
>  

Re: [PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Jakub Kicinski]

On Mon,  9 Mar 2020 20:34:59 +0200 Ido Schimmel wrote:
> From: Petr Machata <petrm@mellanox.com>
> 
> The qdiscs RED, GRED, SFQ and CHOKE use different subsets of the same pool
> of global RED flags. Add a common function for all of these to validate
> that only supported flags are passed. In later patches this function will
> be extended with a check for flag compatibility / meaningfulness.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>

The commit message should mention this changes behavior of the kernel,
as the flags weren't validated, so buggy user space may start to see
errors.

The only flags which are validated today are the gRED per-vq ones, which
are a recent addition and were validated from day one.

Re: [PATCH net-next 3/6] net: sched: RED: Introduce an ECN tail-dropping mode

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon,  9 Mar 2020 20:35:00 +0200 Ido Schimmel wrote:
>> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
>> index f9839d68b811..d72db7643a37 100644
>> --- a/net/sched/sch_red.c
>> +++ b/net/sched/sch_red.c
>> @@ -44,7 +44,8 @@ struct red_sched_data {
>>  	struct Qdisc		*qdisc;
>>  };
>>
>> -#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE)
>> +#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | \
>> +			     TC_RED_ADAPTATIVE | TC_RED_TAILDROP)
>>
>>  static inline int red_use_ecn(struct red_sched_data *q)
>>  {
>> @@ -56,6 +57,11 @@ static inline int red_use_harddrop(struct red_sched_data *q)
>>  	return q->flags & TC_RED_HARDDROP;
>>  }
>>
>> +static inline int red_use_taildrop(struct red_sched_data *q)
>
> Please don't do static inlines in C code, even if the neighboring code
> does.

Ok.

>> +{
>> +	return q->flags & TC_RED_TAILDROP;
>> +}
>> +
>>  static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>  		       struct sk_buff **to_free)
>>  {
>> @@ -76,23 +82,36 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>
>>  	case RED_PROB_MARK:
>>  		qdisc_qstats_overlimit(sch);
>> -		if (!red_use_ecn(q) || !INET_ECN_set_ce(skb)) {
>> +		if (!red_use_ecn(q)) {
>>  			q->stats.prob_drop++;
>>  			goto congestion_drop;
>>  		}
>>
>> -		q->stats.prob_mark++;
>> +		if (INET_ECN_set_ce(skb)) {
>> +			q->stats.prob_mark++;
>> +		} else if (red_use_taildrop(q)) {
>
> This condition is inverted, no?
>
> If user requested taildrop the packet should be queued.

Argh, yes! I was meddling with different ways of coding the if-else
after Ido reviewed it and screwed it up. Thanks for catching it.

My tests have not caught this, because they all hit the hard mark case
:(

>> +			q->stats.prob_drop++;
>> +			goto congestion_drop;
>> +		}
>> +
>> +		/* Non-ECT packet in ECN taildrop mode: queue it. */
>>  		break;
>>
>>  	case RED_HARD_MARK:
>>  		qdisc_qstats_overlimit(sch);
>> -		if (red_use_harddrop(q) || !red_use_ecn(q) ||
>> -		    !INET_ECN_set_ce(skb)) {
>> +		if (red_use_harddrop(q) || !red_use_ecn(q)) {
>> +			q->stats.forced_drop++;
>> +			goto congestion_drop;
>> +		}
>> +
>> +		if (INET_ECN_set_ce(skb)) {
>> +			q->stats.forced_mark++;
>> +		} else if (!red_use_taildrop(q)) {
>
> This one looks correct.
>
>>  			q->stats.forced_drop++;
>>  			goto congestion_drop;
>>  		}
>>
>> -		q->stats.forced_mark++;
>> +		/* Non-ECT packet in ECN taildrop mode: queue it. */
>>  		break;
>>  	}
>>

Re: [PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon,  9 Mar 2020 20:34:59 +0200 Ido Schimmel wrote:
>> From: Petr Machata <petrm@mellanox.com>
>> 
>> The qdiscs RED, GRED, SFQ and CHOKE use different subsets of the same pool
>> of global RED flags. Add a common function for all of these to validate
>> that only supported flags are passed. In later patches this function will
>> be extended with a check for flag compatibility / meaningfulness.
>> 
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>
> The commit message should mention this changes behavior of the kernel,
> as the flags weren't validated, so buggy user space may start to see
> errors.

True, I can add that for v2.

> The only flags which are validated today are the gRED per-vq ones, which
> are a recent addition and were validated from day one.

Do you consider the validation as such to be a problem? Because that
would mean that the qdiscs that have not validated flags this way
basically cannot be extended ever ("a buggy userspace used to get a
quiet slicing of flags, and now they mean something").

Re: [PATCH net-next 1/6] selftests: qdiscs: Add TDC test for RED

[Roman Mashak]

Ido Schimmel <idosch@idosch.org> writes:

> From: Petr Machata <petrm@mellanox.com>
>
> Add a handful of tests for creating RED with different flags.
>

Thanks for adding new tests in TDC.

Could you give more descriptive names for tests? (Look at
tc-tests/qdiscs/fifo.json or tc-tests/qdiscs/ets.json as examples)

Did you try running this with nsPlugin enabled? I think you would need
to add the following for every test:

"plugins": {
        "requires": "nsPlugin"
}

> Signed-off-by: Petr Machata <petrm@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  .../tc-testing/tc-tests/qdiscs/red.json       | 102 ++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
>
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
> new file mode 100644
> index 000000000000..6c5a4c4e0a45
> --- /dev/null
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/red.json
> @@ -0,0 +1,102 @@
> +[
> +    {
> +        "id": "8b6e",
> +        "name": "RED",
> +        "category": [
> +            "qdisc",
> +            "red"
> +        ],
> +        "setup": [
> +            "$IP link add dev $DUMMY type dummy || /bin/true"
> +        ],
> +        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root red limit 1M avpkt 1500 min 100K max 300K",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC qdisc show dev $DUMMY",
> +        "matchPattern": "qdisc red 1: root .* limit 1Mb min 100Kb max 300Kb",
> +        "matchCount": "1",
> +        "teardown": [
> +            "$TC qdisc del dev $DUMMY handle 1: root",
> +            "$IP link del dev $DUMMY type dummy"
> +        ]
> +    },

[...]

Re: [PATCH net-next 1/6] selftests: qdiscs: Add TDC test for RED

[Petr Machata]

Roman Mashak <mrv@mojatatu.com> writes:

> Ido Schimmel <idosch@idosch.org> writes:
>
>> From: Petr Machata <petrm@mellanox.com>
>>
>> Add a handful of tests for creating RED with different flags.
>>
>
> Thanks for adding new tests in TDC.
>
> Could you give more descriptive names for tests? (Look at
> tc-tests/qdiscs/fifo.json or tc-tests/qdiscs/ets.json as examples)

Yep.

> Did you try running this with nsPlugin enabled? I think you would need
> to add the following for every test:

Is there a flag that I need to use to enable nsPlugin? I put in the the
"plugins" stanza that you cite below and didn't observe any changes.
That either means that it works or that it has no effect :)

>
> "plugins": {
>         "requires": "nsPlugin"
> }

Re: [PATCH net-next 1/6] selftests: qdiscs: Add TDC test for RED

[Roman Mashak]

Petr Machata <petrm@mellanox.com> writes:

> Roman Mashak <mrv@mojatatu.com> writes:
>
>> Ido Schimmel <idosch@idosch.org> writes:
>>
>>> From: Petr Machata <petrm@mellanox.com>
>>>
>>> Add a handful of tests for creating RED with different flags.
>>>
>>
>> Thanks for adding new tests in TDC.
>>
>> Could you give more descriptive names for tests? (Look at
>> tc-tests/qdiscs/fifo.json or tc-tests/qdiscs/ets.json as examples)
>
> Yep.
>
>> Did you try running this with nsPlugin enabled? I think you would need
>> to add the following for every test:
>
> Is there a flag that I need to use to enable nsPlugin? I put in the the
> "plugins" stanza that you cite below and didn't observe any changes.
> That either means that it works or that it has no effect :)

Look at plugin-lib/README-PLUGINS how to enable the plugin.

Re: [PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Jakub Kicinski]

On Tue, 10 Mar 2020 10:48:24 +0100 Petr Machata wrote:
> > The only flags which are validated today are the gRED per-vq ones, which
> > are a recent addition and were validated from day one.  
> 
> Do you consider the validation as such to be a problem? Because that
> would mean that the qdiscs that have not validated flags this way
> basically cannot be extended ever ("a buggy userspace used to get a
> quiet slicing of flags, and now they mean something").

I just remember leaving it as is when I was working on GRED, because 
of the potential breakage. The uAPI policy is what it is, then again
we probably lose more by making the code of these ancient Qdiscs ugly
than we win :(

I don't feel like I can ack it with clear conscience tho.

Re: [PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 10 Mar 2020 10:48:24 +0100 Petr Machata wrote:
>> > The only flags which are validated today are the gRED per-vq ones, which
>> > are a recent addition and were validated from day one.
>>
>> Do you consider the validation as such to be a problem? Because that
>> would mean that the qdiscs that have not validated flags this way
>> basically cannot be extended ever ("a buggy userspace used to get a
>> quiet slicing of flags, and now they mean something").
>
> I just remember leaving it as is when I was working on GRED, because
> of the potential breakage. The uAPI policy is what it is, then again
> we probably lose more by making the code of these ancient Qdiscs ugly
> than we win :(
>
> I don't feel like I can ack it with clear conscience tho.

Just to make sure -- are you opposed to adding a new flag, or to
validation? At least the adaptative flag was added years after the
others in 2011. I wasn't paying much attention to kernel back then, but
I think the ABI rules are older than that.

Re: [PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Jakub Kicinski]

On Tue, 10 Mar 2020 23:23:23 +0100 Petr Machata wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> 
> > On Tue, 10 Mar 2020 10:48:24 +0100 Petr Machata wrote:  
> >> > The only flags which are validated today are the gRED per-vq ones, which
> >> > are a recent addition and were validated from day one.  
> >>
> >> Do you consider the validation as such to be a problem? Because that
> >> would mean that the qdiscs that have not validated flags this way
> >> basically cannot be extended ever ("a buggy userspace used to get a
> >> quiet slicing of flags, and now they mean something").  
> >
> > I just remember leaving it as is when I was working on GRED, because
> > of the potential breakage. The uAPI policy is what it is, then again
> > we probably lose more by making the code of these ancient Qdiscs ugly
> > than we win :(
> >
> > I don't feel like I can ack it with clear conscience tho.  
> 
> Just to make sure -- are you opposed to adding a new flag, or to
> validation? 

They are both uABI changes, so both.

> At least the adaptative flag was added years after the
> others in 2011. I wasn't paying much attention to kernel back then, but
> I think the ABI rules are older than that.

Yes, but some (e.g. TC subsystem) didn't really care much about those
rules until more recently.

The alternative to validation/adding flag in place is obviously to add 
a new netlink attribute which would be validated from the start. Can you
give it a try and see how ugly it gets?

Re: [PATCH net-next 2/6] net: sched: Add centralized RED flag checking

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 10 Mar 2020 23:23:23 +0100 Petr Machata wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> 
>> > On Tue, 10 Mar 2020 10:48:24 +0100 Petr Machata wrote:  
>> >> > The only flags which are validated today are the gRED per-vq ones, which
>> >> > are a recent addition and were validated from day one.  
>> >>
>> >> Do you consider the validation as such to be a problem? Because that
>> >> would mean that the qdiscs that have not validated flags this way
>> >> basically cannot be extended ever ("a buggy userspace used to get a
>> >> quiet slicing of flags, and now they mean something").  
>> >
>> > I just remember leaving it as is when I was working on GRED, because
>> > of the potential breakage. The uAPI policy is what it is, then again
>> > we probably lose more by making the code of these ancient Qdiscs ugly
>> > than we win :(
>> >
>> > I don't feel like I can ack it with clear conscience tho.  
>> 
>> Just to make sure -- are you opposed to adding a new flag, or to
>> validation? 
>
> They are both uABI changes, so both.
>
>> At least the adaptative flag was added years after the
>> others in 2011. I wasn't paying much attention to kernel back then, but
>> I think the ABI rules are older than that.
>
> Yes, but some (e.g. TC subsystem) didn't really care much about those
> rules until more recently.
>
> The alternative to validation/adding flag in place is obviously to add 
> a new netlink attribute which would be validated from the start. Can you
> give it a try and see how ugly it gets?

Yeah, I'll give it a stab tomorrow.