Jason Wang
2021-May-31 06:10 UTC
[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr
? 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 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-May-31 10:58 UTC
[PATCH net 2/2] virtio-net: get build_skb() buf by data ptr
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 dataThe 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. 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: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;