Re: [PATCH net-next v2 13/14] virtio_net: small: optimize code

[Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>]

On Thu, 20 Apr 2023 14:32:37 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Tue, Apr 18, 2023 at 2:53 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > Avoid the problem that some variables(headroom and so on) will repeat
> > the calculation when process xdp.
>
> While at it, if we agree to use separate code paths for building skbs.
>
> It would be better to have a helper for building skb for non XDP
> cases, then we can hide those calculation there.


Yes, we can introduce one helper, then receive_small will be more simple.
But these calculation can not shared with xdp case, because xdp case needs to
get these vars before running xdp.

Then the code "copy vnet hdr" and "set metadata" should stay in their own
function.

Only the virtnet_build_skb()[build_skb, skb_reserve, skb_put] can be shared.

static struct sk_buff *virtnet_build_skb(void *buf, unsigned int buflen,
					 unsigned int headroom,
					 unsigned int len)
{
	struct sk_buff *skb;

	skb = build_skb(buf, buflen);
	if (!skb)
		return NULL;

	skb_reserve(skb, headroom);
	skb_put(skb, len);

	return skb;
}

static struct sk_buff *receive_small_build_skb(struct virtnet_info *vi,
					       unsigned int xdp_headroom,
					       void *buf,
					       unsigned int len)
{
	unsigned int header_offset;
	unsigned int headroom;
	unsigned int buflen;
	struct sk_buff *skb;

	header_offset = VIRTNET_RX_PAD + xdp_headroom;
	headroom = vi->hdr_len + header_offset;
	buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

	skb = virtnet_build_skb(buf, buflen, headroom, len);
	if (unlikely(!skb))
		return NULL;

	buf += header_offset;
	memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);

	return skb;
}

Thanks

>
> Thanks
>
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/net/virtio_net.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index f6f5903face2..5a5636178bd3 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1040,11 +1040,10 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >         struct sk_buff *skb;
> >         struct bpf_prog *xdp_prog;
> >         unsigned int xdp_headroom = (unsigned long)ctx;
> > -       unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
> > -       unsigned int headroom = vi->hdr_len + header_offset;
> > -       unsigned int buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
> > -                             SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >         struct page *page = virt_to_head_page(buf);
> > +       unsigned int header_offset;
> > +       unsigned int headroom;
> > +       unsigned int buflen;
> >
> >         len -= vi->hdr_len;
> >         stats->bytes += len;
> > @@ -1072,6 +1071,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >         rcu_read_unlock();
> >
> >  skip_xdp:
> > +       header_offset = VIRTNET_RX_PAD + xdp_headroom;
> > +       headroom = vi->hdr_len + header_offset;
> > +       buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
> > +               SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > +
> >         skb = build_skb(buf, buflen);
> >         if (!skb)
> >                 goto err;
> > --
> > 2.32.0.3.g01195cf9f
> >
>