Re: [RFC net-next v1 07/12] virtio_net: refactor the xmit type

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

On Tue, 24 Sep 2024 15:35:03 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Sep 24, 2024 at 9:32 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > Because the af-xdp will introduce a new xmit type, so I refactor the
> > xmit type mechanism first.
> >
> > In general, pointers are aligned to 4 or 8 bytes.
>
> I think this needs some clarification, the alignment seems to depend
> on the lowest common multiple between the alignments of all struct
> members. So we know both xdp_frame and sk_buff are at least 4 bytes
> aligned.
>
> If we want to reuse the lowest bit of pointers in AF_XDP, the
> alignment of the data structure should be at least 4 bytes.

YES, for AF_XDP. See more in #10.


>
> > If it is aligned to 4
> > bytes, then only two bits are free for a pointer. But there are 4 types
> > here, so we can't use bits to distinguish them. And 2 bits is enough for
> > 4 types:
> >
> >     00 for skb
> >     01 for SKB_ORPHAN
> >     10 for XDP
> >     11 for af-xdp tx
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 90 +++++++++++++++++++++++-----------------
> >  1 file changed, 51 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 630e5b21ad69..41a5ea9b788d 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -45,9 +45,6 @@ module_param(napi_tx, bool, 0644);
> >  #define VIRTIO_XDP_TX          BIT(0)
> >  #define VIRTIO_XDP_REDIR       BIT(1)
> >
> > -#define VIRTIO_XDP_FLAG                BIT(0)
> > -#define VIRTIO_ORPHAN_FLAG     BIT(1)
> > -
> >  /* RX packet size EWMA. The average packet size is used to determine the packet
> >   * buffer size when refilling RX rings. As the entire RX ring may be refilled
> >   * at once, the weight is chosen so that the EWMA will be insensitive to short-
> > @@ -512,34 +509,35 @@ static struct sk_buff *virtnet_skb_append_frag(struct sk_buff *head_skb,
> >                                                struct page *page, void *buf,
> >                                                int len, int truesize);
> >
> > -static bool is_xdp_frame(void *ptr)
> > -{
> > -       return (unsigned long)ptr & VIRTIO_XDP_FLAG;
> > -}
> > +enum virtnet_xmit_type {
> > +       VIRTNET_XMIT_TYPE_SKB,
> > +       VIRTNET_XMIT_TYPE_SKB_ORPHAN,
> > +       VIRTNET_XMIT_TYPE_XDP,
> > +};
> >
> > -static void *xdp_to_ptr(struct xdp_frame *ptr)
> > -{
> > -       return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
> > -}
> > +/* We use the last two bits of the pointer to distinguish the xmit type. */
> > +#define VIRTNET_XMIT_TYPE_MASK (BIT(0) | BIT(1))
> >
> > -static struct xdp_frame *ptr_to_xdp(void *ptr)
> > +static enum virtnet_xmit_type virtnet_xmit_ptr_strip(void **ptr)
>
> Nit: not a native speaker but I think something like pack/unpack might
> be better.


Will fix.

Thanks

>
> With those changes.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks
>