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

[Jason Wang <jasowang@redhat.com>]

在 2021/5/31 下午6:58, Xuan Zhuo 写道:
> On Mon, 31 May 2021 14:10:55 +0800, Jason Wang <jasowang@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@linux.alibaba.com>
>>> Acked-by: Michael S. Tsirkin <mst@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;