* [PATCH net-next v2 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs
@ 2023-12-01 23:00 Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb Victor Nogueira
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Victor Nogueira @ 2023-12-01 23:00 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, daniel
Cc: dcaratti, netdev, kernel
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 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 | 12 ++++++++----
net/sched/act_api.c | 3 ++-
net/sched/cls_api.c | 31 +++++++++++++++----------------
7 files changed, 83 insertions(+), 49 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb
2023-12-01 23:00 [PATCH net-next v2 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
@ 2023-12-01 23:00 ` Victor Nogueira
2023-12-05 11:06 ` Daniel Borkmann
2023-12-01 23:00 ` [PATCH net-next v2 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons Victor Nogueira
2 siblings, 1 reply; 10+ messages in thread
From: Victor Nogueira @ 2023-12-01 23:00 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, daniel
Cc: dcaratti, netdev, kernel
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 | 1 +
include/net/sch_generic.h | 1 -
net/core/dev.c | 5 +++--
net/sched/act_api.c | 2 +-
net/sched/cls_api.c | 23 ++++++++---------------
6 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a76c9171db0e..7bd7ea511100 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
+tc_skb_cb_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..f09bfa1efed0 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -277,6 +277,7 @@ static inline void skb_txtime_consumed(struct sk_buff *skb)
struct tc_skb_cb {
struct qdisc_skb_cb qdisc_cb;
+ u32 drop_reason;
u16 mru;
u8 post_ct:1;
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 3950ced396b5..323496ca0dc3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3924,14 +3924,15 @@ 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;
+ tc_skb_cb(skb)->post_ct = false;
+ 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 = tc_skb_cb_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 c39252d61ebb..12ac05857045 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next v2 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs
2023-12-01 23:00 [PATCH net-next v2 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb Victor Nogueira
@ 2023-12-01 23:00 ` Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons Victor Nogueira
2 siblings, 0 replies; 10+ messages in thread
From: Victor Nogueira @ 2023-12-01 23:00 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, daniel
Cc: dcaratti, netdev, kernel
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 7bd7ea511100..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
-tc_skb_cb_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 f09bfa1efed0..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 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)
-{
- 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..07ca001e94e0 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 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)
+{
+ 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
+tc_skb_cb_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 323496ca0dc3..861c54241a53 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3754,6 +3754,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)) {
@@ -3783,7 +3785,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);
+ tc_skb_cb_drop_reason(to_free));
return rc;
}
@@ -3838,7 +3840,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,
+ tc_skb_cb_drop_reason(to_free));
if (unlikely(contended))
spin_unlock(&q->busylock);
return rc;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons
2023-12-01 23:00 [PATCH net-next v2 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
@ 2023-12-01 23:00 ` Victor Nogueira
2023-12-05 22:28 ` Dave Taht
2 siblings, 1 reply; 10+ messages in thread
From: Victor Nogueira @ 2023-12-01 23:00 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, daniel
Cc: dcaratti, netdev, kernel
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 12ac05857045..429cb232e17b 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb
2023-12-01 23:00 ` [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb Victor Nogueira
@ 2023-12-05 11:06 ` Daniel Borkmann
2023-12-05 11:32 ` Paolo Abeni
2023-12-05 16:27 ` Victor Nogueira
0 siblings, 2 replies; 10+ messages in thread
From: Daniel Borkmann @ 2023-12-05 11:06 UTC (permalink / raw)
To: Victor Nogueira, jhs, xiyou.wangcong, jiri, davem, edumazet,
kuba, pabeni
Cc: dcaratti, netdev, kernel
On 12/2/23 12:00 AM, 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>
> ---
> include/net/pkt_cls.h | 14 ++++++++++++--
> include/net/pkt_sched.h | 1 +
> include/net/sch_generic.h | 1 -
> net/core/dev.c | 5 +++--
> net/sched/act_api.c | 2 +-
> net/sched/cls_api.c | 23 ++++++++---------------
> 6 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index a76c9171db0e..7bd7ea511100 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
> +tc_skb_cb_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..f09bfa1efed0 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -277,6 +277,7 @@ static inline void skb_txtime_consumed(struct sk_buff *skb)
>
> struct tc_skb_cb {
> struct qdisc_skb_cb qdisc_cb;
> + u32 drop_reason;
>
> u16 mru;
Probably also makes sense to reorder zone below mru.
> u8 post_ct:1;
> 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 3950ced396b5..323496ca0dc3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3924,14 +3924,15 @@ 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;
> + tc_skb_cb(skb)->post_ct = false;
Why the double assignment ?
> + 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 = tc_skb_cb_drop_reason(skb);
nit: I'd rename into tcf_get_drop_reason() so it aligns with the tcf_set_drop_reason().
It's weird to name the getter tc_skb_cb_drop_reason() instead.
> 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 c39252d61ebb..12ac05857045 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;
> }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb
2023-12-05 11:06 ` Daniel Borkmann
@ 2023-12-05 11:32 ` Paolo Abeni
2023-12-05 16:28 ` Victor Nogueira
2023-12-05 16:27 ` Victor Nogueira
1 sibling, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2023-12-05 11:32 UTC (permalink / raw)
To: Daniel Borkmann, Victor Nogueira, jhs, xiyou.wangcong, jiri,
davem, edumazet, kuba
Cc: dcaratti, netdev, kernel
On Tue, 2023-12-05 at 12:06 +0100, Daniel Borkmann wrote:
> On 12/2/23 12:00 AM, 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>
> > ---
> > include/net/pkt_cls.h | 14 ++++++++++++--
> > include/net/pkt_sched.h | 1 +
> > include/net/sch_generic.h | 1 -
> > net/core/dev.c | 5 +++--
> > net/sched/act_api.c | 2 +-
> > net/sched/cls_api.c | 23 ++++++++---------------
> > 6 files changed, 25 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> > index a76c9171db0e..7bd7ea511100 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
> > +tc_skb_cb_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..f09bfa1efed0 100644
> > --- a/include/net/pkt_sched.h
> > +++ b/include/net/pkt_sched.h
> > @@ -277,6 +277,7 @@ static inline void skb_txtime_consumed(struct sk_buff *skb)
> >
> > struct tc_skb_cb {
> > struct qdisc_skb_cb qdisc_cb;
> > + u32 drop_reason;
> >
> > u16 mru;
>
> Probably also makes sense to reorder zone below mru.
Or move 'zone' here. It's very minor but I would prefer the latter ;)
(and leave the hole at the end of the struct)
Cheers,
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb
2023-12-05 11:06 ` Daniel Borkmann
2023-12-05 11:32 ` Paolo Abeni
@ 2023-12-05 16:27 ` Victor Nogueira
1 sibling, 0 replies; 10+ messages in thread
From: Victor Nogueira @ 2023-12-05 16:27 UTC (permalink / raw)
To: Daniel Borkmann, Victor Nogueira, jhs, xiyou.wangcong, jiri,
davem, edumazet, kuba, pabeni
Cc: dcaratti, netdev, kernel
On 05/12/2023 08:06, Daniel Borkmann wrote:
>> [...]
>> 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 3950ced396b5..323496ca0dc3 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3924,14 +3924,15 @@ 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;
>> + tc_skb_cb(skb)->post_ct = false;
>
> Why the double assignment ?
Sigh, sorry will change that in v3.
>
>> + 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 = tc_skb_cb_drop_reason(skb);
>
> nit: I'd rename into tcf_get_drop_reason() so it aligns with the
> tcf_set_drop_reason().
> It's weird to name the getter tc_skb_cb_drop_reason() instead.
Seems more consistent, will do.
>> [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb
2023-12-05 11:32 ` Paolo Abeni
@ 2023-12-05 16:28 ` Victor Nogueira
0 siblings, 0 replies; 10+ messages in thread
From: Victor Nogueira @ 2023-12-05 16:28 UTC (permalink / raw)
To: Paolo Abeni, Daniel Borkmann, Victor Nogueira, jhs,
xiyou.wangcong, jiri, davem, edumazet, kuba
Cc: dcaratti, netdev, kernel
On 05/12/2023 08:32, Paolo Abeni wrote:
> On Tue, 2023-12-05 at 12:06 +0100, Daniel Borkmann wrote:
>> On 12/2/23 12:00 AM, 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>
>>> ---
>>> include/net/pkt_cls.h | 14 ++++++++++++--
>>> include/net/pkt_sched.h | 1 +
>>> include/net/sch_generic.h | 1 -
>>> net/core/dev.c | 5 +++--
>>> net/sched/act_api.c | 2 +-
>>> net/sched/cls_api.c | 23 ++++++++---------------
>>> 6 files changed, 25 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>> index a76c9171db0e..7bd7ea511100 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
>>> +tc_skb_cb_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..f09bfa1efed0 100644
>>> --- a/include/net/pkt_sched.h
>>> +++ b/include/net/pkt_sched.h
>>> @@ -277,6 +277,7 @@ static inline void skb_txtime_consumed(struct sk_buff *skb)
>>>
>>> struct tc_skb_cb {
>>> struct qdisc_skb_cb qdisc_cb;
>>> + u32 drop_reason;
>>>
>>> u16 mru;
>>
>> Probably also makes sense to reorder zone below mru.
>
> Or move 'zone' here. It's very minor but I would prefer the latter ;)
> (and leave the hole at the end of the struct)
Yes, this looks better.
Thank you, I'll proceed this way.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons
2023-12-01 23:00 ` [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons Victor Nogueira
@ 2023-12-05 22:28 ` Dave Taht
2023-12-05 23:19 ` Victor Nogueira
0 siblings, 1 reply; 10+ messages in thread
From: Dave Taht @ 2023-12-05 22:28 UTC (permalink / raw)
To: Victor Nogueira
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, daniel,
dcaratti, netdev, kernel
On Fri, Dec 1, 2023 at 6:00 PM Victor Nogueira <victor@mojatatu.com> 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>
> ---
> 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 12ac05857045..429cb232e17b 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
>
>
I have been meaning to get around to adding
QDISC_DROP_REASON_{CONGEST,OVERFLOW,FLOOD,SPIKE} to
cake/fq_codel/red/etc for some time now. Would this be the right
facility to leverage (or something more direct?) I discussed the why
at netdevconf:
https://docs.google.com/document/d/1tTYBPeaRdCO9AGTGQCpoiuLORQzN_bG3TAkEolJPh28/edit
Other names welcomed. Otherwise I suppose I will wait for this stuff to land:
Acked-By: Dave Taht <dave.taht@gmail.com>
--
:( My old R&D campus is up for sale: https://tinyurl.com/yurtlab
Dave Täht CSO, LibreQos
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons
2023-12-05 22:28 ` Dave Taht
@ 2023-12-05 23:19 ` Victor Nogueira
0 siblings, 0 replies; 10+ messages in thread
From: Victor Nogueira @ 2023-12-05 23:19 UTC (permalink / raw)
To: Dave Taht
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, daniel,
dcaratti, netdev, kernel
On 05/12/2023 19:28, Dave Taht wrote:
> On Fri, Dec 1, 2023 at 6:00 PM Victor Nogueira <victor@mojatatu.com> 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>
>> ---
>>
>> [...]
>
> I have been meaning to get around to adding
> QDISC_DROP_REASON_{CONGEST,OVERFLOW,FLOOD,SPIKE} to
> cake/fq_codel/red/etc for some time now. Would this be the right
> facility to leverage (or something more direct?) I discussed the why
> at netdevconf:
>
> https://docs.google.com/document/d/1tTYBPeaRdCO9AGTGQCpoiuLORQzN_bG3TAkEolJPh28/edit
Yes, I think here will be the place to add these new drop reasons.
After this patch lands, we will add more, but feel free to also
propose others.
cheers,
Victor
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-05 23:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-01 23:00 [PATCH net-next v2 0/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 1/3] net: sched: Move drop_reason to struct tc_skb_cb Victor Nogueira
2023-12-05 11:06 ` Daniel Borkmann
2023-12-05 11:32 ` Paolo Abeni
2023-12-05 16:28 ` Victor Nogueira
2023-12-05 16:27 ` Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs Victor Nogueira
2023-12-01 23:00 ` [PATCH net-next v2 3/3] net: sched: Add initial TC error skb drop reasons Victor Nogueira
2023-12-05 22:28 ` Dave Taht
2023-12-05 23:19 ` Victor Nogueira
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.