[PATCH net-next v3 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[PATCH net-next v3 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Victor Nogueira]

This patch builds on Daniel's patch[1] to add initial support of tc drop
reason. The main goal is to distinguish between policy and error drops for
the remainder of the egress qdiscs (other than clsact).
The drop reason is set by cls_api and act_api in the tc skb cb in case
any error occurred in the data path.

Also add new skb drop reasons that are idiosyncratic to TC.

[1] https://lore.kernel.org/all/20231009092655.22025-1-daniel@iogearbox.net

Changes in V3:
- Removed duplicate assignment
- Rename function tc_skb_cb_drop_reason to tcf_get_drop_reason
- Move zone field upwards in struct tc_skb_cb to move hole to the end of 
  the struct

Changes in V2:
- Dropped RFC tag
- Removed check for drop reason being overwritten by filter in cls_api.c
- Simplified logic and removed function tcf_init_drop_reason

Victor Nogueira (3):
  net: sched: Move drop_reason to struct tc_skb_cb
  net: sched: Make tc-related drop reason more flexible for remaining
    qdiscs
  net: sched: Add initial TC error skb drop reasons

 include/net/dropreason-core.h | 30 +++++++++++++++++++++++++++---
 include/net/pkt_cls.h         |  6 ------
 include/net/pkt_sched.h       | 18 ------------------
 include/net/sch_generic.h     | 32 +++++++++++++++++++++++++++++++-
 net/core/dev.c                | 11 +++++++----
 net/sched/act_api.c           |  3 ++-
 net/sched/cls_api.c           | 31 +++++++++++++++----------------
 7 files changed, 82 insertions(+), 49 deletions(-)

-- 
2.25.1

[PATCH net-next v3 1/3] net: sched: Move drop_reason to struct tc_skb_cb

[Victor Nogueira]

Move drop_reason from struct tcf_result to skb cb - more specifically to
struct tc_skb_cb. With that, we'll be able to also set the drop reason for
the remaining qdiscs (aside from clsact) that do not have access to
tcf_result when time comes to set the skb drop reason.

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/pkt_cls.h     | 14 ++++++++++++--
 include/net/pkt_sched.h   |  3 ++-
 include/net/sch_generic.h |  1 -
 net/core/dev.c            |  4 ++--
 net/sched/act_api.c       |  2 +-
 net/sched/cls_api.c       | 23 ++++++++---------------
 6 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a76c9171db0e..761e4500cca0 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -154,10 +154,20 @@ __cls_set_class(unsigned long *clp, unsigned long cl)
 	return xchg(clp, cl);
 }
 
-static inline void tcf_set_drop_reason(struct tcf_result *res,
+struct tc_skb_cb;
+
+static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb);
+
+static inline enum skb_drop_reason
+tcf_get_drop_reason(const struct sk_buff *skb)
+{
+	return tc_skb_cb(skb)->drop_reason;
+}
+
+static inline void tcf_set_drop_reason(const struct sk_buff *skb,
 				       enum skb_drop_reason reason)
 {
-	res->drop_reason = reason;
+	tc_skb_cb(skb)->drop_reason = reason;
 }
 
 static inline void
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 9fa1d0794dfa..9b559aa5c079 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -277,12 +277,13 @@ static inline void skb_txtime_consumed(struct sk_buff *skb)
 
 struct tc_skb_cb {
 	struct qdisc_skb_cb qdisc_cb;
+	u32 drop_reason;
 
+	u16 zone; /* Only valid if post_ct = true */
 	u16 mru;
 	u8 post_ct:1;
 	u8 post_ct_snat:1;
 	u8 post_ct_dnat:1;
-	u16 zone; /* Only valid if post_ct = true */
 };
 
 static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index dcb9160e6467..c499b56bb215 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -332,7 +332,6 @@ struct tcf_result {
 		};
 		const struct tcf_proto *goto_tp;
 	};
-	enum skb_drop_reason		drop_reason;
 };
 
 struct tcf_chain;
diff --git a/net/core/dev.c b/net/core/dev.c
index 1d720fc59161..4b84b72ebae8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3923,14 +3923,14 @@ static int tc_run(struct tcx_entry *entry, struct sk_buff *skb,
 
 	tc_skb_cb(skb)->mru = 0;
 	tc_skb_cb(skb)->post_ct = false;
-	res.drop_reason = *drop_reason;
+	tcf_set_drop_reason(skb, *drop_reason);
 
 	mini_qdisc_bstats_cpu_update(miniq, skb);
 	ret = tcf_classify(skb, miniq->block, miniq->filter_list, &res, false);
 	/* Only tcf related quirks below. */
 	switch (ret) {
 	case TC_ACT_SHOT:
-		*drop_reason = res.drop_reason;
+		*drop_reason = tcf_get_drop_reason(skb);
 		mini_qdisc_qstats_cpu_drop(miniq);
 		break;
 	case TC_ACT_OK:
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index abec5c45b5a4..f2b136ce9282 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1098,7 +1098,7 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 			}
 		} else if (TC_ACT_EXT_CMP(ret, TC_ACT_GOTO_CHAIN)) {
 			if (unlikely(!rcu_access_pointer(a->goto_chain))) {
-				tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 				return TC_ACT_SHOT;
 			}
 			tcf_action_goto_chain_exec(a, res);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1976bd163986..32457a236d77 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1658,7 +1658,6 @@ static inline int __tcf_classify(struct sk_buff *skb,
 				 int act_index,
 				 u32 *last_executed_chain)
 {
-	u32 orig_reason = res->drop_reason;
 #ifdef CONFIG_NET_CLS_ACT
 	const int max_reclassify_loop = 16;
 	const struct tcf_proto *first_tp;
@@ -1683,13 +1682,13 @@ static inline int __tcf_classify(struct sk_buff *skb,
 			 */
 			if (unlikely(n->tp != tp || n->tp->chain != n->chain ||
 				     !tp->ops->get_exts)) {
-				tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 				return TC_ACT_SHOT;
 			}
 
 			exts = tp->ops->get_exts(tp, n->handle);
 			if (unlikely(!exts || n->exts != exts)) {
-				tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 				return TC_ACT_SHOT;
 			}
 
@@ -1713,18 +1712,12 @@ static inline int __tcf_classify(struct sk_buff *skb,
 			goto reset;
 		}
 #endif
-		if (err >= 0) {
-			/* Policy drop or drop reason is over-written by
-			 * classifiers with a bogus value(0) */
-			if (err == TC_ACT_SHOT &&
-			    res->drop_reason == SKB_NOT_DROPPED_YET)
-				tcf_set_drop_reason(res, orig_reason);
+		if (err >= 0)
 			return err;
-		}
 	}
 
 	if (unlikely(n)) {
-		tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+		tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 		return TC_ACT_SHOT;
 	}
 
@@ -1736,7 +1729,7 @@ static inline int __tcf_classify(struct sk_buff *skb,
 				       tp->chain->block->index,
 				       tp->prio & 0xffff,
 				       ntohs(tp->protocol));
-		tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+		tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 		return TC_ACT_SHOT;
 	}
 
@@ -1774,7 +1767,7 @@ int tcf_classify(struct sk_buff *skb,
 				n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie,
 								&act_index);
 				if (!n) {
-					tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+					tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 					return TC_ACT_SHOT;
 				}
 
@@ -1785,7 +1778,7 @@ int tcf_classify(struct sk_buff *skb,
 
 			fchain = tcf_chain_lookup_rcu(block, chain);
 			if (!fchain) {
-				tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 				return TC_ACT_SHOT;
 			}
 
@@ -1807,7 +1800,7 @@ int tcf_classify(struct sk_buff *skb,
 
 			ext = tc_skb_ext_alloc(skb);
 			if (WARN_ON_ONCE(!ext)) {
-				tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
 				return TC_ACT_SHOT;
 			}
 
-- 
2.25.1

Re: [PATCH net-next v3 1/3] net: sched: Move drop_reason to struct tc_skb_cb

[Daniel Borkmann]

On 12/5/23 9:50 PM, Victor Nogueira wrote:
> Move drop_reason from struct tcf_result to skb cb - more specifically to
> struct tc_skb_cb. With that, we'll be able to also set the drop reason for
> the remaining qdiscs (aside from clsact) that do not have access to
> tcf_result when time comes to set the skb drop reason.
> 
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next v3 1/3] net: sched: Move drop_reason to struct tc_skb_cb

[Simon Horman]

On Tue, Dec 05, 2023 at 05:50:28PM -0300, Victor Nogueira wrote:
> Move drop_reason from struct tcf_result to skb cb - more specifically to
> struct tc_skb_cb. With that, we'll be able to also set the drop reason for
> the remaining qdiscs (aside from clsact) that do not have access to
> tcf_result when time comes to set the skb drop reason.
> 
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>

Hi Victor,

The nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  include/net/pkt_cls.h     | 14 ++++++++++++--
>  include/net/pkt_sched.h   |  3 ++-
>  include/net/sch_generic.h |  1 -
>  net/core/dev.c            |  4 ++--
>  net/sched/act_api.c       |  2 +-
>  net/sched/cls_api.c       | 23 ++++++++---------------
>  6 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index a76c9171db0e..761e4500cca0 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -154,10 +154,20 @@ __cls_set_class(unsigned long *clp, unsigned long cl)
>  	return xchg(clp, cl);
>  }
>  
> -static inline void tcf_set_drop_reason(struct tcf_result *res,
> +struct tc_skb_cb;
> +
> +static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb);
> +

nit: It's not very important, as this code disappears in the next patch,
     but FWIIW I don't think the forward declarations of
     tc_skb_cb (struct or function) are needed.

> +static inline enum skb_drop_reason
> +tcf_get_drop_reason(const struct sk_buff *skb)
> +{
> +	return tc_skb_cb(skb)->drop_reason;
> +}
> +
> +static inline void tcf_set_drop_reason(const struct sk_buff *skb,
>  				       enum skb_drop_reason reason)
>  {
> -	res->drop_reason = reason;
> +	tc_skb_cb(skb)->drop_reason = reason;
>  }
>  
>  static inline void

...

[PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Victor Nogueira]

Incrementing on Daniel's patch[1], make tc-related drop reason more
flexible for remaining qdiscs - that is, all qdiscs aside from clsact.
In essence, the drop reason will be set by cls_api and act_api in case
any error occurred in the data path. With that, we can give the user more
detailed information so that they can distinguish between a policy drop
or an error drop.

[1] https://lore.kernel.org/all/20231009092655.22025-1-daniel@iogearbox.net

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/pkt_cls.h     | 16 ----------------
 include/net/pkt_sched.h   | 19 -------------------
 include/net/sch_generic.h | 31 +++++++++++++++++++++++++++++++
 net/core/dev.c            |  7 +++++--
 4 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 761e4500cca0..f308e8268651 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -154,22 +154,6 @@ __cls_set_class(unsigned long *clp, unsigned long cl)
 	return xchg(clp, cl);
 }
 
-struct tc_skb_cb;
-
-static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb);
-
-static inline enum skb_drop_reason
-tcf_get_drop_reason(const struct sk_buff *skb)
-{
-	return tc_skb_cb(skb)->drop_reason;
-}
-
-static inline void tcf_set_drop_reason(const struct sk_buff *skb,
-				       enum skb_drop_reason reason)
-{
-	tc_skb_cb(skb)->drop_reason = reason;
-}
-
 static inline void
 __tcf_bind_filter(struct Qdisc *q, struct tcf_result *r, unsigned long base)
 {
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 9b559aa5c079..1e200d9a066d 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -275,25 +275,6 @@ static inline void skb_txtime_consumed(struct sk_buff *skb)
 	skb->tstamp = ktime_set(0, 0);
 }
 
-struct tc_skb_cb {
-	struct qdisc_skb_cb qdisc_cb;
-	u32 drop_reason;
-
-	u16 zone; /* Only valid if post_ct = true */
-	u16 mru;
-	u8 post_ct:1;
-	u8 post_ct_snat:1;
-	u8 post_ct_dnat:1;
-};
-
-static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb)
-{
-	struct tc_skb_cb *cb = (struct tc_skb_cb *)skb->cb;
-
-	BUILD_BUG_ON(sizeof(*cb) > sizeof_field(struct sk_buff, cb));
-	return cb;
-}
-
 static inline bool tc_qdisc_stats_dump(struct Qdisc *sch,
 				       unsigned long cl,
 				       struct qdisc_walker *arg)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c499b56bb215..1d70c2c1572f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1036,6 +1036,37 @@ static inline struct sk_buff *qdisc_dequeue_head(struct Qdisc *sch)
 	return skb;
 }
 
+struct tc_skb_cb {
+	struct qdisc_skb_cb qdisc_cb;
+	u32 drop_reason;
+
+	u16 zone; /* Only valid if post_ct = true */
+	u16 mru;
+	u8 post_ct:1;
+	u8 post_ct_snat:1;
+	u8 post_ct_dnat:1;
+};
+
+static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb)
+{
+	struct tc_skb_cb *cb = (struct tc_skb_cb *)skb->cb;
+
+	BUILD_BUG_ON(sizeof(*cb) > sizeof_field(struct sk_buff, cb));
+	return cb;
+}
+
+static inline enum skb_drop_reason
+tcf_get_drop_reason(const struct sk_buff *skb)
+{
+	return tc_skb_cb(skb)->drop_reason;
+}
+
+static inline void tcf_set_drop_reason(const struct sk_buff *skb,
+				       enum skb_drop_reason reason)
+{
+	tc_skb_cb(skb)->drop_reason = reason;
+}
+
 /* Instead of calling kfree_skb() while root qdisc lock is held,
  * queue the skb for future freeing at end of __dev_xmit_skb()
  */
diff --git a/net/core/dev.c b/net/core/dev.c
index 4b84b72ebae8..f38c928a34aa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3753,6 +3753,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 
 	qdisc_calculate_pkt_len(skb, q);
 
+	tcf_set_drop_reason(skb, SKB_DROP_REASON_QDISC_DROP);
+
 	if (q->flags & TCQ_F_NOLOCK) {
 		if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
 		    qdisc_run_begin(q)) {
@@ -3782,7 +3784,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 no_lock_out:
 		if (unlikely(to_free))
 			kfree_skb_list_reason(to_free,
-					      SKB_DROP_REASON_QDISC_DROP);
+					      tcf_get_drop_reason(to_free));
 		return rc;
 	}
 
@@ -3837,7 +3839,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 	}
 	spin_unlock(root_lock);
 	if (unlikely(to_free))
-		kfree_skb_list_reason(to_free, SKB_DROP_REASON_QDISC_DROP);
+		kfree_skb_list_reason(to_free,
+				      tcf_get_drop_reason(to_free));
 	if (unlikely(contended))
 		spin_unlock(&q->busylock);
 	return rc;
-- 
2.25.1

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Daniel Borkmann]

On 12/5/23 9:50 PM, Victor Nogueira wrote:
> Incrementing on Daniel's patch[1], make tc-related drop reason more
> flexible for remaining qdiscs - that is, all qdiscs aside from clsact.
> In essence, the drop reason will be set by cls_api and act_api in case
> any error occurred in the data path. With that, we can give the user more
> detailed information so that they can distinguish between a policy drop
> or an error drop.
> 
> [1] https://lore.kernel.org/all/20231009092655.22025-1-daniel@iogearbox.net
> 
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Simon Horman]

On Tue, Dec 05, 2023 at 05:50:29PM -0300, Victor Nogueira wrote:
> Incrementing on Daniel's patch[1], make tc-related drop reason more
> flexible for remaining qdiscs - that is, all qdiscs aside from clsact.
> In essence, the drop reason will be set by cls_api and act_api in case
> any error occurred in the data path. With that, we can give the user more
> detailed information so that they can distinguish between a policy drop
> or an error drop.
> 
> [1] https://lore.kernel.org/all/20231009092655.22025-1-daniel@iogearbox.net
> 
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jakub Kicinski]

On Tue,  5 Dec 2023 17:50:29 -0300 Victor Nogueira wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4b84b72ebae8..f38c928a34aa 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3753,6 +3753,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  
>  	qdisc_calculate_pkt_len(skb, q);
>  
> +	tcf_set_drop_reason(skb, SKB_DROP_REASON_QDISC_DROP);
> +
>  	if (q->flags & TCQ_F_NOLOCK) {
>  		if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
>  		    qdisc_run_begin(q)) {
> @@ -3782,7 +3784,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  no_lock_out:
>  		if (unlikely(to_free))
>  			kfree_skb_list_reason(to_free,
> -					      SKB_DROP_REASON_QDISC_DROP);
> +					      tcf_get_drop_reason(to_free));
>  		return rc;
>  	}
>  
> @@ -3837,7 +3839,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
>  	}
>  	spin_unlock(root_lock);
>  	if (unlikely(to_free))
> -		kfree_skb_list_reason(to_free, SKB_DROP_REASON_QDISC_DROP);
> +		kfree_skb_list_reason(to_free,
> +				      tcf_get_drop_reason(to_free));

You stuff the drop reason into every skb but then only use the one from
the head? Herm. __qdisc_drop() only uses the next pointer can't we
overload the prev pointer to carry the drop reason. That means only
storing it if we already plan to drop the packet.

BTW I lack TC knowledge but struct tc_skb_cb is even more clsact
specific today than tcf_result. And reserving space for drop reason
in a state structure seems odd. Maybe that's just me.

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jamal Hadi Salim]

On Mon, Dec 11, 2023 at 9:25 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  5 Dec 2023 17:50:29 -0300 Victor Nogueira wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 4b84b72ebae8..f38c928a34aa 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3753,6 +3753,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> >
> >       qdisc_calculate_pkt_len(skb, q);
> >
> > +     tcf_set_drop_reason(skb, SKB_DROP_REASON_QDISC_DROP);
> > +
> >       if (q->flags & TCQ_F_NOLOCK) {
> >               if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) &&
> >                   qdisc_run_begin(q)) {
> > @@ -3782,7 +3784,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> >  no_lock_out:
> >               if (unlikely(to_free))
> >                       kfree_skb_list_reason(to_free,
> > -                                           SKB_DROP_REASON_QDISC_DROP);
> > +                                           tcf_get_drop_reason(to_free));
> >               return rc;
> >       }
> >
> > @@ -3837,7 +3839,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> >       }
> >       spin_unlock(root_lock);
> >       if (unlikely(to_free))
> > -             kfree_skb_list_reason(to_free, SKB_DROP_REASON_QDISC_DROP);
> > +             kfree_skb_list_reason(to_free,
> > +                                   tcf_get_drop_reason(to_free));
>
> You stuff the drop reason into every skb but then only use the one from
> the head? Herm. __qdisc_drop() only uses the next pointer can't we
> overload the prev pointer to carry the drop reason. That means only
> storing it if we already plan to drop the packet.
>

So when we looked at this code there was some mystery. It wasnt clear
how to_free could have more than one skb.
According to: 520ac30f4551 Eric says:

"I measured a performance increase of up to 12 %, but this patch is a
prereq so that future batches in enqueue() can fly. "

I am not sure if the batch enqueue ever happened and if it didnt then
there's only one skb in that list... When 7faef0547f4c added the
reason code it assumed a single code for the "list" - so it felt safer
to leave it that way. I guess it will depend on if a list could exist
to rethink this. Eric?

> BTW I lack TC knowledge but struct tc_skb_cb is even more clsact
> specific today than tcf_result. And reserving space for drop reason
> in a state structure seems odd. Maybe that's just me.

It is not clsact main use - it is a scratchpad for all of tc:

struct qdisc_skb_cb {
        struct {
                unsigned int            pkt_len;
                u16                     slave_dev_queue_mapping;
                u16                     tc_classid;
        };
#define QDISC_CB_PRIV_LEN 20
        unsigned char           data[QDISC_CB_PRIV_LEN];
};

struct tc_skb_cb {
        struct qdisc_skb_cb qdisc_cb;

        u16 mru;
        u8 post_ct:1;
        u8 post_ct_snat:1;
        u8 post_ct_dnat:1;
        u16 zone; /* Only valid if post_ct = true */
};

cheers,
jamal

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Eric Dumazet]

On Tue, Dec 12, 2023 at 5:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>

>
> So when we looked at this code there was some mystery. It wasnt clear
> how to_free could have more than one skb.

Some qdisc can free skbs at enqueue() time in  a batch for performance
reason (fq_codel_drop() is such an instance)

I agree that all skbs in this list have probably the same drop_reason ?

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jamal Hadi Salim]

On Tue, Dec 12, 2023 at 11:57 AM 'Eric Dumazet' via kernel issues
<kernel@mojatatu.com> wrote:
>
> On Tue, Dec 12, 2023 at 5:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
>
> >
> > So when we looked at this code there was some mystery. It wasnt clear
> > how to_free could have more than one skb.
>
> Some qdisc can free skbs at enqueue() time in  a batch for performance
> reason (fq_codel_drop() is such an instance)

Ok, makes more sense, should've caught that (there are others like taprio).

> I agree that all skbs in this list have probably the same drop_reason ?

It seems that way. We will review all the qdiscs to see if there's any
exceptions.
Thanks Eric.

cheers,
jamal

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jamal Hadi Salim]

On Tue, Dec 12, 2023 at 12:17 PM Jamal Hadi Salim <hadi@mojatatu.com> wrote:
>
> On Tue, Dec 12, 2023 at 11:57 AM 'Eric Dumazet' via kernel issues
> <kernel@mojatatu.com> wrote:
> >
> > On Tue, Dec 12, 2023 at 5:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> >
> > >
> > > So when we looked at this code there was some mystery. It wasnt clear
> > > how to_free could have more than one skb.
> >
> > Some qdisc can free skbs at enqueue() time in  a batch for performance
> > reason (fq_codel_drop() is such an instance)
>
> Ok, makes more sense, should've caught that (there are others like taprio).
>
> > I agree that all skbs in this list have probably the same drop_reason ?
>
> It seems that way. We will review all the qdiscs to see if there's any
> exceptions.

Putting this to rest:
Other than fq codel, the others that deal with multiple skbs due to
gso segments. So the conclusion is:  if we have a bunch in the list
then they all suffer the same fate. So a single reason for the list is
sufficient.

cheers,
jamal

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jakub Kicinski]

On Wed, 13 Dec 2023 13:36:31 -0500 Jamal Hadi Salim wrote:
> Putting this to rest:
> Other than fq codel, the others that deal with multiple skbs due to
> gso segments. So the conclusion is:  if we have a bunch in the list
> then they all suffer the same fate. So a single reason for the list is
> sufficient.

Alright.

I'm still a bit confused about the cb, tho.

struct qdisc_skb_cb is the state struct.
But we put the drop reason in struct tc_skb_cb.
How does that work. Qdiscs will assume they own all of
qdisc_skb_cb::data ?

Maybe some documentation about the lifetimes of these things
would clarify things?

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jamal Hadi Salim]

On Wed, Dec 13, 2023 at 4:08 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 13 Dec 2023 13:36:31 -0500 Jamal Hadi Salim wrote:
> > Putting this to rest:
> > Other than fq codel, the others that deal with multiple skbs due to
> > gso segments. So the conclusion is:  if we have a bunch in the list
> > then they all suffer the same fate. So a single reason for the list is
> > sufficient.
>
> Alright.
>
> I'm still a bit confused about the cb, tho.
>
> struct qdisc_skb_cb is the state struct.

Per packet state within tc though, no? Once it leaves tc whatever sits
in that space cant be trusted to be valid.
To answer your earlier question tcf_result is not available at the
qdisc level (when we call free_skb_list() but cb is and thats why we
used it)

> But we put the drop reason in struct tc_skb_cb.
> How does that work. Qdiscs will assume they own all of
> qdisc_skb_cb::data ?
>

Short answer, yes. Anyone can scribble over that. And multiple
consumers have a food fight going on - but it is expected behavior:
ebpf's skb->cb, cake, fq_codel etc - all use qdisc_skb_cb::data.
Out of the 48B in skb->cb qdisc_skb_cb redefined the first 28B and
left in qdisc_skb_cb::data as free-for-all space. I think,
unfortunately, that is now cast in stone.
Which still leaves us 20 bytes which is now being squatered by
tc_skb_cb where the drop reason was placed. Shit, i just noticed this
is not exclusive - seems like
drivers/net/amt.c is using this space - not sure why it is even using
tc space. I am wondering if we can make it use the 20B scratch pad.
+Cc author Taehee Yoo - it doesnt seem to conflict but strange that it
is considering qdisc_skb_cb

> Maybe some documentation about the lifetimes of these things
> would clarify things?

What text do you think makes sense and where should it go?

cheers,
jamal

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Taehee Yoo]

On 12/15/23 00:31, Jamal Hadi Salim wrote:

Hi Jamal,

 > On Wed, Dec 13, 2023 at 4:08 PM Jakub Kicinski <kuba@kernel.org> wrote:
 >>
 >> On Wed, 13 Dec 2023 13:36:31 -0500 Jamal Hadi Salim wrote:
 >>> Putting this to rest:
 >>> Other than fq codel, the others that deal with multiple skbs due to
 >>> gso segments. So the conclusion is: if we have a bunch in the list
 >>> then they all suffer the same fate. So a single reason for the list is
 >>> sufficient.
 >>
 >> Alright.
 >>
 >> I'm still a bit confused about the cb, tho.
 >>
 >> struct qdisc_skb_cb is the state struct.
 >
 > Per packet state within tc though, no? Once it leaves tc whatever sits
 > in that space cant be trusted to be valid.
 > To answer your earlier question tcf_result is not available at the
 > qdisc level (when we call free_skb_list() but cb is and thats why we
 > used it)
 >
 >> But we put the drop reason in struct tc_skb_cb.
 >> How does that work. Qdiscs will assume they own all of
 >> qdisc_skb_cb::data ?
 >>
 >
 > Short answer, yes. Anyone can scribble over that. And multiple
 > consumers have a food fight going on - but it is expected behavior:
 > ebpf's skb->cb, cake, fq_codel etc - all use qdisc_skb_cb::data.
 > Out of the 48B in skb->cb qdisc_skb_cb redefined the first 28B and
 > left in qdisc_skb_cb::data as free-for-all space. I think,
 > unfortunately, that is now cast in stone.
 > Which still leaves us 20 bytes which is now being squatered by
 > tc_skb_cb where the drop reason was placed. Shit, i just noticed this
 > is not exclusive - seems like
 > drivers/net/amt.c is using this space - not sure why it is even using
 > tc space. I am wondering if we can make it use the 20B scratch pad.
 > +Cc author Taehee Yoo - it doesnt seem to conflict but strange that it
 > is considering qdisc_skb_cb

The reason why amt considers qdisc_skb_cb is to not use CB area the TC 
is using.
When amt driver send igmp/mld packet, it stores tunnel data in CB before 
calling dev_queue_xmit().
Then, it uses that tunnel data from CB in the amt_dev_xmit().
So, amt CB area should not be used by TC, this is the reason why it 
considers qdisc_skb_cb size.
But It looks wrong, it should use tc_skb_cb instead of qdisc_skb_cb to 
fully avoid CB area of TC, right?

 >
 >> Maybe some documentation about the lifetimes of these things
 >> would clarify things?
 >
 > What text do you think makes sense and where should it go?
 >
 > cheers,
 > jamal

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jamal Hadi Salim]

On Thu, Dec 14, 2023 at 12:18 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On 12/15/23 00:31, Jamal Hadi Salim wrote:
>
> Hi Jamal,
>
>  > On Wed, Dec 13, 2023 at 4:08 PM Jakub Kicinski <kuba@kernel.org> wrote:
>  >>
>  >> On Wed, 13 Dec 2023 13:36:31 -0500 Jamal Hadi Salim wrote:
>  >>> Putting this to rest:
>  >>> Other than fq codel, the others that deal with multiple skbs due to
>  >>> gso segments. So the conclusion is: if we have a bunch in the list
>  >>> then they all suffer the same fate. So a single reason for the list is
>  >>> sufficient.
>  >>
>  >> Alright.
>  >>
>  >> I'm still a bit confused about the cb, tho.
>  >>
>  >> struct qdisc_skb_cb is the state struct.
>  >
>  > Per packet state within tc though, no? Once it leaves tc whatever sits
>  > in that space cant be trusted to be valid.
>  > To answer your earlier question tcf_result is not available at the
>  > qdisc level (when we call free_skb_list() but cb is and thats why we
>  > used it)
>  >
>  >> But we put the drop reason in struct tc_skb_cb.
>  >> How does that work. Qdiscs will assume they own all of
>  >> qdisc_skb_cb::data ?
>  >>
>  >
>  > Short answer, yes. Anyone can scribble over that. And multiple
>  > consumers have a food fight going on - but it is expected behavior:
>  > ebpf's skb->cb, cake, fq_codel etc - all use qdisc_skb_cb::data.
>  > Out of the 48B in skb->cb qdisc_skb_cb redefined the first 28B and
>  > left in qdisc_skb_cb::data as free-for-all space. I think,
>  > unfortunately, that is now cast in stone.
>  > Which still leaves us 20 bytes which is now being squatered by
>  > tc_skb_cb where the drop reason was placed. Shit, i just noticed this
>  > is not exclusive - seems like
>  > drivers/net/amt.c is using this space - not sure why it is even using
>  > tc space. I am wondering if we can make it use the 20B scratch pad.
>  > +Cc author Taehee Yoo - it doesnt seem to conflict but strange that it
>  > is considering qdisc_skb_cb
>
> The reason why amt considers qdisc_skb_cb is to not use CB area the TC
> is using.
> When amt driver send igmp/mld packet, it stores tunnel data in CB before
> calling dev_queue_xmit().
> Then, it uses that tunnel data from CB in the amt_dev_xmit().
> So, amt CB area should not be used by TC, this is the reason why it
> considers qdisc_skb_cb size.
> But It looks wrong, it should use tc_skb_cb instead of qdisc_skb_cb to
> fully avoid CB area of TC, right?


Yeah, that doesnt seem safe if you are storing state expecting it to
be intact afterwards - tc will definitely overwrite parts of it today.
See this:

struct qdisc_skb_cb {
        struct {
                unsigned int            pkt_len;
                u16                     slave_dev_queue_mapping;
                u16                     tc_classid;
        };
#define QDISC_CB_PRIV_LEN 20
        unsigned char           data[QDISC_CB_PRIV_LEN];
};

struct tc_skb_cb {
        struct qdisc_skb_cb qdisc_cb;

        u16 mru;
        u8 post_ct:1;
        u8 post_ct_snat:1;
        u8 post_ct_dnat:1;
        u16 zone; /* Only valid if post_ct = true */
};

tc_skb_cb->mru etc are writting over the area you are using.

The rule is you cant trust skb->cb content after it goes out of your "domain".

cheers,
jamal

>  >
>  >> Maybe some documentation about the lifetimes of these things
>  >> would clarify things?
>  >
>  > What text do you think makes sense and where should it go?
>  >
>  > cheers,
>  > jamal

Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

[Jakub Kicinski]

On Thu, 14 Dec 2023 10:31:24 -0500 Jamal Hadi Salim wrote:
> > I'm still a bit confused about the cb, tho.
> >
> > struct qdisc_skb_cb is the state struct.  
> 
> Per packet state within tc though, no? Once it leaves tc whatever sits
> in that space cant be trusted to be valid.
> To answer your earlier question tcf_result is not available at the
> qdisc level (when we call free_skb_list() but cb is and thats why we
> used it)
> 
> > But we put the drop reason in struct tc_skb_cb.
> > How does that work. Qdiscs will assume they own all of
> > qdisc_skb_cb::data ?
> >  
> 
> Short answer, yes. Anyone can scribble over that. And multiple
> consumers have a food fight going on - but it is expected behavior:
> ebpf's skb->cb, cake, fq_codel etc - all use qdisc_skb_cb::data.
> Out of the 48B in skb->cb qdisc_skb_cb redefined the first 28B and
> left in qdisc_skb_cb::data as free-for-all space. I think,
> unfortunately, that is now cast in stone.
> Which still leaves us 20 bytes which is now being squatered by
> tc_skb_cb where the drop reason was placed.

Okay I got it now, for some reason I thought the new fields in
struct tc_skb_cb overlay the data[] in struct qdisc_skb_cb...

[PATCH net-next v3 3/3] net: sched: Add initial TC error skb drop reasons

[Victor Nogueira]

Continue expanding Daniel's patch by adding new skb drop reasons that
are idiosyncratic to TC.

More specifically:

- SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND: tc cookie was looked up using
  ext, but was not found.

- SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH: tc ext was looked up using cookie
  and either was not found or different from expected.

- SKB_DROP_REASON_TC_CHAIN_NOTFOUND: tc chain lookup failed.

- SKB_DROP_REASON_TC_RECLASSIFY_LOOP: tc exceeded max reclassify loop
  iterations

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/dropreason-core.h | 30 +++++++++++++++++++++++++++---
 net/sched/act_api.c           |  3 ++-
 net/sched/cls_api.c           | 22 ++++++++++++++--------
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 3c70ad53a49c..fa6ace8f1611 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -85,7 +85,11 @@
 	FN(IPV6_NDISC_BAD_OPTIONS)	\
 	FN(IPV6_NDISC_NS_OTHERHOST)	\
 	FN(QUEUE_PURGE)			\
-	FN(TC_ERROR)			\
+	FN(TC_EXT_COOKIE_NOTFOUND)	\
+	FN(TC_COOKIE_EXT_MISMATCH)	\
+	FN(TC_COOKIE_MISMATCH)		\
+	FN(TC_CHAIN_NOTFOUND)		\
+	FN(TC_RECLASSIFY_LOOP)		\
 	FNe(MAX)
 
 /**
@@ -376,8 +380,28 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_IPV6_NDISC_NS_OTHERHOST,
 	/** @SKB_DROP_REASON_QUEUE_PURGE: bulk free. */
 	SKB_DROP_REASON_QUEUE_PURGE,
-	/** @SKB_DROP_REASON_TC_ERROR: generic internal tc error. */
-	SKB_DROP_REASON_TC_ERROR,
+	/**
+	 * @SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND: tc cookie was looked up
+	 * using ext, but was not found.
+	 */
+	SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND,
+	/**
+	 * @SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH: tc ext was lookup using
+	 * cookie and either was not found or different from expected.
+	 */
+	SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH,
+	/**
+	 * @SKB_DROP_REASON_TC_COOKIE_MISMATCH: tc cookie available but was
+	 * unable to match to filter.
+	 */
+	SKB_DROP_REASON_TC_COOKIE_MISMATCH,
+	/** @SKB_DROP_REASON_TC_CHAIN_NOTFOUND: tc chain lookup failed. */
+	SKB_DROP_REASON_TC_CHAIN_NOTFOUND,
+	/**
+	 * @SKB_DROP_REASON_TC_RECLASSIFY_LOOP: tc exceeded max reclassify loop
+	 * iterations.
+	 */
+	SKB_DROP_REASON_TC_RECLASSIFY_LOOP,
 	/**
 	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
 	 * shouldn't be used as a real 'reason' - only for tracing code gen
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f2b136ce9282..ba5aad9be161 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1098,7 +1098,8 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 			}
 		} else if (TC_ACT_EXT_CMP(ret, TC_ACT_GOTO_CHAIN)) {
 			if (unlikely(!rcu_access_pointer(a->goto_chain))) {
-				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb,
+						    SKB_DROP_REASON_TC_CHAIN_NOTFOUND);
 				return TC_ACT_SHOT;
 			}
 			tcf_action_goto_chain_exec(a, res);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 32457a236d77..5012fc0a24a1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1682,13 +1682,15 @@ static inline int __tcf_classify(struct sk_buff *skb,
 			 */
 			if (unlikely(n->tp != tp || n->tp->chain != n->chain ||
 				     !tp->ops->get_exts)) {
-				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb,
+						    SKB_DROP_REASON_TC_COOKIE_MISMATCH);
 				return TC_ACT_SHOT;
 			}
 
 			exts = tp->ops->get_exts(tp, n->handle);
 			if (unlikely(!exts || n->exts != exts)) {
-				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb,
+						    SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH);
 				return TC_ACT_SHOT;
 			}
 
@@ -1717,7 +1719,8 @@ static inline int __tcf_classify(struct sk_buff *skb,
 	}
 
 	if (unlikely(n)) {
-		tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+		tcf_set_drop_reason(skb,
+				    SKB_DROP_REASON_TC_COOKIE_MISMATCH);
 		return TC_ACT_SHOT;
 	}
 
@@ -1729,7 +1732,8 @@ static inline int __tcf_classify(struct sk_buff *skb,
 				       tp->chain->block->index,
 				       tp->prio & 0xffff,
 				       ntohs(tp->protocol));
-		tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+		tcf_set_drop_reason(skb,
+				    SKB_DROP_REASON_TC_RECLASSIFY_LOOP);
 		return TC_ACT_SHOT;
 	}
 
@@ -1767,7 +1771,8 @@ int tcf_classify(struct sk_buff *skb,
 				n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie,
 								&act_index);
 				if (!n) {
-					tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+					tcf_set_drop_reason(skb,
+							    SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND);
 					return TC_ACT_SHOT;
 				}
 
@@ -1778,7 +1783,9 @@ int tcf_classify(struct sk_buff *skb,
 
 			fchain = tcf_chain_lookup_rcu(block, chain);
 			if (!fchain) {
-				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb,
+						    SKB_DROP_REASON_TC_CHAIN_NOTFOUND);
+
 				return TC_ACT_SHOT;
 			}
 
@@ -1800,10 +1807,9 @@ int tcf_classify(struct sk_buff *skb,
 
 			ext = tc_skb_ext_alloc(skb);
 			if (WARN_ON_ONCE(!ext)) {
-				tcf_set_drop_reason(skb, SKB_DROP_REASON_TC_ERROR);
+				tcf_set_drop_reason(skb, SKB_DROP_REASON_NOMEM);
 				return TC_ACT_SHOT;
 			}
-
 			ext->chain = last_executed_chain;
 			ext->mru = cb->mru;
 			ext->post_ct = cb->post_ct;
-- 
2.25.1

Re: [PATCH net-next v3 3/3] net: sched: Add initial TC error skb drop reasons

[Simon Horman]

On Tue, Dec 05, 2023 at 05:50:30PM -0300, Victor Nogueira wrote:
> Continue expanding Daniel's patch by adding new skb drop reasons that
> are idiosyncratic to TC.
> 
> More specifically:
> 
> - SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND: tc cookie was looked up using
>   ext, but was not found.
> 
> - SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH: tc ext was looked up using cookie
>   and either was not found or different from expected.
> 
> - SKB_DROP_REASON_TC_CHAIN_NOTFOUND: tc chain lookup failed.
> 
> - SKB_DROP_REASON_TC_RECLASSIFY_LOOP: tc exceeded max reclassify loop
>   iterations
> 
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v3 3/3] net: sched: Add initial TC error skb drop reasons

[Jakub Kicinski]

On Tue,  5 Dec 2023 17:50:30 -0300 Victor Nogueira wrote:
> +	/**
> +	 * @SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND: tc cookie was looked up
> +	 * using ext, but was not found.
> +	 */
> +	SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND,
> +	/**
> +	 * @SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH: tc ext was lookup using
> +	 * cookie and either was not found or different from expected.
> +	 */
> +	SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH,
> +	/**
> +	 * @SKB_DROP_REASON_TC_COOKIE_MISMATCH: tc cookie available but was
> +	 * unable to match to filter.
> +	 */
> +	SKB_DROP_REASON_TC_COOKIE_MISMATCH,

Do we really need 3 reasons for COOKIE?

Also cookie here is offload state thing right? I wonder how many admins
/ SREs would be able to figure out what's going on based on this kdoc :S
Let alone if it's a configuration problem or a race condition...

Re: [PATCH net-next v3 3/3] net: sched: Add initial TC error skb drop reasons

[Victor Nogueira]

On 11/12/2023 23:30, Jakub Kicinski wrote:
> On Tue,  5 Dec 2023 17:50:30 -0300 Victor Nogueira wrote:
>> +	/**
>> +	 * @SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND: tc cookie was looked up
>> +	 * using ext, but was not found.
>> +	 */
>> +	SKB_DROP_REASON_TC_EXT_COOKIE_NOTFOUND,
>> +	/**
>> +	 * @SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH: tc ext was lookup using
>> +	 * cookie and either was not found or different from expected.
>> +	 */
>> +	SKB_DROP_REASON_TC_COOKIE_EXT_MISMATCH,
>> +	/**
>> +	 * @SKB_DROP_REASON_TC_COOKIE_MISMATCH: tc cookie available but was
>> +	 * unable to match to filter.
>> +	 */
>> +	SKB_DROP_REASON_TC_COOKIE_MISMATCH,
> 
> Do we really need 3 reasons for COOKIE?
> 
> Also cookie here is offload state thing right? I wonder how many admins
> / SREs would be able to figure out what's going on based on this kdoc :S
> Let alone if it's a configuration problem or a race condition...

Yeah, probably overkill. Will merge into one.

cheers,
Victor