[PATCH net-next 0/7] net: dev: add skb drop reasons to net/core/dev.c

[PATCH net-next 0/7] net: dev: add skb drop reasons to net/core/dev.c

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

In the commit c504e5c2f964 ("net: skb: introduce kfree_skb_reason()"),
we added the support of reporting the reasons of skb drops to kfree_skb
tracepoint. And in this series patches, reasons for skb drops are added
to the link layer, which means that 'net/core/dev.c' is our target.

Following functions are processed:

sch_handle_egress()
__dev_xmit_skb()
enqueue_to_backlog()
do_xdp_generic()
sch_handle_ingress()
__netif_receive_skb_core()

and following new drop reasons are added (what they mean can be see in
the document of them):

SKB_DROP_REASON_QDISC_EGRESS
SKB_DROP_REASON_QDISC_DROP
SKB_DROP_REASON_CPU_BACKLOG
SKB_DROP_REASON_XDP
SKB_DROP_REASON_QDISC_INGRESS
SKB_DROP_REASON_PTYPE_ABSENT

In order to add skb drop reasons to kfree_skb_list(), the function
kfree_skb_list_reason() is introduced in the 2th patch, which will be
used in __dev_xmit_skb() in the 3th patch.


Menglong Dong (7):
  net: dev: use kfree_skb_reason() for sch_handle_egress()
  net: skb: introduce the function kfree_skb_list_reason()
  net: dev: add skb drop reasons to __dev_xmit_skb()
  net: dev: use kfree_skb_reason() for enqueue_to_backlog()
  net: dev: use kfree_skb_reason() for do_xdp_generic()
  net: dev: use kfree_skb_reason() for sch_handle_ingress()
  net: dev: use kfree_skb_reason() for __netif_receive_skb_core()

 include/linux/skbuff.h     | 32 +++++++++++++++++++++++++++++++-
 include/trace/events/skb.h |  5 +++++
 net/core/dev.c             | 25 ++++++++++++++++---------
 net/core/skbuff.c          |  7 ++++---
 4 files changed, 56 insertions(+), 13 deletions(-)

-- 
2.35.1

[PATCH net-next 1/7] net: dev: use kfree_skb_reason() for sch_handle_egress()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in sch_handle_egress() with kfree_skb_reason().
The drop reason SKB_DROP_REASON_QDISC_EGRESS is introduced. Considering
the code path of qdisc egerss, we make it distinct with the drop reason
of SKB_DROP_REASON_QDISC_DROP in the next commit.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 4 ++++
 include/trace/events/skb.h | 1 +
 net/core/dev.c             | 2 +-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d67941f78b92..ebd18850b63e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -394,6 +394,10 @@ enum skb_drop_reason {
 						 * entry is full
 						 */
 	SKB_DROP_REASON_NEIGH_DEAD,	/* neigh entry is dead */
+	SKB_DROP_REASON_QDISC_EGRESS,	/* qdisc of type egress check
+					 * failed (mapbe an eBPF program
+					 * is tricking?)
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 1977f301260d..3cadf13448c6 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -45,6 +45,7 @@
 	EM(SKB_DROP_REASON_NEIGH_FAILED, NEIGH_FAILED)		\
 	EM(SKB_DROP_REASON_NEIGH_QUEUEFULL, NEIGH_QUEUEFULL)	\
 	EM(SKB_DROP_REASON_NEIGH_DEAD, NEIGH_DEAD)		\
+	EM(SKB_DROP_REASON_QDISC_EGRESS, QDISC_EGRESS)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/core/dev.c b/net/core/dev.c
index 2d6771075720..828946d74a19 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3860,7 +3860,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 	case TC_ACT_SHOT:
 		mini_qdisc_qstats_cpu_drop(miniq);
 		*ret = NET_XMIT_DROP;
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_EGRESS);
 		return NULL;
 	case TC_ACT_STOLEN:
 	case TC_ACT_QUEUED:
-- 
2.35.1

[PATCH net-next 2/7] net: skb: introduce the function kfree_skb_list_reason()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

To report reasons of skb drops, introduce the function
kfree_skb_list_reason() and make kfree_skb_list() an inline call to
it. This function will be used in the next commit in
__dev_xmit_skb().

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h | 8 +++++++-
 net/core/skbuff.c      | 7 ++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ebd18850b63e..e344603aecc4 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1194,10 +1194,16 @@ static inline void kfree_skb(struct sk_buff *skb)
 }
 
 void skb_release_head_state(struct sk_buff *skb);
-void kfree_skb_list(struct sk_buff *segs);
+void kfree_skb_list_reason(struct sk_buff *segs,
+			   enum skb_drop_reason reason);
 void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt);
 void skb_tx_error(struct sk_buff *skb);
 
+static inline void kfree_skb_list(struct sk_buff *segs)
+{
+	kfree_skb_list_reason(segs, SKB_DROP_REASON_NOT_SPECIFIED);
+}
+
 #ifdef CONFIG_TRACEPOINTS
 void consume_skb(struct sk_buff *skb);
 #else
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b32c5d782fe1..46d7dea78011 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -777,16 +777,17 @@ void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
 }
 EXPORT_SYMBOL(kfree_skb_reason);
 
-void kfree_skb_list(struct sk_buff *segs)
+void kfree_skb_list_reason(struct sk_buff *segs,
+			   enum skb_drop_reason reason)
 {
 	while (segs) {
 		struct sk_buff *next = segs->next;
 
-		kfree_skb(segs);
+		kfree_skb_reason(segs, reason);
 		segs = next;
 	}
 }
-EXPORT_SYMBOL(kfree_skb_list);
+EXPORT_SYMBOL(kfree_skb_list_reason);
 
 /* Dump skb information and contents.
  *
-- 
2.35.1

[PATCH net-next 3/7] net: dev: add skb drop reasons to __dev_xmit_skb()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Add reasons for skb drops to __dev_xmit_skb() by replacing
kfree_skb_list() with kfree_skb_list_reason(). The drop reason of
SKB_DROP_REASON_QDISC_DROP is introduced for qdisc enqueue fails.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 4 ++++
 include/trace/events/skb.h | 1 +
 net/core/dev.c             | 5 +++--
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index e344603aecc4..62f9d15ec6ec 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -398,6 +398,10 @@ enum skb_drop_reason {
 					 * failed (mapbe an eBPF program
 					 * is tricking?)
 					 */
+	SKB_DROP_REASON_QDISC_DROP,	/* dropped by qdisc when packet
+					 * outputting (failed to enqueue to
+					 * current qdisc)
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 3cadf13448c6..80fe15d175e3 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -46,6 +46,7 @@
 	EM(SKB_DROP_REASON_NEIGH_QUEUEFULL, NEIGH_QUEUEFULL)	\
 	EM(SKB_DROP_REASON_NEIGH_DEAD, NEIGH_DEAD)		\
 	EM(SKB_DROP_REASON_QDISC_EGRESS, QDISC_EGRESS)		\
+	EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/core/dev.c b/net/core/dev.c
index 828946d74a19..3280ba2502cd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3730,7 +3730,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 
 no_lock_out:
 		if (unlikely(to_free))
-			kfree_skb_list(to_free);
+			kfree_skb_list_reason(to_free,
+					      SKB_DROP_REASON_QDISC_DROP);
 		return rc;
 	}
 
@@ -3785,7 +3786,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 	}
 	spin_unlock(root_lock);
 	if (unlikely(to_free))
-		kfree_skb_list(to_free);
+		kfree_skb_list_reason(to_free, SKB_DROP_REASON_QDISC_DROP);
 	if (unlikely(contended))
 		spin_unlock(&q->busylock);
 	return rc;
-- 
2.35.1

[PATCH net-next 4/7] net: dev: use kfree_skb_reason() for enqueue_to_backlog()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in enqueue_to_backlog() with
kfree_skb_reason(). The skb rop reason SKB_DROP_REASON_CPU_BACKLOG is
introduced for the case of failing to enqueue the skb to the per CPU
backlog queue. The further reason can be backlog queue full or RPS
flow limition, and I think we meedn't to make further distinctions.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 6 ++++++
 include/trace/events/skb.h | 1 +
 net/core/dev.c             | 6 +++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 62f9d15ec6ec..d2cf87ff84c2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -402,6 +402,12 @@ enum skb_drop_reason {
 					 * outputting (failed to enqueue to
 					 * current qdisc)
 					 */
+	SKB_DROP_REASON_CPU_BACKLOG,	/* failed to enqueue the skb to
+					 * the per CPU backlog queue. This
+					 * can be caused by backlog queue
+					 * full (see netdev_max_backlog in
+					 * net.rst) or RPS flow limit
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 80fe15d175e3..29c360b5e114 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -47,6 +47,7 @@
 	EM(SKB_DROP_REASON_NEIGH_DEAD, NEIGH_DEAD)		\
 	EM(SKB_DROP_REASON_QDISC_EGRESS, QDISC_EGRESS)		\
 	EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)		\
+	EM(SKB_DROP_REASON_CPU_BACKLOG, CPU_BACKLOG)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/core/dev.c b/net/core/dev.c
index 3280ba2502cd..373fa7a33ffa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4541,10 +4541,12 @@ static bool skb_flow_limit(struct sk_buff *skb, unsigned int qlen)
 static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
 			      unsigned int *qtail)
 {
+	enum skb_drop_reason reason;
 	struct softnet_data *sd;
 	unsigned long flags;
 	unsigned int qlen;
 
+	reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	sd = &per_cpu(softnet_data, cpu);
 
 	rps_lock_irqsave(sd, &flags);
@@ -4566,6 +4568,8 @@ static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
 		if (!__test_and_set_bit(NAPI_STATE_SCHED, &sd->backlog.state))
 			napi_schedule_rps(sd);
 		goto enqueue;
+	} else {
+		reason = SKB_DROP_REASON_CPU_BACKLOG;
 	}
 
 drop:
@@ -4573,7 +4577,7 @@ static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
 	rps_unlock_irq_restore(sd, &flags);
 
 	atomic_long_inc(&skb->dev->rx_dropped);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, reason);
 	return NET_RX_DROP;
 }
 
-- 
2.35.1

[PATCH net-next 5/7] net: dev: use kfree_skb_reason() for do_xdp_generic()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in do_xdp_generic() with kfree_skb_reason().
The drop reason SKB_DROP_REASON_XDP is introduced for this case.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 1 +
 include/trace/events/skb.h | 1 +
 net/core/dev.c             | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d2cf87ff84c2..23bbfcd6668b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -408,6 +408,7 @@ enum skb_drop_reason {
 					 * full (see netdev_max_backlog in
 					 * net.rst) or RPS flow limit
 					 */
+	SKB_DROP_REASON_XDP,		/* dropped by XDP in input path */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 29c360b5e114..c117430375c0 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -48,6 +48,7 @@
 	EM(SKB_DROP_REASON_QDISC_EGRESS, QDISC_EGRESS)		\
 	EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)		\
 	EM(SKB_DROP_REASON_CPU_BACKLOG, CPU_BACKLOG)		\
+	EM(SKB_DROP_REASON_XDP, XDP)				\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/core/dev.c b/net/core/dev.c
index 373fa7a33ffa..ae85e024c7b7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4797,7 +4797,7 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 	}
 	return XDP_PASS;
 out_redir:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, SKB_DROP_REASON_XDP);
 	return XDP_DROP;
 }
 EXPORT_SYMBOL_GPL(do_xdp_generic);
-- 
2.35.1

[PATCH net-next 6/7] net: dev: use kfree_skb_reason() for sch_handle_ingress()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in sch_handle_ingress() with
kfree_skb_reason(). Following drop reasons are introduced:

SKB_DROP_REASON_QDISC_INGRESS

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 4 ++++
 include/trace/events/skb.h | 1 +
 net/core/dev.c             | 2 +-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 23bbfcd6668b..04508d15152e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -409,6 +409,10 @@ enum skb_drop_reason {
 					 * net.rst) or RPS flow limit
 					 */
 	SKB_DROP_REASON_XDP,		/* dropped by XDP in input path */
+	SKB_DROP_REASON_QDISC_INGRESS,	/* qdisc of type ingress check
+					 * failed (maybe an eBPF program
+					 * is tricking?)
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index c117430375c0..d635951c9db8 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -49,6 +49,7 @@
 	EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)		\
 	EM(SKB_DROP_REASON_CPU_BACKLOG, CPU_BACKLOG)		\
 	EM(SKB_DROP_REASON_XDP, XDP)				\
+	EM(SKB_DROP_REASON_QDISC_INGRESS, QDISC_INGRESS)	\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/core/dev.c b/net/core/dev.c
index ae85e024c7b7..429ad8265e8c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5013,7 +5013,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 		break;
 	case TC_ACT_SHOT:
 		mini_qdisc_qstats_cpu_drop(miniq);
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_INGRESS);
 		return NULL;
 	case TC_ACT_STOLEN:
 	case TC_ACT_QUEUED:
-- 
2.35.1

[PATCH net-next 7/7] net: dev: use kfree_skb_reason() for __netif_receive_skb_core()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Add reason for skb drops to __netif_receive_skb_core() when packet_type
not found to handle the skb. For this purpose, the drop reason
SKB_DROP_REASON_PTYPE_ABSENT is introduced. Take ether packets for
example, this case mainly happens when L3 protocol is not supported.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h | 5 +++++
 net/core/dev.c         | 8 +++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 04508d15152e..f3945d21cecf 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -413,6 +413,11 @@ enum skb_drop_reason {
 					 * failed (maybe an eBPF program
 					 * is tricking?)
 					 */
+	SKB_DROP_REASON_PTYPE_ABSENT,	/* not packet_type found to handle
+					 * the skb. For an etner packet,
+					 * this means that L3 protocol is
+					 * not supported
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 429ad8265e8c..0bdea7d113f5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5330,11 +5330,13 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
 		*ppt_prev = pt_prev;
 	} else {
 drop:
-		if (!deliver_exact)
+		if (!deliver_exact) {
 			atomic_long_inc(&skb->dev->rx_dropped);
-		else
+			kfree_skb_reason(skb, SKB_DROP_REASON_PTYPE_ABSENT);
+		} else {
 			atomic_long_inc(&skb->dev->rx_nohandler);
-		kfree_skb(skb);
+			kfree_skb(skb);
+		}
 		/* Jamal, now you will not able to escape explaining
 		 * me how you were going to use this. :-)
 		 */
-- 
2.35.1

Re: [PATCH net-next 4/7] net: dev: use kfree_skb_reason() for enqueue_to_backlog()

[Eric Dumazet]

On Thu, Mar 3, 2022 at 9:48 AM <menglong8.dong@gmail.com> wrote:
>
> From: Menglong Dong <imagedong@tencent.com>
>
> Replace kfree_skb() used in enqueue_to_backlog() with
> kfree_skb_reason(). The skb rop reason SKB_DROP_REASON_CPU_BACKLOG is
> introduced for the case of failing to enqueue the skb to the per CPU
> backlog queue. The further reason can be backlog queue full or RPS
> flow limition, and I think we meedn't to make further distinctions.
>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  include/linux/skbuff.h     | 6 ++++++
>  include/trace/events/skb.h | 1 +
>  net/core/dev.c             | 6 +++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 62f9d15ec6ec..d2cf87ff84c2 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -402,6 +402,12 @@ enum skb_drop_reason {
>                                          * outputting (failed to enqueue to
>                                          * current qdisc)
>                                          */
> +       SKB_DROP_REASON_CPU_BACKLOG,    /* failed to enqueue the skb to
> +                                        * the per CPU backlog queue. This
> +                                        * can be caused by backlog queue
> +                                        * full (see netdev_max_backlog in
> +                                        * net.rst) or RPS flow limit
> +                                        */
>         SKB_DROP_REASON_MAX,
>  };
>
> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> index 80fe15d175e3..29c360b5e114 100644
> --- a/include/trace/events/skb.h
> +++ b/include/trace/events/skb.h
> @@ -47,6 +47,7 @@
>         EM(SKB_DROP_REASON_NEIGH_DEAD, NEIGH_DEAD)              \
>         EM(SKB_DROP_REASON_QDISC_EGRESS, QDISC_EGRESS)          \
>         EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)              \
> +       EM(SKB_DROP_REASON_CPU_BACKLOG, CPU_BACKLOG)            \
>         EMe(SKB_DROP_REASON_MAX, MAX)
>
>  #undef EM
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3280ba2502cd..373fa7a33ffa 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4541,10 +4541,12 @@ static bool skb_flow_limit(struct sk_buff *skb, unsigned int qlen)
>  static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
>                               unsigned int *qtail)
>  {
> +       enum skb_drop_reason reason;
>         struct softnet_data *sd;
>         unsigned long flags;
>         unsigned int qlen;
>
> +       reason = SKB_DROP_REASON_NOT_SPECIFIED;
>         sd = &per_cpu(softnet_data, cpu);
>
>         rps_lock_irqsave(sd, &flags);
> @@ -4566,6 +4568,8 @@ static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
>                 if (!__test_and_set_bit(NAPI_STATE_SCHED, &sd->backlog.state))
>                         napi_schedule_rps(sd);
>                 goto enqueue;
> +       } else {

No need for an else {} after a goto  xxx;


> +               reason = SKB_DROP_REASON_CPU_BACKLOG;
>         }
>
>  drop:
> @@ -4573,7 +4577,7 @@ static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
>         rps_unlock_irq_restore(sd, &flags);
>
>         atomic_long_inc(&skb->dev->rx_dropped);
> -       kfree_skb(skb);
> +       kfree_skb_reason(skb, reason);
>         return NET_RX_DROP;
>  }
>
> --
> 2.35.1
>

Re: [PATCH net-next 2/7] net: skb: introduce the function kfree_skb_list_reason()

[Dongli Zhang]

Hi Menglong,

I am going to send another v5 soon which introduces kfree_skb_list_reason() as well.

https://lore.kernel.org/all/20220226084929.6417-2-dongli.zhang@oracle.com/

I will need to make it inline as suggested by Jakub.

Not sure how to handle such scenario :)

Thank you very much!

Dongli Zhang

On 3/3/22 9:47 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> To report reasons of skb drops, introduce the function
> kfree_skb_list_reason() and make kfree_skb_list() an inline call to
> it. This function will be used in the next commit in
> __dev_xmit_skb().
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  include/linux/skbuff.h | 8 +++++++-
>  net/core/skbuff.c      | 7 ++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ebd18850b63e..e344603aecc4 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1194,10 +1194,16 @@ static inline void kfree_skb(struct sk_buff *skb)
>  }
>  
>  void skb_release_head_state(struct sk_buff *skb);
> -void kfree_skb_list(struct sk_buff *segs);
> +void kfree_skb_list_reason(struct sk_buff *segs,
> +			   enum skb_drop_reason reason);
>  void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt);
>  void skb_tx_error(struct sk_buff *skb);
>  
> +static inline void kfree_skb_list(struct sk_buff *segs)
> +{
> +	kfree_skb_list_reason(segs, SKB_DROP_REASON_NOT_SPECIFIED);
> +}
> +
>  #ifdef CONFIG_TRACEPOINTS
>  void consume_skb(struct sk_buff *skb);
>  #else
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b32c5d782fe1..46d7dea78011 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -777,16 +777,17 @@ void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
>  }
>  EXPORT_SYMBOL(kfree_skb_reason);
>  
> -void kfree_skb_list(struct sk_buff *segs)
> +void kfree_skb_list_reason(struct sk_buff *segs,
> +			   enum skb_drop_reason reason)
>  {
>  	while (segs) {
>  		struct sk_buff *next = segs->next;
>  
> -		kfree_skb(segs);
> +		kfree_skb_reason(segs, reason);
>  		segs = next;
>  	}
>  }
> -EXPORT_SYMBOL(kfree_skb_list);
> +EXPORT_SYMBOL(kfree_skb_list_reason);
>  
>  /* Dump skb information and contents.
>   *
> 

Re: [PATCH net-next 2/7] net: skb: introduce the function kfree_skb_list_reason()

[Jakub Kicinski]

On Thu, 3 Mar 2022 16:12:05 -0800 Dongli Zhang wrote:
> I am going to send another v5 soon which introduces kfree_skb_list_reason() as well.
> 
> https://lore.kernel.org/all/20220226084929.6417-2-dongli.zhang@oracle.com/
> 
> I will need to make it inline as suggested by Jakub.
> 
> Not sure how to handle such scenario :)

Probably nothing we can do about that :( I'd say proceed as if the
other series didn't exist, and after one series is merged the author
of the other one will have to rebase.

Re: [PATCH net-next 1/7] net: dev: use kfree_skb_reason() for sch_handle_egress()

[Jakub Kicinski]

On Fri,  4 Mar 2022 01:47:01 +0800 menglong8.dong@gmail.com wrote:
> Replace kfree_skb() used in sch_handle_egress() with kfree_skb_reason().
> The drop reason SKB_DROP_REASON_QDISC_EGRESS is introduced. Considering
> the code path of qdisc egerss, we make it distinct with the drop reason
> of SKB_DROP_REASON_QDISC_DROP in the next commit.

I don't think this has much to do with Qdiscs, this is the TC
egress hook, it's more for filtering. Classful Qdisc like HTB 
will run its own classification. I think.

Maybe TC_EGRESS?

Re: [PATCH net-next 4/7] net: dev: use kfree_skb_reason() for enqueue_to_backlog()

[Menglong Dong]

On Fri, Mar 4, 2022 at 1:59 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Mar 3, 2022 at 9:48 AM <menglong8.dong@gmail.com> wrote:
> >
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Replace kfree_skb() used in enqueue_to_backlog() with
> > kfree_skb_reason(). The skb rop reason SKB_DROP_REASON_CPU_BACKLOG is
> > introduced for the case of failing to enqueue the skb to the per CPU
> > backlog queue. The further reason can be backlog queue full or RPS
> > flow limition, and I think we meedn't to make further distinctions.
> >
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  include/linux/skbuff.h     | 6 ++++++
> >  include/trace/events/skb.h | 1 +
> >  net/core/dev.c             | 6 +++++-
> >  3 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 62f9d15ec6ec..d2cf87ff84c2 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -402,6 +402,12 @@ enum skb_drop_reason {
> >                                          * outputting (failed to enqueue to
> >                                          * current qdisc)
> >                                          */
> > +       SKB_DROP_REASON_CPU_BACKLOG,    /* failed to enqueue the skb to
> > +                                        * the per CPU backlog queue. This
> > +                                        * can be caused by backlog queue
> > +                                        * full (see netdev_max_backlog in
> > +                                        * net.rst) or RPS flow limit
> > +                                        */
> >         SKB_DROP_REASON_MAX,
> >  };
> >
> > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> > index 80fe15d175e3..29c360b5e114 100644
> > --- a/include/trace/events/skb.h
> > +++ b/include/trace/events/skb.h
> > @@ -47,6 +47,7 @@
> >         EM(SKB_DROP_REASON_NEIGH_DEAD, NEIGH_DEAD)              \
> >         EM(SKB_DROP_REASON_QDISC_EGRESS, QDISC_EGRESS)          \
> >         EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)              \
> > +       EM(SKB_DROP_REASON_CPU_BACKLOG, CPU_BACKLOG)            \
> >         EMe(SKB_DROP_REASON_MAX, MAX)
> >
> >  #undef EM
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3280ba2502cd..373fa7a33ffa 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4541,10 +4541,12 @@ static bool skb_flow_limit(struct sk_buff *skb, unsigned int qlen)
> >  static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
> >                               unsigned int *qtail)
> >  {
> > +       enum skb_drop_reason reason;
> >         struct softnet_data *sd;
> >         unsigned long flags;
> >         unsigned int qlen;
> >
> > +       reason = SKB_DROP_REASON_NOT_SPECIFIED;
> >         sd = &per_cpu(softnet_data, cpu);
> >
> >         rps_lock_irqsave(sd, &flags);
> > @@ -4566,6 +4568,8 @@ static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
> >                 if (!__test_and_set_bit(NAPI_STATE_SCHED, &sd->backlog.state))
> >                         napi_schedule_rps(sd);
> >                 goto enqueue;
> > +       } else {
>
> No need for an else {} after a goto  xxx;
>

Yeah, this 'else' can be omitted :) Thanks!

>
> > +               reason = SKB_DROP_REASON_CPU_BACKLOG;
> >         }
> >
> >  drop:
> > @@ -4573,7 +4577,7 @@ static int enqueue_to_backlog(struct sk_buff *skb, int cpu,
> >         rps_unlock_irq_restore(sd, &flags);
> >
> >         atomic_long_inc(&skb->dev->rx_dropped);
> > -       kfree_skb(skb);
> > +       kfree_skb_reason(skb, reason);
> >         return NET_RX_DROP;
> >  }
> >
> > --
> > 2.35.1
> >

Re: [PATCH net-next 1/7] net: dev: use kfree_skb_reason() for sch_handle_egress()

[Menglong Dong]

On Fri, Mar 4, 2022 at 12:25 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri,  4 Mar 2022 01:47:01 +0800 menglong8.dong@gmail.com wrote:
> > Replace kfree_skb() used in sch_handle_egress() with kfree_skb_reason().
> > The drop reason SKB_DROP_REASON_QDISC_EGRESS is introduced. Considering
> > the code path of qdisc egerss, we make it distinct with the drop reason
> > of SKB_DROP_REASON_QDISC_DROP in the next commit.
>
> I don't think this has much to do with Qdiscs, this is the TC
> egress hook, it's more for filtering. Classful Qdisc like HTB
> will run its own classification. I think.
>
> Maybe TC_EGRESS?

You are right, I think I misunderstanded the concept of qdisc and tc before.
and seems all 'QDISC' here should be 'TC'? which means:

QDISC_EGRESS -> TC_EGRESS
QDISC_DROP -> TC_DROP
QDISC_INGRESS -> TC_INGRESS

Re: [PATCH net-next 1/7] net: dev: use kfree_skb_reason() for sch_handle_egress()

[Jakub Kicinski]

On Fri, 4 Mar 2022 12:56:40 +0800 Menglong Dong wrote:
> You are right, I think I misunderstanded the concept of qdisc and tc before.
> and seems all 'QDISC' here should be 'TC'? which means:
> 
> QDISC_EGRESS -> TC_EGRESS
> QDISC_DROP -> TC_DROP

For this one QDISC is good, I think, it will mostly catch packets 
which went thru qdisc_drop(), right?

> QDISC_INGRESS -> TC_INGRESS

Re: [PATCH net-next 1/7] net: dev: use kfree_skb_reason() for sch_handle_egress()

[Menglong Dong]

On Fri, Mar 4, 2022 at 1:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 4 Mar 2022 12:56:40 +0800 Menglong Dong wrote:
> > You are right, I think I misunderstanded the concept of qdisc and tc before.
> > and seems all 'QDISC' here should be 'TC'? which means:
> >
> > QDISC_EGRESS -> TC_EGRESS
> > QDISC_DROP -> TC_DROP
>
> For this one QDISC is good, I think, it will mostly catch packets
> which went thru qdisc_drop(), right?
>

Yeah, you are right.

> > QDISC_INGRESS -> TC_INGRESS