thr3ads.net - Virtualization - [PATCH net 2/2] virtio-net: get build

If this information is useful, please help other people find it:
Share via:

Jason Wang

2021-Jun-01 03:03 UTC

[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr

? 2021/5/31 ??6:58, Xuan Zhuo ??:> On Mon, 31 May 2021 14:10:55 +0800, Jason Wang <jasowang at
redhat.com> wrote:
>> ? 2021/5/14 ??11:16, Xuan Zhuo ??:
>>> In the case of merge, the page passed into page_to_skb() may be a
head
>>> page, not the page where the current data is located.
>>
>> I don't get how this can happen?
>>
>> Maybe you can explain a little bit more?
>>
>> receive_mergeable() call page_to_skb() in two places:
>>
>> 1) XDP_PASS for linearized page , in this case we use xdp_page
>> 2) page_to_skb() for "normal" page, in this case the page
contains the data
> The offset may be greater than PAGE_SIZE, because page is obtained by
> virt_to_head_page(), not the page where buf is located. And
"offset" is the offset
> of buf relative to page.
>
> 	tailroom = truesize - len - offset;
>
> In this case, the tailroom must be less than 0. Although there may be
enough
> content on this page to save skb_shared_info.

Interesting, I think we don't use compound pages for virtio-net. (We 
don't define SKB_FRAG_PAGE_ORDER).

Am I wrong?

Thanks

>
> Thanks.
>
>> Thanks
>>
>>
>>> So when trying to
>>> get the buf where the data is located, you should directly use the
>>> pointer(p) to get the address corresponding to the page.
>>>
>>> At the same time, the offset of the data in the page should also be
>>> obtained using offset_in_page().
>>>
>>> This patch solves this problem. But if you don?t use this patch,
the
>>> original code can also run, because if the page is not the page of
the
>>> current data, the calculated tailroom will be less than 0, and will
not
>>> enter the logic of build_skb() . The significance of this patch is
to
>>> modify this logical problem, allowing more situations to use
>>> build_skb().
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
>>> Acked-by: Michael S. Tsirkin <mst at redhat.com>
>>> ---
>>>    drivers/net/virtio_net.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 3e46c12dde08..073fec4c0df1 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -407,8 +407,12 @@ static struct sk_buff *page_to_skb(struct
virtnet_info *vi,
>>>    		 * see add_recvbuf_mergeable() + get_mergeable_buf_len()
>>>    		 */
>>>    		truesize = PAGE_SIZE;
>>> -		tailroom = truesize - len - offset;
>>> -		buf = page_address(page);
>>> +
>>> +		/* page maybe head page, so we should get the buf by p, not the
>>> +		 * page
>>> +		 */
>>> +		tailroom = truesize - len - offset_in_page(p);
>>> +		buf = (char *)((unsigned long)p & PAGE_MASK);
>>>    	} else {
>>>    		tailroom = truesize - len;
>>>    		buf = p;

Xuan Zhuo

2021-Jun-01 03:08 UTC

head link

[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr

On Tue, 1 Jun 2021 11:03:37 +0800, Jason Wang <jasowang at redhat.com>
wrote:>
> ? 2021/5/31 ??6:58, Xuan Zhuo ??:
> > On Mon, 31 May 2021 14:10:55 +0800, Jason Wang <jasowang at
redhat.com> wrote:
> >> ? 2021/5/14 ??11:16, Xuan Zhuo ??:
> >>> In the case of merge, the page passed into page_to_skb() may
be a head
> >>> page, not the page where the current data is located.
> >>
> >> I don't get how this can happen?
> >>
> >> Maybe you can explain a little bit more?
> >>
> >> receive_mergeable() call page_to_skb() in two places:
> >>
> >> 1) XDP_PASS for linearized page , in this case we use xdp_page
> >> 2) page_to_skb() for "normal" page, in this case the
page contains the data
> > The offset may be greater than PAGE_SIZE, because page is obtained by
> > virt_to_head_page(), not the page where buf is located. And
"offset" is the offset
> > of buf relative to page.
> >
> > 	tailroom = truesize - len - offset;
> >
> > In this case, the tailroom must be less than 0. Although there may be
enough
> > content on this page to save skb_shared_info.
>
>
> Interesting, I think we don't use compound pages for virtio-net. (We
> don't define SKB_FRAG_PAGE_ORDER).
>
> Am I wrong?

It seems to me that it seems to be a fixed setting, not for us to configure
independently

Thanks.

=========================================
net/sock.c

#define SKB_FRAG_PAGE_ORDER	get_order(32768)
DEFINE_STATIC_KEY_FALSE(net_high_order_alloc_disable_key);

/**
 * skb_page_frag_refill - check that a page_frag contains enough room
 * @sz: minimum size of the fragment we want to get
 * @pfrag: pointer to page_frag
 * @gfp: priority for memory allocation
 *
 * Note: While this allocator tries to use high order pages, there is
 * no guarantee that allocations succeed. Therefore, @sz MUST be
 * less or equal than PAGE_SIZE.
 */
bool skb_page_frag_refill(unsigned int sz, struct page_frag *pfrag, gfp_t gfp)
{
	if (pfrag->page) {
		if (page_ref_count(pfrag->page) == 1) {
			pfrag->offset = 0;
			return true;
		}
		if (pfrag->offset + sz <= pfrag->size)
			return true;
		put_page(pfrag->page);
	}

	pfrag->offset = 0;
	if (SKB_FRAG_PAGE_ORDER &&
	    !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
		/* Avoid direct reclaim but allow kswapd to wake */
		pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
					  __GFP_COMP | __GFP_NOWARN |
					  __GFP_NORETRY,
					  SKB_FRAG_PAGE_ORDER);
		if (likely(pfrag->page)) {
			pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
			return true;
		}
	}
	pfrag->page = alloc_page(gfp);
	if (likely(pfrag->page)) {
		pfrag->size = PAGE_SIZE;
		return true;
	}
	return false;
}
EXPORT_SYMBOL(skb_page_frag_refill);

>
> Thanks
>
>
> >
> > Thanks.
> >
> >> Thanks
> >>
> >>
> >>> So when trying to
> >>> get the buf where the data is located, you should directly use
the
> >>> pointer(p) to get the address corresponding to the page.
> >>>
> >>> At the same time, the offset of the data in the page should
also be
> >>> obtained using offset_in_page().
> >>>
> >>> This patch solves this problem. But if you don?t use this
patch, the
> >>> original code can also run, because if the page is not the
page of the
> >>> current data, the calculated tailroom will be less than 0, and
will not
> >>> enter the logic of build_skb() . The significance of this
patch is to
> >>> modify this logical problem, allowing more situations to use
> >>> build_skb().
> >>>
> >>> Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
> >>> Acked-by: Michael S. Tsirkin <mst at redhat.com>
> >>> ---
> >>>    drivers/net/virtio_net.c | 8 ++++++--
> >>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
> >>> index 3e46c12dde08..073fec4c0df1 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -407,8 +407,12 @@ static struct sk_buff *page_to_skb(struct
virtnet_info *vi,
> >>>    		 * see add_recvbuf_mergeable() + get_mergeable_buf_len()
> >>>    		 */
> >>>    		truesize = PAGE_SIZE;
> >>> -		tailroom = truesize - len - offset;
> >>> -		buf = page_address(page);
> >>> +
> >>> +		/* page maybe head page, so we should get the buf by p, not
the
> >>> +		 * page
> >>> +		 */
> >>> +		tailroom = truesize - len - offset_in_page(p);
> >>> +		buf = (char *)((unsigned long)p & PAGE_MASK);
> >>>    	} else {
> >>>    		tailroom = truesize - len;
> >>>    		buf = p;
>

Virtualization - Jun 2021 - [PATCH net 2/2] virtio-net: get build_skb() buf by data ptr

[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr

[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr