[PATCH net-next 0/3] net: use kfree_skb_reason() for ip/neighbour

[PATCH net-next 0/3] net: use kfree_skb_reason() for ip/neighbour

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

In the series "net: use kfree_skb_reason() for ip/udp packet receive",
reasons for skb drops are added to the packet receive process of IP
layer. Link:

https://lore.kernel.org/netdev/20220205074739.543606-1-imagedong@tencent.com/

And in the first patch of this series, skb drop reasons are added to
the packet egress path of IP layer. As kfree_skb() is not used frequent,
I commit these changes at once and didn't create a patch for every
functions that involed. Following functions are handled:

__ip_queue_xmit()
ip_finish_output()
ip_mc_finish_output()
ip6_output()
ip6_finish_output()
ip6_finish_output2()

Following new drop reasons are introduced (what they mean can be seen
in the document of them):

SKB_DROP_REASON_IP_OUTNOROUTES
SKB_DROP_REASON_BPF_CGROUP_EGRESS
SKB_DROP_REASON_IPV6DSIABLED

In the 2th and 3th patches, kfree_skb_reason() is used in neighbour
subsystem instead of kfree_skb(). __neigh_event_send() and
arp_error_report() are involed, and following new drop reasons are
introduced:

SKB_DROP_REASON_NEIGH_FAILED
SKB_DROP_REASON_NEIGH_QUEUEFULL


Menglong Dong (3):
  net: ip: add skb drop reasons for ip egress path
  net: neigh: use kfree_skb_reason() for __neigh_event_send()
  net: neigh: add skb drop reasons to arp_error_report()

 include/linux/skbuff.h     | 22 ++++++++++++++++++++++
 include/trace/events/skb.h |  6 ++++++
 net/core/neighbour.c       |  4 ++--
 net/ipv4/arp.c             |  2 +-
 net/ipv4/ip_output.c       |  6 +++---
 net/ipv6/ip6_output.c      |  6 +++---
 6 files changed, 37 insertions(+), 9 deletions(-)

-- 
2.35.1

[PATCH net-next 1/3] net: ip: add skb drop reasons for ip egress path

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in the packet egress path of
IP layer (both IPv4 and IPv6 are considered).

Following functions are involved:

__ip_queue_xmit()
ip_finish_output()
ip_mc_finish_output()
ip6_output()
ip6_finish_output()
ip6_finish_output2()

Following new drop reasons are introduced:

SKB_DROP_REASON_IP_OUTNOROUTES
SKB_DROP_REASON_BPF_CGROUP_EGRESS
SKB_DROP_REASON_IPV6DSIABLED

Reviewed-by: Mengen Sun <mengensun@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 13 +++++++++++++
 include/trace/events/skb.h |  4 ++++
 net/ipv4/ip_output.c       |  6 +++---
 net/ipv6/ip6_output.c      |  6 +++---
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a3e90efe6586..c310a4a8fc86 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -380,6 +380,19 @@ enum skb_drop_reason {
 					 * the ofo queue, corresponding to
 					 * LINUX_MIB_TCPOFOMERGE
 					 */
+	SKB_DROP_REASON_IP_OUTNOROUTES,	/* route lookup failed during
+					 * packet outputting
+					 */
+	SKB_DROP_REASON_BPF_CGROUP_EGRESS,	/* dropped by eBPF program
+						 * with type of BPF_PROG_TYPE_CGROUP_SKB
+						 * and attach type of
+						 * BPF_CGROUP_INET_EGRESS
+						 * during packet sending
+						 */
+	SKB_DROP_REASON_IPV6DSIABLED,	/* IPv6 is disabled on the device,
+					 * see the doc for disable_ipv6
+					 * in ip-sysctl.rst for detail
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 2ab7193313aa..47dedef7b6b8 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -37,6 +37,10 @@
 	EM(SKB_DROP_REASON_TCP_OLD_DATA, TCP_OLD_DATA)		\
 	EM(SKB_DROP_REASON_TCP_OVERWINDOW, TCP_OVERWINDOW)	\
 	EM(SKB_DROP_REASON_TCP_OFOMERGE, TCP_OFOMERGE)		\
+	EM(SKB_DROP_REASON_IP_OUTNOROUTES, IP_OUTNOROUTES)	\
+	EM(SKB_DROP_REASON_BPF_CGROUP_EGRESS,			\
+	   BPF_CGROUP_EGRESS)					\
+	EM(SKB_DROP_REASON_IPV6DSIABLED, IPV6DSIABLED)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 0c0574eb5f5b..df549b7415fb 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -317,7 +317,7 @@ static int ip_finish_output(struct net *net, struct sock *sk, struct sk_buff *sk
 	case NET_XMIT_CN:
 		return __ip_finish_output(net, sk, skb) ? : ret;
 	default:
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_BPF_CGROUP_EGRESS);
 		return ret;
 	}
 }
@@ -337,7 +337,7 @@ static int ip_mc_finish_output(struct net *net, struct sock *sk,
 	case NET_XMIT_SUCCESS:
 		break;
 	default:
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_BPF_CGROUP_EGRESS);
 		return ret;
 	}
 
@@ -536,7 +536,7 @@ int __ip_queue_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl,
 no_route:
 	rcu_read_unlock();
 	IP_INC_STATS(net, IPSTATS_MIB_OUTNOROUTES);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, SKB_DROP_REASON_IP_OUTNOROUTES);
 	return -EHOSTUNREACH;
 }
 EXPORT_SYMBOL(__ip_queue_xmit);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 0c6c971ce0a5..4cd9e5fd25e4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -130,7 +130,7 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
 	rcu_read_unlock_bh();
 
 	IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTNOROUTES);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, SKB_DROP_REASON_IP_OUTNOROUTES);
 	return -EINVAL;
 }
 
@@ -202,7 +202,7 @@ static int ip6_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
 	case NET_XMIT_CN:
 		return __ip6_finish_output(net, sk, skb) ? : ret;
 	default:
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_BPF_CGROUP_EGRESS);
 		return ret;
 	}
 }
@@ -217,7 +217,7 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 	if (unlikely(idev->cnf.disable_ipv6)) {
 		IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTDISCARDS);
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_IPV6DSIABLED);
 		return 0;
 	}
 
-- 
2.35.1

[PATCH net-next 2/3] net: neigh: use kfree_skb_reason() for __neigh_event_send()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in __neigh_event_send() with
kfree_skb_reason(). Following drop reasons are added:

SKB_DROP_REASON_NEIGH_FAILED
SKB_DROP_REASON_NEIGH_QUEUEFULL

The two reasons above should be the hot path that skb drops in
neighbour subsystem.

Reviewed-by: Mengen Sun <mengensun@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 9 +++++++++
 include/trace/events/skb.h | 2 ++
 net/core/neighbour.c       | 4 ++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c310a4a8fc86..206b66f5ce6b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -393,6 +393,15 @@ enum skb_drop_reason {
 					 * see the doc for disable_ipv6
 					 * in ip-sysctl.rst for detail
 					 */
+	SKB_DROP_REASON_NEIGH_FAILED,	/* dropped as the state of
+					 * neighbour is NUD_FAILED
+					 */
+	SKB_DROP_REASON_NEIGH_QUEUEFULL,	/* the skbs that waiting
+						 * for sending on the queue
+						 * of neigh->arp_queue is
+						 * full, and the skbs on the
+						 * tail will be dropped
+						 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 47dedef7b6b8..dd06366ded4a 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -41,6 +41,8 @@
 	EM(SKB_DROP_REASON_BPF_CGROUP_EGRESS,			\
 	   BPF_CGROUP_EGRESS)					\
 	EM(SKB_DROP_REASON_IPV6DSIABLED, IPV6DSIABLED)		\
+	EM(SKB_DROP_REASON_NEIGH_FAILED, NEIGH_FAILED)		\
+	EM(SKB_DROP_REASON_NEIGH_QUEUEFULL, NEIGH_QUEUEFULL)	\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index ec0bf737b076..c353834e8fa9 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1171,7 +1171,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb,
 			neigh->updated = jiffies;
 			write_unlock_bh(&neigh->lock);
 
-			kfree_skb(skb);
+			kfree_skb_reason(skb, SKB_DROP_REASON_NEIGH_FAILED);
 			return 1;
 		}
 	} else if (neigh->nud_state & NUD_STALE) {
@@ -1193,7 +1193,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb,
 				if (!buff)
 					break;
 				neigh->arp_queue_len_bytes -= buff->truesize;
-				kfree_skb(buff);
+				kfree_skb_reason(buff, SKB_DROP_REASON_NEIGH_QUEUEFULL);
 				NEIGH_CACHE_STAT_INC(neigh->tbl, unres_discards);
 			}
 			skb_dst_force(skb);
-- 
2.35.1

[PATCH net-next 3/3] net: neigh: add skb drop reasons to arp_error_report()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

When neighbour become invalid or destroyed, neigh_invalidate() will be
called. neigh->ops->error_report() will be called if the neighbour's
state is NUD_FAILED, and seems here is the only use of error_report().
So we can tell that the reason of skb drops in arp_error_report() is
SKB_DROP_REASON_NEIGH_FAILED.

Replace kfree_skb() used in arp_error_report() with kfree_skb_reason().

Reviewed-by: Mengen Sun <mengensun@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/ipv4/arp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 4db0325f6e1a..8e4ca4738c43 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -293,7 +293,7 @@ static int arp_constructor(struct neighbour *neigh)
 static void arp_error_report(struct neighbour *neigh, struct sk_buff *skb)
 {
 	dst_link_failure(skb);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, SKB_DROP_REASON_NEIGH_FAILED);
 }
 
 /* Create and send an arp packet. */
-- 
2.35.1

Re: [PATCH net-next 1/3] net: ip: add skb drop reasons for ip egress path

[David Ahern]

On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Replace kfree_skb() with kfree_skb_reason() in the packet egress path of
> IP layer (both IPv4 and IPv6 are considered).
> 
> Following functions are involved:
> 
> __ip_queue_xmit()
> ip_finish_output()
> ip_mc_finish_output()
> ip6_output()
> ip6_finish_output()
> ip6_finish_output2()
> 
> Following new drop reasons are introduced:
> 
> SKB_DROP_REASON_IP_OUTNOROUTES
> SKB_DROP_REASON_BPF_CGROUP_EGRESS
> SKB_DROP_REASON_IPV6DSIABLED
> 
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Reviewed-by: Hao Peng <flyingpeng@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  include/linux/skbuff.h     | 13 +++++++++++++
>  include/trace/events/skb.h |  4 ++++
>  net/ipv4/ip_output.c       |  6 +++---
>  net/ipv6/ip6_output.c      |  6 +++---
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a3e90efe6586..c310a4a8fc86 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -380,6 +380,19 @@ enum skb_drop_reason {
>  					 * the ofo queue, corresponding to
>  					 * LINUX_MIB_TCPOFOMERGE
>  					 */
> +	SKB_DROP_REASON_IP_OUTNOROUTES,	/* route lookup failed during
> +					 * packet outputting
> +					 */

This should be good enough since the name contains OUT.

/* route lookup failed */

> +	SKB_DROP_REASON_BPF_CGROUP_EGRESS,	/* dropped by eBPF program
> +						 * with type of BPF_PROG_TYPE_CGROUP_SKB
> +						 * and attach type of
> +						 * BPF_CGROUP_INET_EGRESS
> +						 * during packet sending
> +						 */

/* dropped by BPF_CGROUP_INET_EGRESS eBPF program */

> +	SKB_DROP_REASON_IPV6DSIABLED,	/* IPv6 is disabled on the device,
> +					 * see the doc for disable_ipv6
> +					 * in ip-sysctl.rst for detail
> +					 */

Just /* IPv6 is disabled on the device */


>  	SKB_DROP_REASON_MAX,
>  };
>  

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 0c0574eb5f5b..df549b7415fb 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c

This file has other relevant drops. e.g., ip_finish_output2 when a neigh
entry can not be created and after skb_gso_segment. The other set for
tun/tap devices has SKB_DROP_REASON_SKB_GSO_SEG which can be used for
the latter. That set also adds kfree_skb_list_reason for the frag drops.


> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 0c6c971ce0a5..4cd9e5fd25e4 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c

Similarly here. The other set should land in the next few days, so you
cna put this set on top of it.

Re: [PATCH net-next 2/3] net: neigh: use kfree_skb_reason() for __neigh_event_send()

[David Ahern]

On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index c310a4a8fc86..206b66f5ce6b 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -393,6 +393,15 @@ enum skb_drop_reason {
>  					 * see the doc for disable_ipv6
>  					 * in ip-sysctl.rst for detail
>  					 */
> +	SKB_DROP_REASON_NEIGH_FAILED,	/* dropped as the state of
> +					 * neighbour is NUD_FAILED
> +					 */

/* neigh entry in failed state */

> +	SKB_DROP_REASON_NEIGH_QUEUEFULL,	/* the skbs that waiting
> +						 * for sending on the queue
> +						 * of neigh->arp_queue is
> +						 * full, and the skbs on the
> +						 * tail will be dropped
> +						 */

/* arp_queue for neigh entry is full */


> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index ec0bf737b076..c353834e8fa9 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1171,7 +1171,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb,
>  			neigh->updated = jiffies;
>  			write_unlock_bh(&neigh->lock);
>  
> -			kfree_skb(skb);
> +			kfree_skb_reason(skb, SKB_DROP_REASON_NEIGH_FAILED);
>  			return 1;
>  		}
>  	} else if (neigh->nud_state & NUD_STALE) {
> @@ -1193,7 +1193,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb,
>  				if (!buff)
>  					break;
>  				neigh->arp_queue_len_bytes -= buff->truesize;
> -				kfree_skb(buff);
> +				kfree_skb_reason(buff, SKB_DROP_REASON_NEIGH_QUEUEFULL);
>  				NEIGH_CACHE_STAT_INC(neigh->tbl, unres_discards);
>  			}
>  			skb_dst_force(skb);

what about out_dead: path? the tracepoint there shows that path is of
interest.

Re: [PATCH net-next 3/3] net: neigh: add skb drop reasons to arp_error_report()

[David Ahern]

On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> When neighbour become invalid or destroyed, neigh_invalidate() will be
> called. neigh->ops->error_report() will be called if the neighbour's
> state is NUD_FAILED, and seems here is the only use of error_report().
> So we can tell that the reason of skb drops in arp_error_report() is
> SKB_DROP_REASON_NEIGH_FAILED.
> 
> Replace kfree_skb() used in arp_error_report() with kfree_skb_reason().
> 
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Reviewed-by: Hao Peng <flyingpeng@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  net/ipv4/arp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 1/3] net: ip: add skb drop reasons for ip egress path

[Menglong Dong]

On Tue, Feb 22, 2022 at 11:13 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Replace kfree_skb() with kfree_skb_reason() in the packet egress path of
> > IP layer (both IPv4 and IPv6 are considered).
> >
> > Following functions are involved:
> >
> > __ip_queue_xmit()
> > ip_finish_output()
> > ip_mc_finish_output()
> > ip6_output()
> > ip6_finish_output()
> > ip6_finish_output2()
> >
> > Following new drop reasons are introduced:
> >
> > SKB_DROP_REASON_IP_OUTNOROUTES
> > SKB_DROP_REASON_BPF_CGROUP_EGRESS
> > SKB_DROP_REASON_IPV6DSIABLED
> >
> > Reviewed-by: Mengen Sun <mengensun@tencent.com>
> > Reviewed-by: Hao Peng <flyingpeng@tencent.com>
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  include/linux/skbuff.h     | 13 +++++++++++++
> >  include/trace/events/skb.h |  4 ++++
> >  net/ipv4/ip_output.c       |  6 +++---
> >  net/ipv6/ip6_output.c      |  6 +++---
> >  4 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index a3e90efe6586..c310a4a8fc86 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -380,6 +380,19 @@ enum skb_drop_reason {
> >                                        * the ofo queue, corresponding to
> >                                        * LINUX_MIB_TCPOFOMERGE
> >                                        */
> > +     SKB_DROP_REASON_IP_OUTNOROUTES, /* route lookup failed during
> > +                                      * packet outputting
> > +                                      */
>
> This should be good enough since the name contains OUT.
>
> /* route lookup failed */
>
> > +     SKB_DROP_REASON_BPF_CGROUP_EGRESS,      /* dropped by eBPF program
> > +                                              * with type of BPF_PROG_TYPE_CGROUP_SKB
> > +                                              * and attach type of
> > +                                              * BPF_CGROUP_INET_EGRESS
> > +                                              * during packet sending
> > +                                              */
>
> /* dropped by BPF_CGROUP_INET_EGRESS eBPF program */
>
> > +     SKB_DROP_REASON_IPV6DSIABLED,   /* IPv6 is disabled on the device,
> > +                                      * see the doc for disable_ipv6
> > +                                      * in ip-sysctl.rst for detail
> > +                                      */
>
> Just /* IPv6 is disabled on the device */
>
>
> >       SKB_DROP_REASON_MAX,
> >  };
> >
>
> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > index 0c0574eb5f5b..df549b7415fb 100644
> > --- a/net/ipv4/ip_output.c
> > +++ b/net/ipv4/ip_output.c
>
> This file has other relevant drops. e.g., ip_finish_output2 when a neigh
> entry can not be created and after skb_gso_segment. The other set for
> tun/tap devices has SKB_DROP_REASON_SKB_GSO_SEG which can be used for
> the latter. That set also adds kfree_skb_list_reason for the frag drops.
>

I tried to add a drop reason for neigh creating fail, but I found it's hard
to find the root reason, as __neigh_create() can fail in many cases.
And I'm not sure if there is any help when we get a
'SKB_DROP_REASON_NEIGH_CREATEFAIL' message.

Seems it's hard to make every drop reason accurate, is it ok if we
use the name 'SKB_DROP_REASON_NEIGH_CREATEFAIL' for
this path?

>
> > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > index 0c6c971ce0a5..4cd9e5fd25e4 100644
> > --- a/net/ipv6/ip6_output.c
> > +++ b/net/ipv6/ip6_output.c
>
> Similarly here. The other set should land in the next few days, so you
> cna put this set on top of it.

Yeah, I can make use of it.

>

Re: [PATCH net-next 2/3] net: neigh: use kfree_skb_reason() for __neigh_event_send()

[Menglong Dong]

On Tue, Feb 22, 2022 at 11:17 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index c310a4a8fc86..206b66f5ce6b 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -393,6 +393,15 @@ enum skb_drop_reason {
> >                                        * see the doc for disable_ipv6
> >                                        * in ip-sysctl.rst for detail
> >                                        */
> > +     SKB_DROP_REASON_NEIGH_FAILED,   /* dropped as the state of
> > +                                      * neighbour is NUD_FAILED
> > +                                      */
>
> /* neigh entry in failed state */
>
> > +     SKB_DROP_REASON_NEIGH_QUEUEFULL,        /* the skbs that waiting
> > +                                              * for sending on the queue
> > +                                              * of neigh->arp_queue is
> > +                                              * full, and the skbs on the
> > +                                              * tail will be dropped
> > +                                              */
>
> /* arp_queue for neigh entry is full */
>
>
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index ec0bf737b076..c353834e8fa9 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -1171,7 +1171,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb,
> >                       neigh->updated = jiffies;
> >                       write_unlock_bh(&neigh->lock);
> >
> > -                     kfree_skb(skb);
> > +                     kfree_skb_reason(skb, SKB_DROP_REASON_NEIGH_FAILED);
> >                       return 1;
> >               }
> >       } else if (neigh->nud_state & NUD_STALE) {
> > @@ -1193,7 +1193,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb,
> >                               if (!buff)
> >                                       break;
> >                               neigh->arp_queue_len_bytes -= buff->truesize;
> > -                             kfree_skb(buff);
> > +                             kfree_skb_reason(buff, SKB_DROP_REASON_NEIGH_QUEUEFULL);
> >                               NEIGH_CACHE_STAT_INC(neigh->tbl, unres_discards);
> >                       }
> >                       skb_dst_force(skb);
>
> what about out_dead: path? the tracepoint there shows that path is of
> interest.

You are right, that path should be considered too.

Re: [PATCH net-next 1/3] net: ip: add skb drop reasons for ip egress path

[Menglong Dong]

On Tue, Feb 22, 2022 at 11:13 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Replace kfree_skb() with kfree_skb_reason() in the packet egress path of
> > IP layer (both IPv4 and IPv6 are considered).
> >
> > Following functions are involved:
> >
> > __ip_queue_xmit()
> > ip_finish_output()
> > ip_mc_finish_output()
> > ip6_output()
> > ip6_finish_output()
> > ip6_finish_output2()
> >
> > Following new drop reasons are introduced:
> >
> > SKB_DROP_REASON_IP_OUTNOROUTES
> > SKB_DROP_REASON_BPF_CGROUP_EGRESS
> > SKB_DROP_REASON_IPV6DSIABLED
> >
> > Reviewed-by: Mengen Sun <mengensun@tencent.com>
> > Reviewed-by: Hao Peng <flyingpeng@tencent.com>
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  include/linux/skbuff.h     | 13 +++++++++++++
> >  include/trace/events/skb.h |  4 ++++
> >  net/ipv4/ip_output.c       |  6 +++---
> >  net/ipv6/ip6_output.c      |  6 +++---
> >  4 files changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index a3e90efe6586..c310a4a8fc86 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -380,6 +380,19 @@ enum skb_drop_reason {
> >                                        * the ofo queue, corresponding to
> >                                        * LINUX_MIB_TCPOFOMERGE
> >                                        */
> > +     SKB_DROP_REASON_IP_OUTNOROUTES, /* route lookup failed during
> > +                                      * packet outputting
> > +                                      */
>
> This should be good enough since the name contains OUT.
>
> /* route lookup failed */
>
> > +     SKB_DROP_REASON_BPF_CGROUP_EGRESS,      /* dropped by eBPF program
> > +                                              * with type of BPF_PROG_TYPE_CGROUP_SKB
> > +                                              * and attach type of
> > +                                              * BPF_CGROUP_INET_EGRESS
> > +                                              * during packet sending
> > +                                              */
>
> /* dropped by BPF_CGROUP_INET_EGRESS eBPF program */
>
> > +     SKB_DROP_REASON_IPV6DSIABLED,   /* IPv6 is disabled on the device,
> > +                                      * see the doc for disable_ipv6
> > +                                      * in ip-sysctl.rst for detail
> > +                                      */
>
> Just /* IPv6 is disabled on the device */
>
>
> >       SKB_DROP_REASON_MAX,
> >  };
> >
>
> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > index 0c0574eb5f5b..df549b7415fb 100644
> > --- a/net/ipv4/ip_output.c
> > +++ b/net/ipv4/ip_output.c
>
> This file has other relevant drops. e.g., ip_finish_output2 when a neigh
> entry can not be created and after skb_gso_segment. The other set for
> tun/tap devices has SKB_DROP_REASON_SKB_GSO_SEG which can be used for
> the latter. That set also adds kfree_skb_list_reason for the frag drops.
>
>
> > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > index 0c6c971ce0a5..4cd9e5fd25e4 100644
> > --- a/net/ipv6/ip6_output.c
> > +++ b/net/ipv6/ip6_output.c
>
> Similarly here. The other set should land in the next few days, so you
> cna put this set on top of it.

Do I need to wait for that set to be ready and use something in it?
Seems they are not ready yet, and I think maybe I can send a v2
now without it?

>

Re: [PATCH net-next 1/3] net: ip: add skb drop reasons for ip egress path

[Roman Mashak]

Menglong Dong <menglong8.dong@gmail.com> writes:

> On Tue, Feb 22, 2022 at 11:13 AM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
>> > From: Menglong Dong <imagedong@tencent.com>
>> >
>> > Replace kfree_skb() with kfree_skb_reason() in the packet egress path of
>> > IP layer (both IPv4 and IPv6 are considered).
>> >
>> > Following functions are involved:
>> >
>> > __ip_queue_xmit()
>> > ip_finish_output()
>> > ip_mc_finish_output()
>> > ip6_output()
>> > ip6_finish_output()
>> > ip6_finish_output2()
>> >
>> > Following new drop reasons are introduced:
>> >
>> > SKB_DROP_REASON_IP_OUTNOROUTES
>> > SKB_DROP_REASON_BPF_CGROUP_EGRESS
>> > SKB_DROP_REASON_IPV6DSIABLED

Is this a typo and should be SKB_DROP_REASON_IPV6DISABLED ?

[...]

Re: [PATCH net-next 1/3] net: ip: add skb drop reasons for ip egress path

[Menglong Dong]

On Fri, Feb 25, 2022 at 9:41 PM Roman Mashak <mrv@mojatatu.com> wrote:
>
> Menglong Dong <menglong8.dong@gmail.com> writes:
>
> > On Tue, Feb 22, 2022 at 11:13 AM David Ahern <dsahern@kernel.org> wrote:
> >>
> >> On 2/20/22 8:57 AM, menglong8.dong@gmail.com wrote:
> >> > From: Menglong Dong <imagedong@tencent.com>
> >> >
> >> > Replace kfree_skb() with kfree_skb_reason() in the packet egress path of
> >> > IP layer (both IPv4 and IPv6 are considered).
> >> >
> >> > Following functions are involved:
> >> >
> >> > __ip_queue_xmit()
> >> > ip_finish_output()
> >> > ip_mc_finish_output()
> >> > ip6_output()
> >> > ip6_finish_output()
> >> > ip6_finish_output2()
> >> >
> >> > Following new drop reasons are introduced:
> >> >
> >> > SKB_DROP_REASON_IP_OUTNOROUTES
> >> > SKB_DROP_REASON_BPF_CGROUP_EGRESS
> >> > SKB_DROP_REASON_IPV6DSIABLED
>
> Is this a typo and should be SKB_DROP_REASON_IPV6DISABLED ?

In fact......yeah, this is a typo :)
I'll fix it. Thanks!

>
> [...]
>