| Message ID | 1705490503-28844-1-git-send-email-wangyunjian@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] tun: add missing rx stats accounting in tun_xdp_act | expand |
Yunjian Wang wrote: > The TUN can be used as vhost-net backend, and it is necessary to count > the packets transmitted from TUN to vhost-net/virtio-net. However, there > are some places in the receive path that were not taken into account > when using XDP. The commit 8ae1aff0b331 ("tuntap: split out XDP logic") > only includes dropped counter for XDP_DROP, XDP_ABORTED, and invalid > XDP actions. It would be beneficial to also include new accounting for > successfully received bytes using dev_sw_netstats_rx_add and introduce > new dropped counter for XDP errors on XDP_TX and XDP_REDIRECT. From the description it is clear that these are two separate changes wrapped into one patch. I should have flagged this previously. Ack on returning the error counter that was previously present and matches the Fixes tag. For the second change, I had to check a few other XDP capable drivers to verify that it is indeed common to count such packets. > Fixes: 8ae1aff0b331 ("tuntap: split out XDP logic") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > v3: update commit log and code > v2: add Fixes tag > --- > drivers/net/tun.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index afa5497f7c35..0704a17e74e1 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1625,18 +1625,15 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile, > static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog, > struct xdp_buff *xdp, u32 act) > { > - int err; > + unsigned int datasize = xdp->data_end - xdp->data; > + int err = 0; > > switch (act) { > case XDP_REDIRECT: > err = xdp_do_redirect(tun->dev, xdp, xdp_prog); > - if (err) > - return err; > break; > case XDP_TX: > err = tun_xdp_tx(tun->dev, xdp); > - if (err < 0) > - return err; > break; > case XDP_PASS: > break; > @@ -1651,6 +1648,13 @@ static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog, > break; > } > > + if (err < 0) { > + act = err; > + dev_core_stats_rx_dropped_inc(tun->dev); > + } else if (act == XDP_REDIRECT || act == XDP_TX) { > + dev_sw_netstats_rx_add(tun->dev, datasize); > + } > + Let's avoid adding yet another branch and just do these operations in the case statements, like XDP_DROP. > return act; > } > > -- > 2.41.0 >
> -----Original Message----- > From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com] > Sent: Wednesday, January 17, 2024 11:42 PM > To: wangyunjian <wangyunjian@huawei.com>; > willemdebruijn.kernel@gmail.com; jasowang@redhat.com; kuba@kernel.org; > davem@davemloft.net > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; xudingke > <xudingke@huawei.com>; wangyunjian <wangyunjian@huawei.com> > Subject: Re: [PATCH net v3] tun: add missing rx stats accounting in tun_xdp_act > > Yunjian Wang wrote: > > The TUN can be used as vhost-net backend, and it is necessary to count > > the packets transmitted from TUN to vhost-net/virtio-net. However, > > there are some places in the receive path that were not taken into > > account when using XDP. The commit 8ae1aff0b331 ("tuntap: split out > > XDP logic") only includes dropped counter for XDP_DROP, XDP_ABORTED, > > and invalid XDP actions. It would be beneficial to also include new > > accounting for successfully received bytes using > > dev_sw_netstats_rx_add and introduce new dropped counter for XDP errors > on XDP_TX and XDP_REDIRECT. > > From the description it is clear that these are two separate changes wrapped > into one patch. I should have flagged this previously. Do I need to split these two modifications into 2 patches? 1. only fix dropped counter 2. add new accounting for successfully received bytes Or: Only fix dropped counter? > > Ack on returning the error counter that was previously present and matches > the Fixes tag. > > For the second change, I had to check a few other XDP capable drivers to verify > that it is indeed common to count such packets. > > > Fixes: 8ae1aff0b331 ("tuntap: split out XDP logic") > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > --- > > v3: update commit log and code > > v2: add Fixes tag > > --- > > drivers/net/tun.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c index > > afa5497f7c35..0704a17e74e1 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1625,18 +1625,15 @@ static struct sk_buff *__tun_build_skb(struct > > tun_file *tfile, static int tun_xdp_act(struct tun_struct *tun, struct > bpf_prog *xdp_prog, > > struct xdp_buff *xdp, u32 act) { > > - int err; > > + unsigned int datasize = xdp->data_end - xdp->data; > > + int err = 0; > > > > switch (act) { > > case XDP_REDIRECT: > > err = xdp_do_redirect(tun->dev, xdp, xdp_prog); > > - if (err) > > - return err; > > break; > > case XDP_TX: > > err = tun_xdp_tx(tun->dev, xdp); > > - if (err < 0) > > - return err; > > break; > > case XDP_PASS: > > break; > > @@ -1651,6 +1648,13 @@ static int tun_xdp_act(struct tun_struct *tun, > struct bpf_prog *xdp_prog, > > break; > > } > > > > + if (err < 0) { > > + act = err; > > + dev_core_stats_rx_dropped_inc(tun->dev); > > + } else if (act == XDP_REDIRECT || act == XDP_TX) { > > + dev_sw_netstats_rx_add(tun->dev, datasize); > > + } > > + > > Let's avoid adding yet another branch and just do these operations in the case > statements, like XDP_DROP. Fix it like this? --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1625,18 +1625,25 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile, static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog, struct xdp_buff *xdp, u32 act) { + unsigned int datasize = xdp->data_end - xdp->data; int err; switch (act) { case XDP_REDIRECT: err = xdp_do_redirect(tun->dev, xdp, xdp_prog); - if (err) + if (err) { + dev_core_stats_rx_dropped_inc(tun->dev); return err; + } + dev_sw_netstats_rx_add(tun->dev, datasize); break; case XDP_TX: err = tun_xdp_tx(tun->dev, xdp); - if (err < 0) + if (err < 0) { + dev_core_stats_rx_dropped_inc(tun->dev); return err; + } + dev_sw_netstats_rx_add(tun->dev, datasize); break; case XDP_PASS: > > > return act; > > } > > > > -- > > 2.41.0 > > > >
wangyunjian wrote: > > -----Original Message----- > > From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com] > > Sent: Wednesday, January 17, 2024 11:42 PM > > To: wangyunjian <wangyunjian@huawei.com>; > > willemdebruijn.kernel@gmail.com; jasowang@redhat.com; kuba@kernel.org; > > davem@davemloft.net > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; xudingke > > <xudingke@huawei.com>; wangyunjian <wangyunjian@huawei.com> > > Subject: Re: [PATCH net v3] tun: add missing rx stats accounting in tun_xdp_act > > > > Yunjian Wang wrote: > > > The TUN can be used as vhost-net backend, and it is necessary to count > > > the packets transmitted from TUN to vhost-net/virtio-net. However, > > > there are some places in the receive path that were not taken into > > > account when using XDP. The commit 8ae1aff0b331 ("tuntap: split out > > > XDP logic") only includes dropped counter for XDP_DROP, XDP_ABORTED, > > > and invalid XDP actions. It would be beneficial to also include new > > > accounting for successfully received bytes using > > > dev_sw_netstats_rx_add and introduce new dropped counter for XDP errors > > on XDP_TX and XDP_REDIRECT. > > > > From the description it is clear that these are two separate changes wrapped > > into one patch. I should have flagged this previously. > > Do I need to split these two modifications into 2 patches? > 1. only fix dropped counter > 2. add new accounting for successfully received bytes > Or: > Only fix dropped counter? It's definitely good to fix both. It might be a bit pedantic, but two separate patches is more correct. The second fix, add missing byte counter, goes back to the original introduction of XDP for tun, so has a different tag: Fixes: 761876c857cb ("tap: XDP support") > > > > > Ack on returning the error counter that was previously present and matches > > the Fixes tag. > > > > For the second change, I had to check a few other XDP capable drivers to verify > > that it is indeed common to count such packets. > > > > > Fixes: 8ae1aff0b331 ("tuntap: split out XDP logic") > > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > > --- > > > v3: update commit log and code > > > v2: add Fixes tag > > > --- > > > drivers/net/tun.c | 14 +++++++++----- > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c index > > > afa5497f7c35..0704a17e74e1 100644 > > > --- a/drivers/net/tun.c > > > +++ b/drivers/net/tun.c > > > @@ -1625,18 +1625,15 @@ static struct sk_buff *__tun_build_skb(struct > > > tun_file *tfile, static int tun_xdp_act(struct tun_struct *tun, struct > > bpf_prog *xdp_prog, > > > struct xdp_buff *xdp, u32 act) { > > > - int err; > > > + unsigned int datasize = xdp->data_end - xdp->data; > > > + int err = 0; > > > > > > switch (act) { > > > case XDP_REDIRECT: > > > err = xdp_do_redirect(tun->dev, xdp, xdp_prog); > > > - if (err) > > > - return err; > > > break; > > > case XDP_TX: > > > err = tun_xdp_tx(tun->dev, xdp); > > > - if (err < 0) > > > - return err; > > > break; > > > case XDP_PASS: > > > break; > > > @@ -1651,6 +1648,13 @@ static int tun_xdp_act(struct tun_struct *tun, > > struct bpf_prog *xdp_prog, > > > break; > > > } > > > > > > + if (err < 0) { > > > + act = err; > > > + dev_core_stats_rx_dropped_inc(tun->dev); > > > + } else if (act == XDP_REDIRECT || act == XDP_TX) { > > > + dev_sw_netstats_rx_add(tun->dev, datasize); > > > + } > > > + > > > > Let's avoid adding yet another branch and just do these operations in the case > > statements, like XDP_DROP. > > Fix it like this? Perhaps avoid computing datasize is all paths, when it is not used in common XDP_PASS, high performance XDP_DROP and a few others. Not sure whether (all) compilers would optimze that. dev_core_stats_rx_dropped_inc(tun->dev, xdp, xdp->data_end - xdp->data); > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1625,18 +1625,25 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile, > static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog, > struct xdp_buff *xdp, u32 act) > { > + unsigned int datasize = xdp->data_end - xdp->data; > int err; > > switch (act) { > case XDP_REDIRECT: > err = xdp_do_redirect(tun->dev, xdp, xdp_prog); > - if (err) > + if (err) { > + dev_core_stats_rx_dropped_inc(tun->dev); > return err; > + } > + dev_sw_netstats_rx_add(tun->dev, datasize); > break; > case XDP_TX: > err = tun_xdp_tx(tun->dev, xdp); > - if (err < 0) > + if (err < 0) { > + dev_core_stats_rx_dropped_inc(tun->dev); > return err; > + } > + dev_sw_netstats_rx_add(tun->dev, datasize); > break; > case XDP_PASS: > > > > > > return act; > > > } > > > > > > -- > > > 2.41.0 > > > > > > > >
> -----Original Message----- > From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com] > Sent: Thursday, January 18, 2024 10:44 PM > To: wangyunjian <wangyunjian@huawei.com>; Willem de Bruijn > <willemdebruijn.kernel@gmail.com>; jasowang@redhat.com; > kuba@kernel.org; davem@davemloft.net > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; xudingke > <xudingke@huawei.com> > Subject: RE: [PATCH net v3] tun: add missing rx stats accounting in tun_xdp_act > > wangyunjian wrote: > > > -----Original Message----- > > > From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com] > > > Sent: Wednesday, January 17, 2024 11:42 PM > > > To: wangyunjian <wangyunjian@huawei.com>; > > > willemdebruijn.kernel@gmail.com; jasowang@redhat.com; > > > kuba@kernel.org; davem@davemloft.net > > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; xudingke > > > <xudingke@huawei.com>; wangyunjian <wangyunjian@huawei.com> > > > Subject: Re: [PATCH net v3] tun: add missing rx stats accounting in > > > tun_xdp_act > > > > > > Yunjian Wang wrote: > > > > The TUN can be used as vhost-net backend, and it is necessary to > > > > count the packets transmitted from TUN to vhost-net/virtio-net. > > > > However, there are some places in the receive path that were not > > > > taken into account when using XDP. The commit 8ae1aff0b331 > > > > ("tuntap: split out XDP logic") only includes dropped counter for > > > > XDP_DROP, XDP_ABORTED, and invalid XDP actions. It would be > > > > beneficial to also include new accounting for successfully > > > > received bytes using dev_sw_netstats_rx_add and introduce new > > > > dropped counter for XDP errors > > > on XDP_TX and XDP_REDIRECT. > > > > > > From the description it is clear that these are two separate changes > > > wrapped into one patch. I should have flagged this previously. > > > > Do I need to split these two modifications into 2 patches? > > 1. only fix dropped counter > > 2. add new accounting for successfully received bytes > > Or: > > Only fix dropped counter? > > It's definitely good to fix both. > > It might be a bit pedantic, but two separate patches is more correct. > > The second fix, add missing byte counter, goes back to the original introduction > of XDP for tun, so has a different tag: > > Fixes: 761876c857cb ("tap: XDP support") OK, I will update it to 2 patches. Thanks > > > > > > > > > Ack on returning the error counter that was previously present and > > > matches the Fixes tag. > > > > > > For the second change, I had to check a few other XDP capable > > > drivers to verify that it is indeed common to count such packets. > > > > > > > Fixes: 8ae1aff0b331 ("tuntap: split out XDP logic") > > > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > > > --- > > > > v3: update commit log and code > > > > v2: add Fixes tag > > > > --- > > > > drivers/net/tun.c | 14 +++++++++----- > > > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c index > > > > afa5497f7c35..0704a17e74e1 100644 > > > > --- a/drivers/net/tun.c > > > > +++ b/drivers/net/tun.c > > > > @@ -1625,18 +1625,15 @@ static struct sk_buff > > > > *__tun_build_skb(struct tun_file *tfile, static int > > > > tun_xdp_act(struct tun_struct *tun, struct > > > bpf_prog *xdp_prog, > > > > struct xdp_buff *xdp, u32 act) { > > > > - int err; > > > > + unsigned int datasize = xdp->data_end - xdp->data; > > > > + int err = 0; > > > > > > > > switch (act) { > > > > case XDP_REDIRECT: > > > > err = xdp_do_redirect(tun->dev, xdp, xdp_prog); > > > > - if (err) > > > > - return err; > > > > break; > > > > case XDP_TX: > > > > err = tun_xdp_tx(tun->dev, xdp); > > > > - if (err < 0) > > > > - return err; > > > > break; > > > > case XDP_PASS: > > > > break; > > > > @@ -1651,6 +1648,13 @@ static int tun_xdp_act(struct tun_struct > > > > *tun, > > > struct bpf_prog *xdp_prog, > > > > break; > > > > } > > > > > > > > + if (err < 0) { > > > > + act = err; > > > > + dev_core_stats_rx_dropped_inc(tun->dev); > > > > + } else if (act == XDP_REDIRECT || act == XDP_TX) { > > > > + dev_sw_netstats_rx_add(tun->dev, datasize); > > > > + } > > > > + > > > > > > Let's avoid adding yet another branch and just do these operations > > > in the case statements, like XDP_DROP. > > > > Fix it like this? > > Perhaps avoid computing datasize is all paths, when it is not used in common > XDP_PASS, high performance XDP_DROP and a few others. Not sure whether > (all) compilers would optimze that. > > dev_core_stats_rx_dropped_inc(tun->dev, xdp, > xdp->data_end - xdp->data); OK, I agree. > > > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1625,18 +1625,25 @@ static struct sk_buff *__tun_build_skb(struct > > tun_file *tfile, static int tun_xdp_act(struct tun_struct *tun, struct > bpf_prog *xdp_prog, > > struct xdp_buff *xdp, u32 act) { > > + unsigned int datasize = xdp->data_end - xdp->data; > > int err; > > > > switch (act) { > > case XDP_REDIRECT: > > err = xdp_do_redirect(tun->dev, xdp, xdp_prog); > > - if (err) > > + if (err) { > > + dev_core_stats_rx_dropped_inc(tun->dev); > > return err; > > + } > > + dev_sw_netstats_rx_add(tun->dev, datasize); > > break; > > case XDP_TX: > > err = tun_xdp_tx(tun->dev, xdp); > > - if (err < 0) > > + if (err < 0) { > > + dev_core_stats_rx_dropped_inc(tun->dev); > > return err; > > + } > > + dev_sw_netstats_rx_add(tun->dev, datasize); > > break; > > case XDP_PASS: > > > > > > > > > return act; > > > > } > > > > > > > > -- > > > > 2.41.0 > > > > > > > > > > > > > >
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index afa5497f7c35..0704a17e74e1 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1625,18 +1625,15 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile, static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog, struct xdp_buff *xdp, u32 act) { - int err; + unsigned int datasize = xdp->data_end - xdp->data; + int err = 0; switch (act) { case XDP_REDIRECT: err = xdp_do_redirect(tun->dev, xdp, xdp_prog); - if (err) - return err; break; case XDP_TX: err = tun_xdp_tx(tun->dev, xdp); - if (err < 0) - return err; break; case XDP_PASS: break; @@ -1651,6 +1648,13 @@ static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog, break; } + if (err < 0) { + act = err; + dev_core_stats_rx_dropped_inc(tun->dev); + } else if (act == XDP_REDIRECT || act == XDP_TX) { + dev_sw_netstats_rx_add(tun->dev, datasize); + } + return act; }
The TUN can be used as vhost-net backend, and it is necessary to count the packets transmitted from TUN to vhost-net/virtio-net. However, there are some places in the receive path that were not taken into account when using XDP. The commit 8ae1aff0b331 ("tuntap: split out XDP logic") only includes dropped counter for XDP_DROP, XDP_ABORTED, and invalid XDP actions. It would be beneficial to also include new accounting for successfully received bytes using dev_sw_netstats_rx_add and introduce new dropped counter for XDP errors on XDP_TX and XDP_REDIRECT. Fixes: 8ae1aff0b331 ("tuntap: split out XDP logic") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> --- v3: update commit log and code v2: add Fixes tag --- drivers/net/tun.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)