| Message ID | 20220221053440.7320-3-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tun/tap: use kfree_skb_reason() to trace dropped skb | expand |
On 2/20/22 10:34 PM, Dongli Zhang wrote: > The TAP can be used as vhost-net backend. E.g., the tap_handle_frame() is > the interface to forward the skb from TAP to vhost-net/virtio-net. > > However, there are many "goto drop" in the TAP driver. Therefore, the > kfree_skb_reason() is involved at each "goto drop" to help userspace > ftrace/ebpf to track the reason for the loss of packets. > > The below reasons are introduced: > > - SKB_DROP_REASON_SKB_CSUM > - SKB_DROP_REASON_SKB_COPY_DATA > - SKB_DROP_REASON_SKB_GSO_SEG > - SKB_DROP_REASON_DEV_HDR > - SKB_DROP_REASON_FULL_RING > > Cc: Joao Martins <joao.m.martins@oracle.com> > Cc: Joe Jin <joe.jin@oracle.com> > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > Changed since v1: > - revise the reason name > Changed since v2: > - declare drop_reason as type "enum skb_drop_reason" > - handle the drop in skb_list_walk_safe() case > > drivers/net/tap.c | 35 +++++++++++++++++++++++++---------- > include/linux/skbuff.h | 9 +++++++++ > include/trace/events/skb.h | 5 +++++ > 3 files changed, 39 insertions(+), 10 deletions(-) > couple of places where the new reason should be in reverse xmas order; logic wise: Reviewed-by: David Ahern <dsahern@kernel.org>
Hi David, On 2/21/22 7:24 PM, David Ahern wrote: > On 2/20/22 10:34 PM, Dongli Zhang wrote: >> The TAP can be used as vhost-net backend. E.g., the tap_handle_frame() is >> the interface to forward the skb from TAP to vhost-net/virtio-net. >> >> However, there are many "goto drop" in the TAP driver. Therefore, the >> kfree_skb_reason() is involved at each "goto drop" to help userspace >> ftrace/ebpf to track the reason for the loss of packets. >> >> The below reasons are introduced: >> >> - SKB_DROP_REASON_SKB_CSUM >> - SKB_DROP_REASON_SKB_COPY_DATA >> - SKB_DROP_REASON_SKB_GSO_SEG >> - SKB_DROP_REASON_DEV_HDR >> - SKB_DROP_REASON_FULL_RING >> >> Cc: Joao Martins <joao.m.martins@oracle.com> >> Cc: Joe Jin <joe.jin@oracle.com> >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> --- >> Changed since v1: >> - revise the reason name >> Changed since v2: >> - declare drop_reason as type "enum skb_drop_reason" >> - handle the drop in skb_list_walk_safe() case >> >> drivers/net/tap.c | 35 +++++++++++++++++++++++++---------- >> include/linux/skbuff.h | 9 +++++++++ >> include/trace/events/skb.h | 5 +++++ >> 3 files changed, 39 insertions(+), 10 deletions(-) >> > > couple of places where the new reason should be in reverse xmas order; > logic wise: > > Reviewed-by: David Ahern <dsahern@kernel.org> > I will re-order the reasons in the same patch and re-send with your Reviewed-by in the next version. Thank you very much! Dongli Zhang
On 2/21/22 8:31 PM, Dongli Zhang wrote: > Hi David, > > On 2/21/22 7:24 PM, David Ahern wrote: >> On 2/20/22 10:34 PM, Dongli Zhang wrote: >>> The TAP can be used as vhost-net backend. E.g., the tap_handle_frame() is >>> the interface to forward the skb from TAP to vhost-net/virtio-net. >>> >>> However, there are many "goto drop" in the TAP driver. Therefore, the >>> kfree_skb_reason() is involved at each "goto drop" to help userspace >>> ftrace/ebpf to track the reason for the loss of packets. >>> >>> The below reasons are introduced: >>> >>> - SKB_DROP_REASON_SKB_CSUM >>> - SKB_DROP_REASON_SKB_COPY_DATA >>> - SKB_DROP_REASON_SKB_GSO_SEG >>> - SKB_DROP_REASON_DEV_HDR >>> - SKB_DROP_REASON_FULL_RING >>> >>> Cc: Joao Martins <joao.m.martins@oracle.com> >>> Cc: Joe Jin <joe.jin@oracle.com> >>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >>> --- >>> Changed since v1: >>> - revise the reason name >>> Changed since v2: >>> - declare drop_reason as type "enum skb_drop_reason" >>> - handle the drop in skb_list_walk_safe() case >>> >>> drivers/net/tap.c | 35 +++++++++++++++++++++++++---------- >>> include/linux/skbuff.h | 9 +++++++++ >>> include/trace/events/skb.h | 5 +++++ >>> 3 files changed, 39 insertions(+), 10 deletions(-) >>> >> >> couple of places where the new reason should be in reverse xmas order; >> logic wise: >> >> Reviewed-by: David Ahern <dsahern@kernel.org> >> > > I will re-order the reasons in the same patch and re-send with your Reviewed-by > in the next version. > I have sent out v4 and I finally decide to not re-order reasons for this patch as this may makes trouble for backport. I will not follow the reverse xmas order here, as all existing variables are not declared in reverse xmas order. Thank you very much! Dongli Zhang
diff --git a/drivers/net/tap.c b/drivers/net/tap.c index 8e3a28b..b48f519 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -322,6 +322,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) struct tap_dev *tap; struct tap_queue *q; netdev_features_t features = TAP_FEATURES; + enum skb_drop_reason drop_reason; tap = tap_dev_get_rcu(dev); if (!tap) @@ -343,12 +344,16 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) struct sk_buff *segs = __skb_gso_segment(skb, features, false); struct sk_buff *next; - if (IS_ERR(segs)) + if (IS_ERR(segs)) { + drop_reason = SKB_DROP_REASON_SKB_GSO_SEG; goto drop; + } if (!segs) { - if (ptr_ring_produce(&q->ring, skb)) + if (ptr_ring_produce(&q->ring, skb)) { + drop_reason = SKB_DROP_REASON_FULL_RING; goto drop; + } goto wake_up; } @@ -356,8 +361,9 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) skb_list_walk_safe(segs, skb, next) { skb_mark_not_on_list(skb); if (ptr_ring_produce(&q->ring, skb)) { - kfree_skb(skb); - kfree_skb_list(next); + drop_reason = SKB_DROP_REASON_FULL_RING; + kfree_skb_reason(skb, drop_reason); + kfree_skb_list_reason(next, drop_reason); break; } } @@ -369,10 +375,14 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) */ if (skb->ip_summed == CHECKSUM_PARTIAL && !(features & NETIF_F_CSUM_MASK) && - skb_checksum_help(skb)) + skb_checksum_help(skb)) { + drop_reason = SKB_DROP_REASON_SKB_CSUM; goto drop; - if (ptr_ring_produce(&q->ring, skb)) + } + if (ptr_ring_produce(&q->ring, skb)) { + drop_reason = SKB_DROP_REASON_FULL_RING; goto drop; + } } wake_up: @@ -383,7 +393,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) /* Count errors/drops only here, thus don't care about args. */ if (tap->count_rx_dropped) tap->count_rx_dropped(tap); - kfree_skb(skb); + kfree_skb_reason(skb, drop_reason); return RX_HANDLER_CONSUMED; } EXPORT_SYMBOL_GPL(tap_handle_frame); @@ -632,6 +642,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, int depth; bool zerocopy = false; size_t linear; + enum skb_drop_reason drop_reason; if (q->flags & IFF_VNET_HDR) { vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); @@ -696,8 +707,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, else err = skb_copy_datagram_from_iter(skb, 0, from, len); - if (err) + if (err) { + drop_reason = SKB_DROP_REASON_SKB_COPY_DATA; goto err_kfree; + } skb_set_network_header(skb, ETH_HLEN); skb_reset_mac_header(skb); @@ -706,8 +719,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, if (vnet_hdr_len) { err = virtio_net_hdr_to_skb(skb, &vnet_hdr, tap_is_little_endian(q)); - if (err) + if (err) { + drop_reason = SKB_DROP_REASON_DEV_HDR; goto err_kfree; + } } skb_probe_transport_header(skb); @@ -738,7 +753,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, return total_len; err_kfree: - kfree_skb(skb); + kfree_skb_reason(skb, drop_reason); err: rcu_read_lock(); diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 87ebe2f..52550c7 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -380,6 +380,15 @@ enum skb_drop_reason { * the ofo queue, corresponding to * LINUX_MIB_TCPOFOMERGE */ + SKB_DROP_REASON_SKB_CSUM, /* sk_buff checksum error */ + SKB_DROP_REASON_SKB_COPY_DATA, /* failed to copy data from or to + * sk_buff + */ + SKB_DROP_REASON_SKB_GSO_SEG, /* gso segmentation error */ + SKB_DROP_REASON_DEV_HDR, /* there is something wrong with + * device driver specific header + */ + SKB_DROP_REASON_FULL_RING, /* ring buffer is full */ SKB_DROP_REASON_MAX, }; diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h index 2ab7193..5b5f135 100644 --- a/include/trace/events/skb.h +++ b/include/trace/events/skb.h @@ -37,6 +37,11 @@ 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_SKB_CSUM, SKB_CSUM) \ + EM(SKB_DROP_REASON_SKB_COPY_DATA, SKB_COPY_DATA) \ + EM(SKB_DROP_REASON_SKB_GSO_SEG, SKB_GSO_SEG) \ + EM(SKB_DROP_REASON_DEV_HDR, DEV_HDR) \ + EM(SKB_DROP_REASON_FULL_RING, FULL_RING) \ EMe(SKB_DROP_REASON_MAX, MAX) #undef EM
The TAP can be used as vhost-net backend. E.g., the tap_handle_frame() is the interface to forward the skb from TAP to vhost-net/virtio-net. However, there are many "goto drop" in the TAP driver. Therefore, the kfree_skb_reason() is involved at each "goto drop" to help userspace ftrace/ebpf to track the reason for the loss of packets. The below reasons are introduced: - SKB_DROP_REASON_SKB_CSUM - SKB_DROP_REASON_SKB_COPY_DATA - SKB_DROP_REASON_SKB_GSO_SEG - SKB_DROP_REASON_DEV_HDR - SKB_DROP_REASON_FULL_RING Cc: Joao Martins <joao.m.martins@oracle.com> Cc: Joe Jin <joe.jin@oracle.com> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- Changed since v1: - revise the reason name Changed since v2: - declare drop_reason as type "enum skb_drop_reason" - handle the drop in skb_list_walk_safe() case drivers/net/tap.c | 35 +++++++++++++++++++++++++---------- include/linux/skbuff.h | 9 +++++++++ include/trace/events/skb.h | 5 +++++ 3 files changed, 39 insertions(+), 10 deletions(-)