Re: [PATCH 16/33] virtio_net: introduce virtnet_xdp_handler() to seprate the logic of run xdp

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Fri, 3 Feb 2023 03:55:26 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Feb 02, 2023 at 07:00:41PM +0800, Xuan Zhuo wrote:
> > At present, we have two long similar logic to perform XDP Progs. And in
> > the implementation of XSK, we will have this need.
> >
> > Therefore, this PATCH separates the code of executing XDP, which is
> > conducive to later maintenance and facilitates subsequent XSK for reuse.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> So you first add a new function then move users over.
> This means that it's hard during review to make sure
> nothing is lost in translation.
> Do the refactoring in a single patch instead.

I agree.

Thanks.

>
> > ---
> >  drivers/net/virtio/main.c       | 53 +++++++++++++++++++++++++++++++++
> >  drivers/net/virtio/virtio_net.h | 11 +++++++
> >  2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/net/virtio/main.c b/drivers/net/virtio/main.c
> > index 5683cb576474..9d4b84b23ef7 100644
> > --- a/drivers/net/virtio/main.c
> > +++ b/drivers/net/virtio/main.c
> > @@ -478,6 +478,59 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> >  	return ret;
> >  }
> >
> > +int virtnet_xdp_handler(struct bpf_prog *xdp_prog, struct xdp_buff *xdp,
> > +			struct net_device *dev,
> > +			unsigned int *xdp_xmit,
> > +			struct virtnet_rq_stats *stats)
> > +{
> > +	struct xdp_frame *xdpf;
> > +	int err;
> > +	u32 act;
> > +
> > +	act = bpf_prog_run_xdp(xdp_prog, xdp);
> > +	stats->xdp_packets++;
> > +
> > +	switch (act) {
> > +	case XDP_PASS:
> > +		return VIRTNET_XDP_RES_PASS;
> > +
> > +	case XDP_TX:
> > +		stats->xdp_tx++;
> > +		xdpf = xdp_convert_buff_to_frame(xdp);
> > +		if (unlikely(!xdpf))
> > +			return VIRTNET_XDP_RES_DROP;
> > +
> > +		err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> > +		if (unlikely(!err)) {
> > +			xdp_return_frame_rx_napi(xdpf);
> > +		} else if (unlikely(err < 0)) {
> > +			trace_xdp_exception(dev, xdp_prog, act);
> > +			return VIRTNET_XDP_RES_DROP;
> > +		}
> > +
> > +		*xdp_xmit |= VIRTIO_XDP_TX;
> > +		return VIRTNET_XDP_RES_CONSUMED;
> > +
> > +	case XDP_REDIRECT:
> > +		stats->xdp_redirects++;
> > +		err = xdp_do_redirect(dev, xdp, xdp_prog);
> > +		if (err)
> > +			return VIRTNET_XDP_RES_DROP;
> > +
> > +		*xdp_xmit |= VIRTIO_XDP_REDIR;
> > +		return VIRTNET_XDP_RES_CONSUMED;
> > +
> > +	default:
> > +		bpf_warn_invalid_xdp_action(dev, xdp_prog, act);
> > +		fallthrough;
> > +	case XDP_ABORTED:
> > +		trace_xdp_exception(dev, xdp_prog, act);
> > +		fallthrough;
> > +	case XDP_DROP:
> > +		return VIRTNET_XDP_RES_DROP;
> > +	}
> > +}
> > +
> >  static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
> >  {
> >  	return vi->xdp_enabled ? VIRTIO_XDP_HEADROOM : 0;
> > diff --git a/drivers/net/virtio/virtio_net.h b/drivers/net/virtio/virtio_net.h
> > index 8bf31429ae28..af3e7e817f9e 100644
> > --- a/drivers/net/virtio/virtio_net.h
> > +++ b/drivers/net/virtio/virtio_net.h
> > @@ -22,6 +22,12 @@
> >  #include <net/net_failover.h>
> >  #include <net/xdp_sock_drv.h>
> >
> > +enum {
> > +	VIRTNET_XDP_RES_PASS,
> > +	VIRTNET_XDP_RES_DROP,
> > +	VIRTNET_XDP_RES_CONSUMED,
> > +};
> > +
> >  #define VIRTIO_XDP_FLAG	BIT(0)
> >
> >  struct virtnet_info {
> > @@ -262,4 +268,9 @@ static void __free_old_xmit(struct send_queue *sq, bool in_napi,
> >  		stats->packets++;
> >  	}
> >  }
> > +
> > +int virtnet_xdp_handler(struct bpf_prog *xdp_prog, struct xdp_buff *xdp,
> > +			struct net_device *dev,
> > +			unsigned int *xdp_xmit,
> > +			struct virtnet_rq_stats *stats);
> >  #endif
> > --
> > 2.32.0.3.g01195cf9f
>