[PATCH v2 net-next 0/7] Small tc-taprio improvements

[PATCH v2 net-next 0/7] Small tc-taprio improvements

[Vladimir Oltean]

This series contains:
- the proper protected variant of rcu_dereference() of admin and oper
  schedules for accesses from the slow path
- a removal of an extra function pointer indirection for
  qdisc->dequeue() and qdisc->peek()
- a removal of WARN_ON_ONCE() checks that can never trigger
- the addition of netlink extack messages to some qdisc->init() failures

These were split from an earlier patch set, hence the v2.

Vladimir Oltean (7):
  net/sched: taprio: taprio_offload_config_changed() is protected by
    rtnl_mutex
  net/sched: taprio: taprio_dump and taprio_change are protected by
    rtnl_mutex
  net/sched: taprio: use rtnl_dereference for oper and admin sched in
    taprio_destroy()
  net/sched: taprio: remove redundant FULL_OFFLOAD_IS_ENABLED check in
    taprio_enqueue
  net/sched: taprio: stop going through private ops for dequeue and peek
  net/sched: taprio: add extack messages in taprio_init
  net/sched: taprio: replace safety precautions with comments

 net/sched/sch_taprio.c | 112 +++++++++++++----------------------------
 1 file changed, 34 insertions(+), 78 deletions(-)

-- 
2.34.1

[PATCH v2 net-next 1/7] net/sched: taprio: taprio_offload_config_changed() is protected by rtnl_mutex

[Vladimir Oltean]

The locking in taprio_offload_config_changed() is wrong (but also
inconsequentially so). The current_entry_lock does not serialize changes
to the admin and oper schedules, only to the current entry. In fact, the
rtnl_mutex does that, and that is taken at the time when taprio_change()
is called.

Replace the rcu_dereference_protected() method with the proper RCU
annotation, and drop the unnecessary spin lock.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 net/sched/sch_taprio.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 5bffc37022e0..550afbbae8bc 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1194,16 +1194,10 @@ static void taprio_offload_config_changed(struct taprio_sched *q)
 {
 	struct sched_gate_list *oper, *admin;
 
-	spin_lock(&q->current_entry_lock);
-
-	oper = rcu_dereference_protected(q->oper_sched,
-					 lockdep_is_held(&q->current_entry_lock));
-	admin = rcu_dereference_protected(q->admin_sched,
-					  lockdep_is_held(&q->current_entry_lock));
+	oper = rtnl_dereference(q->oper_sched);
+	admin = rtnl_dereference(q->admin_sched);
 
 	switch_schedules(q, &admin, &oper);
-
-	spin_unlock(&q->current_entry_lock);
 }
 
 static u32 tc_map_to_queue_mask(struct net_device *dev, u32 tc_mask)
-- 
2.34.1

[PATCH v2 net-next 2/7] net/sched: taprio: taprio_dump and taprio_change are protected by rtnl_mutex

[Vladimir Oltean]

Since the writer-side lock is taken here, we do not need to open an RCU
read-side critical section, instead we can use rtnl_dereference() to
tell lockdep we are serialized with concurrent writes.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 net/sched/sch_taprio.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 550afbbae8bc..63cbf856894a 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1486,10 +1486,8 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	}
 	INIT_LIST_HEAD(&new_admin->entries);
 
-	rcu_read_lock();
-	oper = rcu_dereference(q->oper_sched);
-	admin = rcu_dereference(q->admin_sched);
-	rcu_read_unlock();
+	oper = rtnl_dereference(q->oper_sched);
+	admin = rtnl_dereference(q->admin_sched);
 
 	/* no changes - no new mqprio settings */
 	if (!taprio_mqprio_cmp(dev, mqprio))
@@ -1880,9 +1878,8 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct nlattr *nest, *sched_nest;
 	unsigned int i;
 
-	rcu_read_lock();
-	oper = rcu_dereference(q->oper_sched);
-	admin = rcu_dereference(q->admin_sched);
+	oper = rtnl_dereference(q->oper_sched);
+	admin = rtnl_dereference(q->admin_sched);
 
 	opt.num_tc = netdev_get_num_tc(dev);
 	memcpy(opt.prio_tc_map, dev->prio_tc_map, sizeof(opt.prio_tc_map));
@@ -1926,8 +1923,6 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	nla_nest_end(skb, sched_nest);
 
 done:
-	rcu_read_unlock();
-
 	return nla_nest_end(skb, nest);
 
 admin_error:
@@ -1937,7 +1932,6 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	nla_nest_cancel(skb, nest);
 
 start_error:
-	rcu_read_unlock();
 	return -ENOSPC;
 }
 
-- 
2.34.1

[PATCH v2 net-next 3/7] net/sched: taprio: use rtnl_dereference for oper and admin sched in taprio_destroy()

[Vladimir Oltean]

Sparse complains that taprio_destroy() dereferences q->oper_sched and
q->admin_sched without rcu_dereference(), since they are marked as __rcu
in the taprio private structure.

1671:28: warning: incorrect type in argument 1 (different address spaces)
1671:28:    expected struct callback_head *head
1671:28:    got struct callback_head [noderef] __rcu *
1674:28: warning: incorrect type in argument 1 (different address spaces)
1674:28:    expected struct callback_head *head
1674:28:    got struct callback_head [noderef] __rcu *

To silence that build warning, do actually use rtnl_dereference(), since
we know the rtnl_mutex is held at the time of q->destroy().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: resend to net-next, use rtnl_dereference() instead of opening an
        rcu_read_lock()

 net/sched/sch_taprio.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 63cbf856894a..6113c6646559 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1636,6 +1636,7 @@ static void taprio_destroy(struct Qdisc *sch)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
+	struct sched_gate_list *oper, *admin;
 	unsigned int i;
 
 	spin_lock(&taprio_list_lock);
@@ -1659,11 +1660,14 @@ static void taprio_destroy(struct Qdisc *sch)
 
 	netdev_reset_tc(dev);
 
-	if (q->oper_sched)
-		call_rcu(&q->oper_sched->rcu, taprio_free_sched_cb);
+	oper = rtnl_dereference(q->oper_sched);
+	admin = rtnl_dereference(q->admin_sched);
+
+	if (oper)
+		call_rcu(&oper->rcu, taprio_free_sched_cb);
 
-	if (q->admin_sched)
-		call_rcu(&q->admin_sched->rcu, taprio_free_sched_cb);
+	if (admin)
+		call_rcu(&admin->rcu, taprio_free_sched_cb);
 }
 
 static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
-- 
2.34.1

[PATCH v2 net-next 4/7] net/sched: taprio: remove redundant FULL_OFFLOAD_IS_ENABLED check in taprio_enqueue

[Vladimir Oltean]

Since commit 13511704f8d7 ("net: taprio offload: enforce qdisc to netdev
queue mapping"), __dev_queue_xmit() will select a txq->qdisc for the
full offload case of taprio which isn't the root taprio qdisc, so
qdisc enqueues will never pass through taprio_enqueue().

That commit already introduced one safety precaution check for
FULL_OFFLOAD_IS_ENABLED(); a second one is really not needed, so
simplify the conditional for entering into the GSO segmentation logic.
Also reword the comment a little, to appear more natural after the code
change.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 net/sched/sch_taprio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 6113c6646559..0781fc4a2789 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -455,10 +455,10 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 	/* Large packets might not be transmitted when the transmission duration
 	 * exceeds any configured interval. Therefore, segment the skb into
-	 * smaller chunks. Skip it for the full offload case, as the driver
-	 * and/or the hardware is expected to handle this.
+	 * smaller chunks. Drivers with full offload are expected to handle
+	 * this in hardware.
 	 */
-	if (skb_is_gso(skb) && !FULL_OFFLOAD_IS_ENABLED(q->flags)) {
+	if (skb_is_gso(skb)) {
 		unsigned int slen = 0, numsegs = 0, len = qdisc_pkt_len(skb);
 		netdev_features_t features = netif_skb_features(skb);
 		struct sk_buff *segs, *nskb;
-- 
2.34.1

[PATCH v2 net-next 5/7] net/sched: taprio: stop going through private ops for dequeue and peek

[Vladimir Oltean]

Since commit 13511704f8d7 ("net: taprio offload: enforce qdisc to netdev
queue mapping"), taprio_dequeue_soft() and taprio_peek_soft() are de
facto the only implementations for Qdisc_ops :: dequeue and Qdisc_ops ::
peek that taprio provides.

This is because in full offload mode, __dev_queue_xmit() will select a
txq->qdisc which is never root taprio qdisc. So if nothing is enqueued
in the root qdisc, it will never be run and nothing will get dequeued
from it.

Therefore, we can remove the private indirection from taprio, and always
point Qdisc_ops :: dequeue to taprio_dequeue_soft (now simply named
taprio_dequeue) and Qdisc_ops :: peek to taprio_peek_soft (now simply
named taprio_peek).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 net/sched/sch_taprio.c | 58 +++++++++---------------------------------
 1 file changed, 12 insertions(+), 46 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 0781fc4a2789..f3eadea101e1 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -79,8 +79,6 @@ struct taprio_sched {
 	struct sched_gate_list __rcu *admin_sched;
 	struct hrtimer advance_timer;
 	struct list_head taprio_list;
-	struct sk_buff *(*dequeue)(struct Qdisc *sch);
-	struct sk_buff *(*peek)(struct Qdisc *sch);
 	u32 txtime_delay;
 };
 
@@ -492,7 +490,7 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	return taprio_enqueue_one(skb, sch, child, to_free);
 }
 
-static struct sk_buff *taprio_peek_soft(struct Qdisc *sch)
+static struct sk_buff *taprio_peek(struct Qdisc *sch)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
@@ -501,6 +499,11 @@ static struct sk_buff *taprio_peek_soft(struct Qdisc *sch)
 	u32 gate_mask;
 	int i;
 
+	if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
+		WARN_ONCE(1, "Trying to peek into the root of a taprio qdisc configured with full offload\n");
+		return NULL;
+	}
+
 	rcu_read_lock();
 	entry = rcu_dereference(q->current_entry);
 	gate_mask = entry ? entry->gate_mask : TAPRIO_ALL_GATES_OPEN;
@@ -536,20 +539,6 @@ static struct sk_buff *taprio_peek_soft(struct Qdisc *sch)
 	return NULL;
 }
 
-static struct sk_buff *taprio_peek_offload(struct Qdisc *sch)
-{
-	WARN_ONCE(1, "Trying to peek into the root of a taprio qdisc configured with full offload\n");
-
-	return NULL;
-}
-
-static struct sk_buff *taprio_peek(struct Qdisc *sch)
-{
-	struct taprio_sched *q = qdisc_priv(sch);
-
-	return q->peek(sch);
-}
-
 static void taprio_set_budget(struct taprio_sched *q, struct sched_entry *entry)
 {
 	atomic_set(&entry->budget,
@@ -557,7 +546,7 @@ static void taprio_set_budget(struct taprio_sched *q, struct sched_entry *entry)
 			     atomic64_read(&q->picos_per_byte)));
 }
 
-static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch)
+static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
@@ -566,6 +555,11 @@ static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch)
 	u32 gate_mask;
 	int i;
 
+	if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
+		WARN_ONCE(1, "Trying to dequeue from the root of a taprio qdisc configured with full offload\n");
+		return NULL;
+	}
+
 	rcu_read_lock();
 	entry = rcu_dereference(q->current_entry);
 	/* if there's no entry, it means that the schedule didn't
@@ -645,20 +639,6 @@ static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch)
 	return skb;
 }
 
-static struct sk_buff *taprio_dequeue_offload(struct Qdisc *sch)
-{
-	WARN_ONCE(1, "Trying to dequeue from the root of a taprio qdisc configured with full offload\n");
-
-	return NULL;
-}
-
-static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
-{
-	struct taprio_sched *q = qdisc_priv(sch);
-
-	return q->dequeue(sch);
-}
-
 static bool should_restart_cycle(const struct sched_gate_list *oper,
 				 const struct sched_entry *entry)
 {
@@ -1557,17 +1537,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->advance_timer.function = advance_sched;
 	}
 
-	if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
-		q->dequeue = taprio_dequeue_offload;
-		q->peek = taprio_peek_offload;
-	} else {
-		/* Be sure to always keep the function pointers
-		 * in a consistent state.
-		 */
-		q->dequeue = taprio_dequeue_soft;
-		q->peek = taprio_peek_soft;
-	}
-
 	err = taprio_get_start_time(sch, new_admin, &start);
 	if (err < 0) {
 		NL_SET_ERR_MSG(extack, "Internal error: failed get start time");
@@ -1682,9 +1651,6 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
 	hrtimer_init(&q->advance_timer, CLOCK_TAI, HRTIMER_MODE_ABS);
 	q->advance_timer.function = advance_sched;
 
-	q->dequeue = taprio_dequeue_soft;
-	q->peek = taprio_peek_soft;
-
 	q->root = sch;
 
 	/* We only support static clockids. Use an invalid value as default
-- 
2.34.1

[PATCH v2 net-next 6/7] net/sched: taprio: add extack messages in taprio_init

[Vladimir Oltean]

Stop contributing to the proverbial user unfriendliness of tc, and tell
the user what is wrong wherever possible.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 net/sched/sch_taprio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index f3eadea101e1..bc93f709d354 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1663,11 +1663,15 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
 	list_add(&q->taprio_list, &taprio_list);
 	spin_unlock(&taprio_list_lock);
 
-	if (sch->parent != TC_H_ROOT)
+	if (sch->parent != TC_H_ROOT) {
+		NL_SET_ERR_MSG_MOD(extack, "Can only be attached as root qdisc");
 		return -EOPNOTSUPP;
+	}
 
-	if (!netif_is_multiqueue(dev))
+	if (!netif_is_multiqueue(dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Multi-queue device is required");
 		return -EOPNOTSUPP;
+	}
 
 	/* pre-allocate qdisc, attachment can't fail */
 	q->qdiscs = kcalloc(dev->num_tx_queues,
-- 
2.34.1

[PATCH v2 net-next 7/7] net/sched: taprio: replace safety precautions with comments

[Vladimir Oltean]

The WARN_ON_ONCE() checks introduced in commit 13511704f8d7 ("net:
taprio offload: enforce qdisc to netdev queue mapping") take a small
toll on performance, but otherwise, the conditions are never expected to
happen. Replace them with comments, such that the information is still
conveyed to developers.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 net/sched/sch_taprio.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index bc93f709d354..8ae454052201 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -433,6 +433,9 @@ static int taprio_enqueue_one(struct sk_buff *skb, struct Qdisc *sch,
 	return qdisc_enqueue(skb, child, to_free);
 }
 
+/* Will not be called in the full offload case, since the TX queues are
+ * attached to the Qdisc created using qdisc_create_dflt()
+ */
 static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			  struct sk_buff **to_free)
 {
@@ -440,11 +443,6 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct Qdisc *child;
 	int queue;
 
-	if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
-		WARN_ONCE(1, "Trying to enqueue skb into the root of a taprio qdisc configured with full offload\n");
-		return qdisc_drop(skb, sch, to_free);
-	}
-
 	queue = skb_get_queue_mapping(skb);
 
 	child = q->qdiscs[queue];
@@ -490,6 +488,9 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	return taprio_enqueue_one(skb, sch, child, to_free);
 }
 
+/* Will not be called in the full offload case, since the TX queues are
+ * attached to the Qdisc created using qdisc_create_dflt()
+ */
 static struct sk_buff *taprio_peek(struct Qdisc *sch)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
@@ -499,11 +500,6 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch)
 	u32 gate_mask;
 	int i;
 
-	if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
-		WARN_ONCE(1, "Trying to peek into the root of a taprio qdisc configured with full offload\n");
-		return NULL;
-	}
-
 	rcu_read_lock();
 	entry = rcu_dereference(q->current_entry);
 	gate_mask = entry ? entry->gate_mask : TAPRIO_ALL_GATES_OPEN;
@@ -546,6 +542,9 @@ static void taprio_set_budget(struct taprio_sched *q, struct sched_entry *entry)
 			     atomic64_read(&q->picos_per_byte)));
 }
 
+/* Will not be called in the full offload case, since the TX queues are
+ * attached to the Qdisc created using qdisc_create_dflt()
+ */
 static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
@@ -555,11 +554,6 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
 	u32 gate_mask;
 	int i;
 
-	if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
-		WARN_ONCE(1, "Trying to dequeue from the root of a taprio qdisc configured with full offload\n");
-		return NULL;
-	}
-
 	rcu_read_lock();
 	entry = rcu_dereference(q->current_entry);
 	/* if there's no entry, it means that the schedule didn't
-- 
2.34.1

Re: [PATCH v2 net-next 7/7] net/sched: taprio: replace safety precautions with comments

[Jakub Kicinski]

On Thu, 15 Sep 2022 13:50:46 +0300 Vladimir Oltean wrote:
> The WARN_ON_ONCE() checks introduced in commit 13511704f8d7 ("net:
> taprio offload: enforce qdisc to netdev queue mapping") take a small
> toll on performance, but otherwise, the conditions are never expected to
> happen. Replace them with comments, such that the information is still
> conveyed to developers.

Another option is DEBUG_NET_WARN_ON_ONCE() FWIW, you probably know..

Re: [PATCH v2 net-next 0/7] Small tc-taprio improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 15 Sep 2022 13:50:39 +0300 you wrote:
> This series contains:
> - the proper protected variant of rcu_dereference() of admin and oper
>   schedules for accesses from the slow path
> - a removal of an extra function pointer indirection for
>   qdisc->dequeue() and qdisc->peek()
> - a removal of WARN_ON_ONCE() checks that can never trigger
> - the addition of netlink extack messages to some qdisc->init() failures
> 
> [...]

Here is the summary with links:
  - [v2,net-next,1/7] net/sched: taprio: taprio_offload_config_changed() is protected by rtnl_mutex
    https://git.kernel.org/netdev/net-next/c/c8cbe123be6d
  - [v2,net-next,2/7] net/sched: taprio: taprio_dump and taprio_change are protected by rtnl_mutex
    https://git.kernel.org/netdev/net-next/c/18cdd2f0998a
  - [v2,net-next,3/7] net/sched: taprio: use rtnl_dereference for oper and admin sched in taprio_destroy()
    https://git.kernel.org/netdev/net-next/c/9af23657b336
  - [v2,net-next,4/7] net/sched: taprio: remove redundant FULL_OFFLOAD_IS_ENABLED check in taprio_enqueue
    https://git.kernel.org/netdev/net-next/c/fa65edde5e49
  - [v2,net-next,5/7] net/sched: taprio: stop going through private ops for dequeue and peek
    https://git.kernel.org/netdev/net-next/c/25becba6290b
  - [v2,net-next,6/7] net/sched: taprio: add extack messages in taprio_init
    https://git.kernel.org/netdev/net-next/c/026de64d7bc3
  - [v2,net-next,7/7] net/sched: taprio: replace safety precautions with comments
    https://git.kernel.org/netdev/net-next/c/2c08a4f898d0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 net-next 7/7] net/sched: taprio: replace safety precautions with comments

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 02:01:19PM -0700, Jakub Kicinski wrote:
> On Thu, 15 Sep 2022 13:50:46 +0300 Vladimir Oltean wrote:
> > The WARN_ON_ONCE() checks introduced in commit 13511704f8d7 ("net:
> > taprio offload: enforce qdisc to netdev queue mapping") take a small
> > toll on performance, but otherwise, the conditions are never expected to
> > happen. Replace them with comments, such that the information is still
> > conveyed to developers.
> 
> Another option is DEBUG_NET_WARN_ON_ONCE() FWIW, you probably know..

Just for replacing WARN_ON_ONCE(), yes, maybe, but when you factor in
that the code also had calls to qdisc_drop(), I suppose you meant
replacing it with something like this?

	if (DEBUG_NET_WARN_ON_ONCE(unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))))
		return qdisc_drop(skb, sch, to_free);

This won't work because DEBUG_NET_WARN_ON_ONCE() force-casts WARN_ON_ONCE()
to void, discarding its evaluated value.

We'd be left with something custom like below:

	if (IS_ENABLED(CONFIG_DEBUG_NET) && unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
		WARN_ONCE(1, "Trying to enqueue skb into the root of a taprio qdisc configured with full offload\n");
		return qdisc_drop(skb, sch, to_free);
	}

which may work, but it's so odd looking that it's just not worth the
trouble, I feel?

Re: [PATCH v2 net-next 7/7] net/sched: taprio: replace safety precautions with comments

[Jakub Kicinski]

On Wed, 21 Sep 2022 00:16:26 +0000 Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 02:01:19PM -0700, Jakub Kicinski wrote:
> > Another option is DEBUG_NET_WARN_ON_ONCE() FWIW, you probably know..  
> 
> Just for replacing WARN_ON_ONCE(), yes, maybe, but when you factor in
> that the code also had calls to qdisc_drop(), I suppose you meant
> replacing it with something like this?
> 
> 	if (DEBUG_NET_WARN_ON_ONCE(unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))))
> 		return qdisc_drop(skb, sch, to_free);
> 
> This won't work because DEBUG_NET_WARN_ON_ONCE() force-casts WARN_ON_ONCE()
> to void, discarding its evaluated value.
> 
> We'd be left with something custom like below:
> 
> 	if (IS_ENABLED(CONFIG_DEBUG_NET) && unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
> 		WARN_ONCE(1, "Trying to enqueue skb into the root of a taprio qdisc configured with full offload\n");
> 		return qdisc_drop(skb, sch, to_free);
> 	}
> 
> which may work, but it's so odd looking that it's just not worth the
> trouble, I feel?

I meant as a way of retaining the sanity check, a bare:

	DEBUG_NET_WARN_ON_ONCE(FULL_OFFLOAD_IS_ENABLED(q->flags));

no other handling. Not sure how much sense it makes here,
it's best suited as syzbot fodder, perhaps the combination
with offload is pointless.