mbox series

[net-next,00/19] net: add skb drop reasons for TCP, IP, dev and neigh

Message ID 20220215112812.2093852-1-imagedong@tencent.com (mailing list archive)
Headers show
Series net: add skb drop reasons for TCP, IP, dev and neigh | expand

Message

Menglong Dong Feb. 15, 2022, 11:27 a.m. UTC 
From: Menglong Dong <imagedong@tencent.com>

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(-)

Comments

Jakub Kicinski Feb. 15, 2022, 4:04 p.m. UTC | #1 
On Tue, 15 Feb 2022 19:27:53 +0800 menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> 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.
David Ahern Feb. 15, 2022, 4:09 p.m. UTC | #2 
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.
Menglong Dong Feb. 16, 2022, 1:55 a.m. UTC | #3 
On Wed, Feb 16, 2022 at 12:09 AM David Ahern <dsahern@kernel.org> wrote:
>
> 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.

Ok, I'll make the amount of patches at a proper level, thanks!