Re: [PATCH net] net: avoid 32 x truesize under-estimation for tiny skbs

[Jason Wang <jasowang@redhat.com>]

在 2021/4/2 下午7:00, Xuan Zhuo 写道:
> On Fri, 2 Apr 2021 10:52:12 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> So I wonder something like the following like this help. We know the
>> frag size, that means, if we know there's sufficient tailroom we can use
>> build_skb() without reserving dedicated room for skb_shared_info.
>>
>> Thanks
>>
>>
> Do you mean so?
>
> I have also considered this scenario, although build_skb is not always used, but
> it is also very good for the right situation.


Something like this. Would you mind to post a formal patch to net-next 
with some perf numbers?

Thanks


>
> Thanks.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index bb4ea9dbc16b..3db207c67422 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -383,17 +383,12 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   {
>   	struct sk_buff *skb;
>   	struct virtio_net_hdr_mrg_rxbuf *hdr;
> -	unsigned int copy, hdr_len, hdr_padded_len;
> -	char *p;
> +	unsigned int copy, hdr_len, hdr_padded_len, shinfo_size;
> +	char *p, *hdr_p;
>
>   	p = page_address(page) + offset;
> +	hdr_p = p;
>
> -	/* copy small packet so we can reuse these pages for small data */
> -	skb = napi_alloc_skb(&rq->napi, GOOD_COPY_LEN);
> -	if (unlikely(!skb))
> -		return NULL;
> -
> -	hdr = skb_vnet_hdr(skb);
>
>   	hdr_len = vi->hdr_len;
>   	if (vi->mergeable_rx_bufs)
> @@ -401,27 +396,57 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   	else
>   		hdr_padded_len = sizeof(struct padded_vnet_hdr);
>
> -	/* hdr_valid means no XDP, so we can copy the vnet header */
> -	if (hdr_valid)
> -		memcpy(hdr, p, hdr_len);
> -
>   	len -= hdr_len;
>   	offset += hdr_padded_len;
>   	p += hdr_padded_len;
>
> -	copy = len;
> -	if (copy > skb_tailroom(skb))
> -		copy = skb_tailroom(skb);
> -	skb_put_data(skb, p, copy);
> +	shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +
> +	if (truesize - len - hdr_len >= shinfo_size && len > GOOD_COPY_LEN) {
> +		skb = build_skb(p, truesize);
> +		if (unlikely(!skb))
> +			return NULL;
> +
> +		skb_put(skb, len);
> +
> +		/* hdr_valid means no XDP, so we can copy the vnet header */
> +		if (hdr_valid) {
> +			hdr = skb_vnet_hdr(skb);
> +			memcpy(hdr, hdr_p, hdr_len);
> +		}
> +
> +		if (metasize) {
> +			__skb_pull(skb, metasize);
> +			skb_metadata_set(skb, metasize);
> +		}
> +
> +		return skb;
> +	} else {
> +		/* copy small packet so we can reuse these pages for small data */
> +		skb = napi_alloc_skb(&rq->napi, GOOD_COPY_LEN);
> +		if (unlikely(!skb))
> +			return NULL;
> +
> +		copy = len;
> +		if (copy > skb_tailroom(skb))
> +			copy = skb_tailroom(skb);
> +		skb_put_data(skb, p, copy);
> +
> +		len -= copy;
> +		offset += copy;
> +	}
> +
> +	hdr = skb_vnet_hdr(skb);
> +
> +	/* hdr_valid means no XDP, so we can copy the vnet header */
> +	if (hdr_valid)
> +		memcpy(hdr, hdr_p, hdr_len);
>
>   	if (metasize) {
>   		__skb_pull(skb, metasize);
>   		skb_metadata_set(skb, metasize);
>   	}
>
> -	len -= copy;
> -	offset += copy;
> -
>   	if (vi->mergeable_rx_bufs) {
>   		if (len)
>   			skb_add_rx_frag(skb, 0, page, offset, len, truesize);
>