Messages in this thread Patch in this message | | | From | wangyunjian <> | Subject | RE: [PATCH net v3] tun: add missing rx stats accounting in tun_xdp_act | Date | Thu, 18 Jan 2024 12:55:26 +0000 |
| |
> -----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 > > > >
| |