| Message ID | 20220208035510.1200-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 |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 2/7/22 7:55 PM, Dongli Zhang wrote: > The TUN can be used as vhost-net backend. E.g, the tun_net_xmit() is the > interface to forward the skb from TUN to vhost-net/virtio-net. > > However, there are many "goto drop" in the TUN 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. > > Cc: Joao Martins <joao.m.martins@oracle.com> > Cc: Joe Jin <joe.jin@oracle.com> > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > drivers/net/tun.c | 33 +++++++++++++++++++++++++-------- > include/linux/skbuff.h | 6 ++++++ > include/trace/events/skb.h | 6 ++++++ > 3 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index fed85447701a..d67f2419dbb4 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > struct netdev_queue *queue; > struct tun_file *tfile; > int len = skb->len; > + int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > > rcu_read_lock(); > tfile = rcu_dereference(tun->tfiles[txq]); > > /* Drop packet if interface is not attached */ > - if (!tfile) > + if (!tfile) { > + drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED; That is going to be a confusing reason code (tap device existed to get here) and does not really explain this error. > goto drop; > + } > > if (!rcu_dereference(tun->steering_prog)) > tun_automq_xmit(tun, skb); > @@ -1078,19 +1081,27 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > /* Drop if the filter does not like it. > * This is a noop if the filter is disabled. > * Filter can be enabled only for the TAP devices. */ > - if (!check_filter(&tun->txflt, skb)) > + if (!check_filter(&tun->txflt, skb)) { > + drop_reason = SKB_DROP_REASON_TAP_RUN_FILTER; just SKB_DROP_REASON_TAP_FILTER > goto drop; > + } > > if (tfile->socket.sk->sk_filter && > - sk_filter(tfile->socket.sk, skb)) > + sk_filter(tfile->socket.sk, skb)) { > + drop_reason = SKB_DROP_REASON_SKB_TRIM; SKB_DROP_REASON_SOCKET_FILTER The remainder of your changes feels like another variant of your previous "function / line" reason code. You are creating new reason codes for every goto failure with a code based name. The reason needs to be the essence of the failure in a user friendly label.
Hi David, On 2/7/22 9:03 PM, David Ahern wrote: > On 2/7/22 7:55 PM, Dongli Zhang wrote: >> The TUN can be used as vhost-net backend. E.g, the tun_net_xmit() is the >> interface to forward the skb from TUN to vhost-net/virtio-net. >> >> However, there are many "goto drop" in the TUN 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. >> >> Cc: Joao Martins <joao.m.martins@oracle.com> >> Cc: Joe Jin <joe.jin@oracle.com> >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> --- >> drivers/net/tun.c | 33 +++++++++++++++++++++++++-------- >> include/linux/skbuff.h | 6 ++++++ >> include/trace/events/skb.h | 6 ++++++ >> 3 files changed, 37 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index fed85447701a..d67f2419dbb4 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) >> struct netdev_queue *queue; >> struct tun_file *tfile; >> int len = skb->len; >> + int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > I will avoid initializing here. > >> >> rcu_read_lock(); >> tfile = rcu_dereference(tun->tfiles[txq]); >> >> /* Drop packet if interface is not attached */ >> - if (!tfile) >> + if (!tfile) { >> + drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED; Initially I was using TUN_NOT_ATTACHED. I used a more generic DEV_NOT_ATTACHED in order to re-use the reason in the future. How about TUN specific TUN_NOT_ATTACHED, as the core issue is because the below is not hit. rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile); > > That is going to be a confusing reason code (tap device existed to get > here) and does not really explain this error. > > >> goto drop; >> + } >> >> if (!rcu_dereference(tun->steering_prog)) >> tun_automq_xmit(tun, skb); >> @@ -1078,19 +1081,27 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) >> /* Drop if the filter does not like it. >> * This is a noop if the filter is disabled. >> * Filter can be enabled only for the TAP devices. */ >> - if (!check_filter(&tun->txflt, skb)) >> + if (!check_filter(&tun->txflt, skb)) { >> + drop_reason = SKB_DROP_REASON_TAP_RUN_FILTER; > > just SKB_DROP_REASON_TAP_FILTER I will use SKB_DROP_REASON_TAP_FILTER. > >> goto drop; >> + } >> >> if (tfile->socket.sk->sk_filter && >> - sk_filter(tfile->socket.sk, skb)) >> + sk_filter(tfile->socket.sk, skb)) { >> + drop_reason = SKB_DROP_REASON_SKB_TRIM; > > SKB_DROP_REASON_SOCKET_FILTER Sorry for my mistake, I should have re-used this SKB_DROP_REASON_SOCKET_FILTER. > > The remainder of your changes feels like another variant of your > previous "function / line" reason code. You are creating new reason > codes for every goto failure with a code based name. The reason needs to > be the essence of the failure in a user friendly label. > The remainder are: - SKB_DROP_REASON_SKB_TRIM - SKB_DROP_REASON_SKB_ORPHAN_FRAGS - SKB_DROP_REASON_SKB_PULL - SKB_DROP_REASON_DEV_DOWN - SKB_DROP_REASON_SKB_COPY_DATA (introduced by Patch 1/2) I tried to make them self-explaining and re-usable to other developers. Yes, I am creating new reason codes for every goto failure with a code based name because each function might be failed due to many reasons. In addition, I need to avoid duplicate 'drop_reason' returned by a function in order to help developer identify the specific line of code that the sk_buff is dropped. Thank you very much! Dongli Zhang
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index fed85447701a..d67f2419dbb4 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) struct netdev_queue *queue; struct tun_file *tfile; int len = skb->len; + int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; rcu_read_lock(); tfile = rcu_dereference(tun->tfiles[txq]); /* Drop packet if interface is not attached */ - if (!tfile) + if (!tfile) { + drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED; goto drop; + } if (!rcu_dereference(tun->steering_prog)) tun_automq_xmit(tun, skb); @@ -1078,19 +1081,27 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) /* Drop if the filter does not like it. * This is a noop if the filter is disabled. * Filter can be enabled only for the TAP devices. */ - if (!check_filter(&tun->txflt, skb)) + if (!check_filter(&tun->txflt, skb)) { + drop_reason = SKB_DROP_REASON_TAP_RUN_FILTER; goto drop; + } if (tfile->socket.sk->sk_filter && - sk_filter(tfile->socket.sk, skb)) + sk_filter(tfile->socket.sk, skb)) { + drop_reason = SKB_DROP_REASON_SKB_TRIM; goto drop; + } len = run_ebpf_filter(tun, skb, len); - if (len == 0 || pskb_trim(skb, len)) + if (len == 0 || pskb_trim(skb, len)) { + drop_reason = SKB_DROP_REASON_SKB_TRIM; goto drop; + } - if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) + if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) { + drop_reason = SKB_DROP_REASON_SKB_ORPHAN_FRAGS; goto drop; + } skb_tx_timestamp(skb); @@ -1101,8 +1112,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) nf_reset_ct(skb); - if (ptr_ring_produce(&tfile->tx_ring, skb)) + if (ptr_ring_produce(&tfile->tx_ring, skb)) { + drop_reason = SKB_DROP_REASON_PTR_FULL; goto drop; + } /* NETIF_F_LLTX requires to do our own update of trans_start */ queue = netdev_get_tx_queue(dev, txq); @@ -1119,7 +1132,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) drop: atomic_long_inc(&dev->tx_dropped); skb_tx_error(skb); - kfree_skb(skb); + kfree_skb_reason(skb, drop_reason); rcu_read_unlock(); return NET_XMIT_DROP; } @@ -1717,6 +1730,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, u32 rxhash = 0; int skb_xdp = 1; bool frags = tun_napi_frags_enabled(tfile); + int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; if (!(tun->flags & IFF_NO_PI)) { if (len < sizeof(pi)) @@ -1820,9 +1834,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, if (err) { err = -EFAULT; + drop_reason = SKB_DROP_REASON_SKB_COPY_DATA; drop: atomic_long_inc(&tun->dev->rx_dropped); - kfree_skb(skb); + kfree_skb_reason(skb, drop_reason); if (frags) { tfile->napi.skb = NULL; mutex_unlock(&tfile->napi_mutex); @@ -1869,6 +1884,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, case IFF_TAP: if (frags && !pskb_may_pull(skb, ETH_HLEN)) { err = -ENOMEM; + drop_reason = SKB_DROP_REASON_SKB_PULL; goto drop; } skb->protocol = eth_type_trans(skb, tun->dev); @@ -1922,6 +1938,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, if (unlikely(!(tun->dev->flags & IFF_UP))) { err = -EIO; rcu_read_unlock(); + drop_reason = SKB_DROP_REASON_DEV_DOWN; goto drop; } diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 16c30d2e20dc..db2ef8e8d878 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -323,8 +323,14 @@ enum skb_drop_reason { SKB_DROP_REASON_SKB_GSO_SEGMENT, SKB_DROP_REASON_SKB_CHECKSUM, SKB_DROP_REASON_SKB_COPY_DATA, + SKB_DROP_REASON_SKB_TRIM, + SKB_DROP_REASON_SKB_ORPHAN_FRAGS, + SKB_DROP_REASON_SKB_PULL, + SKB_DROP_REASON_DEV_NOT_ATTACHED, + SKB_DROP_REASON_DEV_DOWN, SKB_DROP_REASON_PTR_FULL, SKB_DROP_REASON_VIRTNET_HDR, + SKB_DROP_REASON_TAP_RUN_FILTER, SKB_DROP_REASON_MAX, }; diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h index bf1509c31cea..03121373d2f0 100644 --- a/include/trace/events/skb.h +++ b/include/trace/events/skb.h @@ -19,8 +19,14 @@ EM(SKB_DROP_REASON_SKB_GSO_SEGMENT, SKB_GSO_SEGMENT) \ EM(SKB_DROP_REASON_SKB_CHECKSUM, SKB_CHECKSUM) \ EM(SKB_DROP_REASON_SKB_COPY_DATA, SKB_COPY_DATA) \ + EM(SKB_DROP_REASON_SKB_TRIM, SKB_TRIM) \ + EM(SKB_DROP_REASON_SKB_ORPHAN_FRAGS, SKB_ORPHAN_FRAGS) \ + EM(SKB_DROP_REASON_SKB_PULL, SKB_PULL) \ + EM(SKB_DROP_REASON_DEV_NOT_ATTACHED, DEV_NOT_ATTACHED) \ + EM(SKB_DROP_REASON_DEV_DOWN, DEV_DOWN) \ EM(SKB_DROP_REASON_PTR_FULL, PTR_FULL) \ EM(SKB_DROP_REASON_VIRTNET_HDR, VIRTNET_HDR) \ + EM(SKB_DROP_REASON_TAP_RUN_FILTER, TAP_RUN_FILTER) \ EMe(SKB_DROP_REASON_MAX, MAX) #undef EM
The TUN can be used as vhost-net backend. E.g, the tun_net_xmit() is the interface to forward the skb from TUN to vhost-net/virtio-net. However, there are many "goto drop" in the TUN 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. Cc: Joao Martins <joao.m.martins@oracle.com> Cc: Joe Jin <joe.jin@oracle.com> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- drivers/net/tun.c | 33 +++++++++++++++++++++++++-------- include/linux/skbuff.h | 6 ++++++ include/trace/events/skb.h | 6 ++++++ 3 files changed, 37 insertions(+), 8 deletions(-)