On Tue, 1 Jun 2021 11:27:12 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote: > > 在 2021/6/1 上午11:08, Xuan Zhuo 写道: > > On Tue, 1 Jun 2021 11:03:37 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote: > >> 在 2021/5/31 下午6:58, Xuan Zhuo 写道: > >>> On Mon, 31 May 2021 14:10:55 +0800, Jason Wang <jasowang@xxxxxxxxxx> 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 > > > Looks like you are right. > > See comments below. > > > > > > 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@xxxxxxxxxxxxxxxxx> > >>>>> Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx> > >>>>> --- > >>>>> 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); > > > I wonder why offset_in_page(p) is correct? I guess it should be: > > tailroom = truesize - len - headroom; > > The reason is that the buffer is not necessarily allocated at the page > boundary. In my understanding, offset_in_page(p) is the offset of p in the page where it is located. In this case, the two should be equal. This has nothing to do with which page is allocated. Of course I think using headroom is a good idea, and it is semantically better. Thanks. > > Thanks > > > >>>>> + buf = (char *)((unsigned long)p & PAGE_MASK); > >>>>> } else { > >>>>> tailroom = truesize - len; > >>>>> buf = p; > _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization