Xuan Zhuo
2021-Jun-01 03:08 UTC
[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; >
Jason Wang
2021-Jun-01 03:27 UTC
[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr
? 2021/6/1 ??11:08, Xuan Zhuo ??:> 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 > independentlyLooks 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 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);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. Thanks>>>>> + buf = (char *)((unsigned long)p & PAGE_MASK); >>>>> } else { >>>>> tailroom = truesize - len; >>>>> buf = p;