| Message ID | 20220915105046.2404072-8-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2c08a4f898d0a8e08f431709a1ae728a6fddaabd |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Small tc-taprio improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 60 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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..
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?
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.
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
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(-)