From: Menglong Dong <redacted>
In this series patches, reasons for skb drops are added to TCP, IP, dev
and neigh.
For TCP layer, the path of TCP data receive and enqueue are considered.
However, it's more complex for TCP state processing, as I find that it's
hard to report skb drop reasons to where it is freed. For example,
when skb is dropped in tcp_rcv_state_process(), the reason can be caused
by the call of tcp_v4_conn_request(), and it's hard to return a drop
reason from tcp_v4_conn_request(). So I just skip such case for this
moment.
For IP layer, skb drop reasons are added to the packet outputting path.
Seems the reasons are not complex, so I didn't split the commits by
functions.
For neighbour part, SKB_DROP_REASON_NEIGH_FAILED and
SKB_DROP_REASON_NEIGH_QUEUEFULL are added.
For link layer, reasons are added for both packet inputting and
outputting path.
The amount of patches in this series seems a bit too many, maybe I should
join some of them? For example, combine the patches of dev to one.
Menglong Dong (19):
net: tcp: introduce tcp_drop_reason()
net: tcp: add skb drop reasons to tcp_v4_rcv()
net: tcp: use kfree_skb_reason() for tcp_v6_rcv()
net: tcp: add skb drop reasons to tcp_v{4,6}_inbound_md5_hash()
net: tcp: add skb drop reasons to tcp_add_backlog()
net: tcp: use kfree_skb_reason() for tcp_v{4,6}_do_rcv()
net: tcp: use tcp_drop_reason() for tcp_rcv_established()
net: tcp: use tcp_drop_reason() for tcp_data_queue()
net: tcp: use tcp_drop_reason() for tcp_data_queue_ofo()
net: ip: add skb drop reasons during ip outputting
net: neigh: use kfree_skb_reason() for __neigh_event_send()
net: neigh: add skb drop reasons to arp_error_report()
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 | 82 +++++++++++++++++++++++++++++++++++++-
include/net/tcp.h | 3 +-
include/trace/events/skb.h | 21 ++++++++++
net/core/dev.c | 25 +++++++-----
net/core/neighbour.c | 4 +-
net/core/skbuff.c | 7 ++--
net/ipv4/arp.c | 2 +-
net/ipv4/ip_output.c | 6 +--
net/ipv4/tcp_input.c | 45 ++++++++++++++++-----
net/ipv4/tcp_ipv4.c | 36 ++++++++++++-----
net/ipv6/ip6_output.c | 6 +--
net/ipv6/tcp_ipv6.c | 42 ++++++++++++++-----
12 files changed, 227 insertions(+), 52 deletions(-)
--
2.34.1
From: Menglong Dong <redacted>
For TCP protocol, tcp_drop() is used to free the skb when it needs
to be dropped. To make use of kfree_skb_reason() and collect drop
reasons, introduce the function tcp_drop_reason().
tcp_drop_reason() will finally call kfree_skb_reason() and pass the
drop reason to 'kfree_skb' tracepoint.
PS: __kfree_skb() was used in tcp_drop(), I'm not sure if it's ok
to replace it with kfree_skb_reason().
Signed-off-by: Menglong Dong <redacted>
---
net/ipv4/tcp_input.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -4684,10 +4684,19 @@ static bool tcp_ooo_try_coalesce(struct sock *sk,returnres;}-staticvoidtcp_drop(structsock*sk,structsk_buff*skb)+staticvoidtcp_drop_reason(structsock*sk,structsk_buff*skb,+enumskb_drop_reasonreason){sk_drops_add(sk,skb);-__kfree_skb(skb);+/* why __kfree_skb() used here before, other than kfree_skb()?+*confusing......+*/+kfree_skb_reason(skb,reason);+}++staticinlinevoidtcp_drop(structsock*sk,structsk_buff*skb)+{+tcp_drop_reason(sk,skb,SKB_DROP_REASON_NOT_SPECIFIED);}/* This one checks to see if we can put data from the
From: Menglong Dong <redacted>
Pass the address of drop reason to tcp_v4_inbound_md5_hash() and
tcp_v6_inbound_md5_hash() to store the reasons for skb drops when this
function fails. Therefore, the drop reason can be passed to
kfree_skb_reason() when the skb needs to be freed.
Following drop reasons are added:
SKB_DROP_REASON_TCP_MD5NOTFOUND
SKB_DROP_REASON_TCP_MD5UNEXPECTED
SKB_DROP_REASON_TCP_MD5FAILURE
Signed-off-by: Menglong Dong <redacted>
---
include/linux/skbuff.h | 7 +++++++
include/trace/events/skb.h | 4 ++++
net/ipv4/tcp_ipv4.c | 13 +++++++++----
net/ipv6/tcp_ipv6.c | 11 ++++++++---
4 files changed, 28 insertions(+), 7 deletions(-)
@@ -346,6 +346,13 @@ enum skb_drop_reason {*udppacketdropoutof*udp_memory_allocated.*/+SKB_DROP_REASON_TCP_MD5NOTFOUND,/* No MD5 hash and one+*expected+*/+SKB_DROP_REASON_TCP_MD5UNEXPECTED,/* MD5 hash and we're not+*expectingone+*/+SKB_DROP_REASON_TCP_MD5FAILURE,/* MD5 hash and its wrong */SKB_DROP_REASON_MAX,};
@@ -27,6 +27,10 @@EM(SKB_DROP_REASON_IP_NOPROTO,IP_NOPROTO)\EM(SKB_DROP_REASON_SOCKET_RCVBUFF,SOCKET_RCVBUFF)\EM(SKB_DROP_REASON_PROTO_MEM,PROTO_MEM)\+EM(SKB_DROP_REASON_TCP_MD5NOTFOUND,TCP_MD5NOTFOUND)\+EM(SKB_DROP_REASON_TCP_MD5UNEXPECTED,\+TCP_MD5UNEXPECTED)\+EM(SKB_DROP_REASON_TCP_MD5FAILURE,TCP_MD5FAILURE)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
@@ -1412,7 +1412,8 @@ EXPORT_SYMBOL(tcp_v4_md5_hash_skb);/* Called with rcu_read_lock() */staticbooltcp_v4_inbound_md5_hash(conststructsock*sk,conststructsk_buff*skb,-intdif,intsdif)+intdif,intsdif,+enumskb_drop_reason*reason){#ifdef CONFIG_TCP_MD5SIG/*
@@ -353,6 +353,10 @@ enum skb_drop_reason {*expectingone*/SKB_DROP_REASON_TCP_MD5FAILURE,/* MD5 hash and its wrong */+SKB_DROP_REASON_SOCKET_BACKLOG,/* failed to add skb to socket+*backlog(see+*LINUX_MIB_TCPBACKLOGDROP)+*/SKB_DROP_REASON_MAX,};
@@ -31,6 +31,7 @@EM(SKB_DROP_REASON_TCP_MD5UNEXPECTED,\TCP_MD5UNEXPECTED)\EM(SKB_DROP_REASON_TCP_MD5FAILURE,TCP_MD5FAILURE)\+EM(SKB_DROP_REASON_SOCKET_BACKLOG,SOCKET_BACKLOG)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
@@ -1757,7 +1759,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)reset:tcp_v4_send_reset(rsk,skb);discard:-kfree_skb(skb);+kfree_skb_reason(skb,reason);/* Be careful here. If this function gets more complicated and*gccsuffersfromregisterpressureonthex86,sk(in%ebx)*mightbedestroyedhere.Thiscurrentversioncompilescorrectly,
@@ -32,6 +32,7 @@TCP_MD5UNEXPECTED)\EM(SKB_DROP_REASON_TCP_MD5FAILURE,TCP_MD5FAILURE)\EM(SKB_DROP_REASON_SOCKET_BACKLOG,SOCKET_BACKLOG)\+EM(SKB_DROP_REASON_TCP_FLAGS,TCP_FLAGS)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
From: Menglong Dong <redacted>
Replace tcp_drop() used in tcp_data_queue() with tcp_drop_reason().
Following drop reasons are introduced:
SKB_DROP_REASON_TCP_ZEROWINDOW
SKB_DROP_REASON_TCP_OLD_DATA
SKB_DROP_REASON_TCP_OVERWINDOW
SKB_DROP_REASON_TCP_OLD_DATA is used for the case that end_seq of skb
less than the left edges of receive window. (Maybe there is a better
name?)
Signed-off-by: Menglong Dong <redacted>
---
include/linux/skbuff.h | 13 +++++++++++++
include/trace/events/skb.h | 3 +++
net/ipv4/tcp_input.c | 13 +++++++++++--
3 files changed, 27 insertions(+), 2 deletions(-)
@@ -358,6 +358,19 @@ enum skb_drop_reason {*LINUX_MIB_TCPBACKLOGDROP)*/SKB_DROP_REASON_TCP_FLAGS,/* TCP flags invalid */+SKB_DROP_REASON_TCP_ZEROWINDOW,/* TCP receive window size is zero,+*seeLINUX_MIB_TCPZEROWINDOWDROP+*/+SKB_DROP_REASON_TCP_OLD_DATA,/* the TCP data reveived is already+*receivedbefore(spuriousretrans+*mayhappened),see+*LINUX_MIB_DELAYEDACKLOST+*/+SKB_DROP_REASON_TCP_OVERWINDOW,/* the TCP data is out of window,+*theseqofthefirstbyteexceed+*therightedgesofreceive+*window+*/SKB_DROP_REASON_MAX,};
@@ -33,6 +33,9 @@EM(SKB_DROP_REASON_TCP_MD5FAILURE,TCP_MD5FAILURE)\EM(SKB_DROP_REASON_SOCKET_BACKLOG,SOCKET_BACKLOG)\EM(SKB_DROP_REASON_TCP_FLAGS,TCP_FLAGS)\+EM(SKB_DROP_REASON_TCP_ZEROWINDOW,TCP_ZEROWINDOW)\+EM(SKB_DROP_REASON_TCP_OLD_DATA,TCP_OLD_DATA)\+EM(SKB_DROP_REASON_TCP_OVERWINDOW,TCP_OVERWINDOW)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
@@ -5009,6 +5010,7 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)skb_dst_drop(skb);__skb_pull(skb,tcp_hdr(skb)->doff*4);+reason=SKB_DROP_REASON_NOT_SPECIFIED;tp->rx_opt.dsack=0;/* Queue data for delivery to the user.
@@ -5062,6 +5066,7 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)if(!after(TCP_SKB_CB(skb)->end_seq,tp->rcv_nxt)){tcp_rcv_spurious_retrans(sk,skb);/* A retransmit, 2nd most common case. Force an immediate ack. */+reason=SKB_DROP_REASON_TCP_OLD_DATA;NET_INC_STATS(sock_net(sk),LINUX_MIB_DELAYEDACKLOST);tcp_dsack_set(sk,TCP_SKB_CB(skb)->seq,TCP_SKB_CB(skb)->end_seq);
@@ -5069,13 +5074,16 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)tcp_enter_quickack_mode(sk,TCP_MAX_QUICKACKS);inet_csk_schedule_ack(sk);drop:-tcp_drop(sk,skb);+tcp_drop_reason(sk,skb,reason);return;}/* Out of window. F.e. zero window probe. */-if(!before(TCP_SKB_CB(skb)->seq,tp->rcv_nxt+tcp_receive_window(tp)))+if(!before(TCP_SKB_CB(skb)->seq,+tp->rcv_nxt+tcp_receive_window(tp))){+reason=SKB_DROP_REASON_TCP_OVERWINDOW;gotoout_of_window;+}if(before(TCP_SKB_CB(skb)->seq,tp->rcv_nxt)){/* Partial packet, seq < rcv_next < end_seq */
@@ -371,6 +371,9 @@ enum skb_drop_reason {*therightedgesofreceive*window*/+SKB_DROP_REASON_TCP_OFOMERGE,/* the data of skb is already in+*theofoqueue.+*/SKB_DROP_REASON_MAX,};
@@ -36,6 +36,7 @@EM(SKB_DROP_REASON_TCP_ZEROWINDOW,TCP_ZEROWINDOW)\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)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
@@ -4892,7 +4894,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)tcp_dsack_extend(sk,TCP_SKB_CB(skb1)->seq,TCP_SKB_CB(skb1)->end_seq);NET_INC_STATS(sock_net(sk),LINUX_MIB_TCPOFOMERGE);-tcp_drop(sk,skb1);+tcp_drop_reason(sk,skb1,SKB_DROP_REASON_TCP_OFOMERGE);}/* If there is no skb after us, we are the last_skb ! */if(!skb1)
From: Menglong Dong <redacted>
As kfree_skb() is not used frequently during packet outputting in ip
layer, we do this job (add reasons for skb drops) at once.
kfree_skb() is replaced by kfree_skb_reason() in following functions:
__ip_queue_xmit(), ip_finish_output(), ip_mc_finish_output(),
ip6_output(), ip6_finish_output(), ip6_finish_output2()
and following drop reasons are added:
SKB_DROP_REASON_IP_OUTNOROUTES
SKB_DROP_REASON_BPF_CGROUP_EGRESS
SKB_DROP_REASON_IPV6DSIABLED
Signed-off-by: Menglong Dong <redacted>
---
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(-)
@@ -374,6 +374,19 @@ enum skb_drop_reason {SKB_DROP_REASON_TCP_OFOMERGE,/* the data of skb is already in*theofoqueue.*/+SKB_DROP_REASON_IP_OUTNOROUTES,/* route lookup failed during+*packetoutputting+*/+SKB_DROP_REASON_BPF_CGROUP_EGRESS,/* dropped by eBPF program+*withtypeofBPF_PROG_TYPE_CGROUP_SKB+*andattachtypeof+*BPF_CGROUP_INET_EGRESS+*duringpacketsending+*/+SKB_DROP_REASON_IPV6DSIABLED,/* IPv6 is disabled on the device,+*seethedocfordisable_ipv6+*inip-sysctl.rstfordetail+*/SKB_DROP_REASON_MAX,};
@@ -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
From: Menglong Dong <redacted>
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
layer.
Signed-off-by: Menglong Dong <redacted>
---
include/linux/skbuff.h | 9 +++++++++
include/trace/events/skb.h | 2 ++
net/core/neighbour.c | 4 ++--
3 files changed, 13 insertions(+), 2 deletions(-)
@@ -387,6 +387,15 @@ enum skb_drop_reason {*seethedocfordisable_ipv6*inip-sysctl.rstfordetail*/+SKB_DROP_REASON_NEIGH_FAILED,/* dropped as the state of+*neighbourisNUD_FAILED+*/+SKB_DROP_REASON_NEIGH_QUEUEFULL,/* the skbs that waiting+*forsendingonthequeue+*ofneigh->arp_queueis+*full,andtheskbsonthe+*tailwillbedropped+*/SKB_DROP_REASON_MAX,};
@@ -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
From: Menglong Dong <redacted>
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.
Signed-off-by: Menglong Dong <redacted>
---
net/ipv4/arp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Menglong Dong <redacted>
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 egress, we make it distinct with the drop reason
of SKB_DROP_REASON_QDISC_DROP in the next commit.
Signed-off-by: Menglong Dong <redacted>
---
include/linux/skbuff.h | 4 ++++
include/trace/events/skb.h | 1 +
net/core/dev.c | 2 +-
3 files changed, 6 insertions(+), 1 deletion(-)
@@ -43,6 +43,7 @@EM(SKB_DROP_REASON_IPV6DSIABLED,IPV6DSIABLED)\EM(SKB_DROP_REASON_NEIGH_FAILED,NEIGH_FAILED)\EM(SKB_DROP_REASON_NEIGH_QUEUEFULL,NEIGH_QUEUEFULL)\+EM(SKB_DROP_REASON_QDISC_EGRESS,QDISC_EGRESS)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
From: Menglong Dong <redacted>
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 <redacted>
---
include/linux/skbuff.h | 8 +++++++-
net/core/skbuff.c | 7 ++++---
2 files changed, 11 insertions(+), 4 deletions(-)
@@ -44,6 +44,7 @@EM(SKB_DROP_REASON_NEIGH_FAILED,NEIGH_FAILED)\EM(SKB_DROP_REASON_NEIGH_QUEUEFULL,NEIGH_QUEUEFULL)\EM(SKB_DROP_REASON_QDISC_EGRESS,QDISC_EGRESS)\+EM(SKB_DROP_REASON_QDISC_DROP,QDISC_DROP)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
From: Menglong Dong <redacted>
Replace kfree_skb() used in enqueue_to_backlog() with
kfree_skb_reason(). The skb drop 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 needn't to make further distinctions.
Signed-off-by: Menglong Dong <redacted>
---
include/linux/skbuff.h | 6 ++++++
include/trace/events/skb.h | 1 +
net/core/dev.c | 6 +++++-
3 files changed, 12 insertions(+), 1 deletion(-)
@@ -45,6 +45,7 @@EM(SKB_DROP_REASON_NEIGH_QUEUEFULL,NEIGH_QUEUEFULL)\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
@@ -46,6 +46,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
@@ -411,6 +411,10 @@ enum skb_drop_reason {*net.rst)orRPSflowlimit*/SKB_DROP_REASON_XDP,/* dropped by XDP in input path */+SKB_DROP_REASON_QDISC_INGRESS,/* qdisc of type ingress check+*failed(maybeaneBPFprogram+*istricking?)+*/SKB_DROP_REASON_MAX,};
@@ -47,6 +47,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
From: Menglong Dong <redacted>
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 <redacted>
---
include/linux/skbuff.h | 5 +++++
include/trace/events/skb.h | 1 +
net/core/dev.c | 8 +++++---
3 files changed, 11 insertions(+), 3 deletions(-)
@@ -415,6 +415,11 @@ enum skb_drop_reason {*failed(maybeaneBPFprogram*istricking?)*/+SKB_DROP_REASON_PTYPE_ABSENT,/* no packet_type found to handle+*theskb.Foranetnerpacket,+*thismeansthatL3protocolis+*notsupported+*/SKB_DROP_REASON_MAX,};
@@ -48,6 +48,7 @@EM(SKB_DROP_REASON_CPU_BACKLOG,CPU_BACKLOG)\EM(SKB_DROP_REASON_XDP,XDP)\EM(SKB_DROP_REASON_QDISC_INGRESS,QDISC_INGRESS)\+EM(SKB_DROP_REASON_PTYPE_ABSENT,PTYPE_ABSENT)\EMe(SKB_DROP_REASON_MAX,MAX)#undef EM
@@ -5323,11 +5323,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*mehowyouweregoingtousethis.:-)*/
From: Jakub Kicinski <kuba@kernel.org> Date: 2022-02-15 16:05:01
On Tue, 15 Feb 2022 19:27:53 +0800 menglong8.dong@gmail.com wrote:
From: Menglong Dong <redacted>
In this series patches, reasons for skb drops are added to TCP, IP, dev
and neigh.
For TCP layer, the path of TCP data receive and enqueue are considered.
However, it's more complex for TCP state processing, as I find that it's
hard to report skb drop reasons to where it is freed. For example,
when skb is dropped in tcp_rcv_state_process(), the reason can be caused
by the call of tcp_v4_conn_request(), and it's hard to return a drop
reason from tcp_v4_conn_request(). So I just skip such case for this
moment.
For IP layer, skb drop reasons are added to the packet outputting path.
Seems the reasons are not complex, so I didn't split the commits by
functions.
For neighbour part, SKB_DROP_REASON_NEIGH_FAILED and
SKB_DROP_REASON_NEIGH_QUEUEFULL are added.
For link layer, reasons are added for both packet inputting and
outputting path.
The amount of patches in this series seems a bit too many, maybe I should
join some of them? For example, combine the patches of dev to one.
This series does not apply cleanly.
There's no reason to send 19 patches at a time. Please try to send
smaller series, that's are easier to review, under 10 patches
preferably, certainly under 15.
From: David Ahern <dsahern@kernel.org> Date: 2022-02-15 16:09:11
On 2/15/22 9:04 AM, Jakub Kicinski wrote:
There's no reason to send 19 patches at a time. Please try to send
smaller series, that's are easier to review, under 10 patches
preferably, certainly under 15.
+1. It takes time to review code paths and make sure the changes are
correct.
Send the first 9 as set; those target the TCP stack and then wait for
them to be merged before sending more.
From: Eric Dumazet <edumazet@google.com> Date: 2022-02-15 17:34:59
On Tue, Feb 15, 2022 at 3:30 AM [off-list ref] wrote:
quoted hunk
From: Menglong Dong <redacted>
For TCP protocol, tcp_drop() is used to free the skb when it needs
to be dropped. To make use of kfree_skb_reason() and collect drop
reasons, introduce the function tcp_drop_reason().
tcp_drop_reason() will finally call kfree_skb_reason() and pass the
drop reason to 'kfree_skb' tracepoint.
PS: __kfree_skb() was used in tcp_drop(), I'm not sure if it's ok
to replace it with kfree_skb_reason().
Signed-off-by: Menglong Dong <redacted>
---
net/ipv4/tcp_input.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -4684,10 +4684,19 @@ static bool tcp_ooo_try_coalesce(struct sock *sk,returnres;}-staticvoidtcp_drop(structsock*sk,structsk_buff*skb)+staticvoidtcp_drop_reason(structsock*sk,structsk_buff*skb,+enumskb_drop_reasonreason){sk_drops_add(sk,skb);-__kfree_skb(skb);+/* why __kfree_skb() used here before, other than kfree_skb()?+*confusing......
Do not add comments like that if you do not know the difference...
__kfree_skb() is used by TCP stack because it owns skb in receive
queues, and avoids touching skb->users
because it must be one already.
(We made sure not using skb_get() in TCP)
It seems fine to use kfree_skb() in tcp_drop(), it is hardly fast
path, and the added cost is pure noise.
@@ -4684,10 +4684,19 @@ static bool tcp_ooo_try_coalesce(struct sock *sk,returnres;}-staticvoidtcp_drop(structsock*sk,structsk_buff*skb)+staticvoidtcp_drop_reason(structsock*sk,structsk_buff*skb,+enumskb_drop_reasonreason){sk_drops_add(sk,skb);-__kfree_skb(skb);+/* why __kfree_skb() used here before, other than kfree_skb()?+*confusing......
Do not add comments like that if you do not know the difference...
__kfree_skb() is used by TCP stack because it owns skb in receive
queues, and avoids touching skb->users
because it must be one already.
and it bypasses kfree_skb tracepoint which seems by design.
On Wed, Feb 16, 2022 at 12:09 AM David Ahern [off-list ref] wrote:
On 2/15/22 9:04 AM, Jakub Kicinski wrote:
quoted
There's no reason to send 19 patches at a time. Please try to send
smaller series, that's are easier to review, under 10 patches
preferably, certainly under 15.
+1. It takes time to review code paths and make sure the changes are
correct.
Send the first 9 as set; those target the TCP stack and then wait for
them to be merged before sending more.
Ok, I'll make the amount of patches at a proper level, thanks!
On Wed, Feb 16, 2022 at 1:34 AM Eric Dumazet [off-list ref] wrote:
On Tue, Feb 15, 2022 at 3:30 AM [off-list ref] wrote:
quoted
From: Menglong Dong <redacted>
For TCP protocol, tcp_drop() is used to free the skb when it needs
to be dropped. To make use of kfree_skb_reason() and collect drop
reasons, introduce the function tcp_drop_reason().
tcp_drop_reason() will finally call kfree_skb_reason() and pass the
drop reason to 'kfree_skb' tracepoint.
PS: __kfree_skb() was used in tcp_drop(), I'm not sure if it's ok
to replace it with kfree_skb_reason().
Signed-off-by: Menglong Dong <redacted>
---
net/ipv4/tcp_input.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -4684,10 +4684,19 @@ static bool tcp_ooo_try_coalesce(struct sock *sk,returnres;}-staticvoidtcp_drop(structsock*sk,structsk_buff*skb)+staticvoidtcp_drop_reason(structsock*sk,structsk_buff*skb,+enumskb_drop_reasonreason){sk_drops_add(sk,skb);-__kfree_skb(skb);+/* why __kfree_skb() used here before, other than kfree_skb()?+*confusing......
Do not add comments like that if you do not know the difference...
__kfree_skb() is used by TCP stack because it owns skb in receive
queues, and avoids touching skb->users
because it must be one already.
(We made sure not using skb_get() in TCP)
It seems fine to use kfree_skb() in tcp_drop(), it is hardly fast
path, and the added cost is pure noise.
I understand why __kfree_skb() was used now, and it seems
this commit is ok (with the comments removed of course). I'll
keep it still.
Thanks!
Menglong Dong
@@ -4684,10 +4684,19 @@ static bool tcp_ooo_try_coalesce(struct sock *sk,returnres;}-staticvoidtcp_drop(structsock*sk,structsk_buff*skb)+staticvoidtcp_drop_reason(structsock*sk,structsk_buff*skb,+enumskb_drop_reasonreason){sk_drops_add(sk,skb);-__kfree_skb(skb);+/* why __kfree_skb() used here before, other than kfree_skb()?+*confusing......
Do not add comments like that if you do not know the difference...
__kfree_skb() is used by TCP stack because it owns skb in receive
queues, and avoids touching skb->users
because it must be one already.
and it bypasses kfree_skb tracepoint which seems by design.
Do you mean it shouldn't be traced here?
According to my understanding, __kfree_skb() was used in the
beginning as skb->users aren't touched by TCP. Later,
tcp_drop() was introduced to record drop count to the socket.
Considering the skb is indeed dropped and no other event is triggered,
is it ok to trigger the kfree_skb tracepoint?
Thanks!
Menglong Dong