From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux.dev,
"Michael S. Tsirkin" <mst@redhat.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
netdev@vger.kernel.org
Subject: Re: [PATCH vhost 4/6] virtio_net: big mode support premapped
Date: Fri, 19 Apr 2024 16:14:02 +0800 [thread overview]
Message-ID: <1713514442.7951615-1-xuanzhuo@linux.alibaba.com> (raw)
In-Reply-To: <CACGkMEu-SXDrnUX2jcVepxOo-QYo7QhwDRs-4_Rrc3Pf1K1Q6g@mail.gmail.com>
On Fri, 19 Apr 2024 16:12:15 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Fri, Apr 19, 2024 at 3:28 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Fri, 19 Apr 2024 15:24:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Fri, Apr 19, 2024 at 3:07 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Fri, 19 Apr 2024 13:46:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Fri, Apr 19, 2024 at 12:23 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Fri, 19 Apr 2024 08:43:43 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > On Thu, Apr 18, 2024 at 4:35 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, 18 Apr 2024 14:25:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > On Thu, Apr 11, 2024 at 10:51 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > > > > >
> > > > > > > > > > In big mode, pre-mapping DMA is beneficial because if the pages are not
> > > > > > > > > > used, we can reuse them without needing to unmap and remap.
> > > > > > > > > >
> > > > > > > > > > We require space to store the DMA address. I use the page.dma_addr to
> > > > > > > > > > store the DMA address from the pp structure inside the page.
> > > > > > > > > >
> > > > > > > > > > Every page retrieved from get_a_page() is mapped, and its DMA address is
> > > > > > > > > > stored in page.dma_addr. When a page is returned to the chain, we check
> > > > > > > > > > the DMA status; if it is not mapped (potentially having been unmapped),
> > > > > > > > > > we remap it before returning it to the chain.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/net/virtio_net.c | 98 +++++++++++++++++++++++++++++++++-------
> > > > > > > > > > 1 file changed, 81 insertions(+), 17 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > > index 4446fb54de6d..7ea7e9bcd5d7 100644
> > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > @@ -50,6 +50,7 @@ module_param(napi_tx, bool, 0644);
> > > > > > > > > >
> > > > > > > > > > #define page_chain_next(p) ((struct page *)((p)->pp))
> > > > > > > > > > #define page_chain_add(p, n) ((p)->pp = (void *)n)
> > > > > > > > > > +#define page_dma_addr(p) ((p)->dma_addr)
> > > > > > > > > >
> > > > > > > > > > /* RX packet size EWMA. The average packet size is used to determine the packet
> > > > > > > > > > * buffer size when refilling RX rings. As the entire RX ring may be refilled
> > > > > > > > > > @@ -434,6 +435,46 @@ skb_vnet_common_hdr(struct sk_buff *skb)
> > > > > > > > > > return (struct virtio_net_common_hdr *)skb->cb;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +static void sg_fill_dma(struct scatterlist *sg, dma_addr_t addr, u32 len)
> > > > > > > > > > +{
> > > > > > > > > > + sg->dma_address = addr;
> > > > > > > > > > + sg->length = len;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void page_chain_unmap(struct receive_queue *rq, struct page *p)
> > > > > > > > > > +{
> > > > > > > > > > + virtqueue_dma_unmap_page_attrs(rq->vq, page_dma_addr(p), PAGE_SIZE,
> > > > > > > > > > + DMA_FROM_DEVICE, 0);
> > > > > > > > > > +
> > > > > > > > > > + page_dma_addr(p) = DMA_MAPPING_ERROR;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static int page_chain_map(struct receive_queue *rq, struct page *p)
> > > > > > > > > > +{
> > > > > > > > > > + dma_addr_t addr;
> > > > > > > > > > +
> > > > > > > > > > + addr = virtqueue_dma_map_page_attrs(rq->vq, p, 0, PAGE_SIZE, DMA_FROM_DEVICE, 0);
> > > > > > > > > > + if (virtqueue_dma_mapping_error(rq->vq, addr))
> > > > > > > > > > + return -ENOMEM;
> > > > > > > > > > +
> > > > > > > > > > + page_dma_addr(p) = addr;
> > > > > > > > > > + return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void page_chain_release(struct receive_queue *rq)
> > > > > > > > > > +{
> > > > > > > > > > + struct page *p, *n;
> > > > > > > > > > +
> > > > > > > > > > + for (p = rq->pages; p; p = n) {
> > > > > > > > > > + n = page_chain_next(p);
> > > > > > > > > > +
> > > > > > > > > > + page_chain_unmap(rq, p);
> > > > > > > > > > + __free_pages(p, 0);
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + rq->pages = NULL;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > /*
> > > > > > > > > > * put the whole most recent used list in the beginning for reuse
> > > > > > > > > > */
> > > > > > > > > > @@ -441,6 +482,13 @@ static void give_pages(struct receive_queue *rq, struct page *page)
> > > > > > > > > > {
> > > > > > > > > > struct page *end;
> > > > > > > > > >
> > > > > > > > > > + if (page_dma_addr(page) == DMA_MAPPING_ERROR) {
> > > > > > > > >
> > > > > > > > > This looks strange, the map should be done during allocation. Under
> > > > > > > > > which condition could we hit this?
> > > > > > > >
> > > > > > > > This first page is umapped before we call page_to_skb().
> > > > > > > > The page can be put back to the link in case of failure.
> > > > > > >
> > > > > > > See below.
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > + if (page_chain_map(rq, page)) {
> > > > > > > > > > + __free_pages(page, 0);
> > > > > > > > > > + return;
> > > > > > > > > > + }
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > /* Find end of list, sew whole thing into vi->rq.pages. */
> > > > > > > > > > for (end = page; page_chain_next(end); end = page_chain_next(end));
> > > > > > > > > >
> > > > > > > > > > @@ -456,8 +504,15 @@ static struct page *get_a_page(struct receive_queue *rq, gfp_t gfp_mask)
> > > > > > > > > > rq->pages = page_chain_next(p);
> > > > > > > > > > /* clear chain here, it is used to chain pages */
> > > > > > > > > > page_chain_add(p, NULL);
> > > > > > > > > > - } else
> > > > > > > > > > + } else {
> > > > > > > > > > p = alloc_page(gfp_mask);
> > > > > > > > > > +
> > > > > > > > > > + if (page_chain_map(rq, p)) {
> > > > > > > > > > + __free_pages(p, 0);
> > > > > > > > > > + return NULL;
> > > > > > > > > > + }
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > return p;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > @@ -613,8 +668,6 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > > > > > > > > return NULL;
> > > > > > > > > >
> > > > > > > > > > page = page_chain_next(page);
> > > > > > > > > > - if (page)
> > > > > > > > > > - give_pages(rq, page);
> > > > > > > > > > goto ok;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > @@ -640,6 +693,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > > > > > > > > skb_add_rx_frag(skb, 0, page, offset, len, truesize);
> > > > > > > > > > else
> > > > > > > > > > page_to_free = page;
> > > > > > > > > > + page = NULL;
> > > > > > > > > > goto ok;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > @@ -657,6 +711,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > > > > > > > > BUG_ON(offset >= PAGE_SIZE);
> > > > > > > > > > while (len) {
> > > > > > > > > > unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> > > > > > > > > > +
> > > > > > > > > > + /* unmap the page before using it. */
> > > > > > > > > > + if (!offset)
> > > > > > > > > > + page_chain_unmap(rq, page);
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > This sounds strange, do we need a virtqueue_sync_for_cpu() helper here?
> > > > > > > >
> > > > > > > > I think we do not need that. Because the umap api does it.
> > > > > > > > We do not work with DMA_SKIP_SYNC;
> > > > > > >
> > > > > > > Well, the problem is unmap is too heavyweight and it reduces the
> > > > > > > effort of trying to avoid map/umaps as much as possible.
> > > > > > >
> > > > > > > For example, for most of the case DMA sync is just a nop. And such
> > > > > > > unmap() cause strange code in give_pages() as we discuss above?
> > > > > >
> > > > > > YES. You are right. For the first page, we just need to sync for cpu.
> > > > > > And we do not need to check the dma status.
> > > > > > But here (in page_to_skb), we need to call unmap, because this page is put
> > > > > > to the skb.
> > > > >
> > > > > Right, but issue still,
> > > > >
> > > > > The only case that we may hit
> > > > >
> > > > > if (page_dma_addr(page) == DMA_MAPPING_ERROR)
> > > > >
> > > > > is when the packet is smaller than GOOD_COPY_LEN.
> > > > >
> > > > > So if we sync_for_cpu for the head page, we don't do:
> > > > >
> > > > > 1) unmap in the receive_big()
> > > > > 2) do snyc_for_cpu() just before skb_put_data(), so the page could be
> > > > > recycled to the pool without unmapping?
> > > >
> > > >
> > > > I do not get.
> > >
> > > I meant something like e1000_copybreak().
> > >
> > > >
> > > > I think we can remove the code "if (page_dma_addr(page) == DMA_MAPPING_ERROR)"
> > > > from give_pages(). We just do unmap when the page is leaving virtio-net.
> > >
> > > That's the point.
> > >
> > > >
> > > > >
> > > > > And I think we should do something similar for the mergeable case?
> > > >
> > > > Do what?
> > > >
> > > > We have used the sync api for mergeable case.
> > >
> > > Where?
> > >
> > > I see virtnet_rq_get_buf which did sync but it is done after the page_to_skb().
> >
> > What means "done"?
> >
> > Do you want to reuse the buffer?
>
> Nope, I think I misread the code. Mergeable buffers should be fine as
> the unmap were during virtnet_receive().
>
> But the code might needs some tweak in the future
>
> in virtnet_receive() we had:
>
> if (!vi->big_packets || vi->mergeable_rx_bufs) {
> virtnet_rq_get_buf
> receive_buf()
> } else {
> virtqueue_get_buf()
> }
>
> but there's another switch in receive_buf():
>
> if (vi->mergeable_rx_bufs)
> else if (vi->big_packets)
> else
> ...
>
> Which is kind of a mess somehow.
YES. I will change this in the AF_XDP patch set.
Thanks.
>
> Thanks
> >
> > Thanks.
> >
> > >
> > > >
> > > >
> > > > >
> > > > > Btw, I found one the misleading comment introduced by f80bd740cb7c9
> > > > >
> > > > > /* copy small packet so we can reuse these pages */
> > > > > if (!NET_IP_ALIGN && len > GOOD_COPY_LEN && tailroom >= shinfo_size) {
> > > > >
> > > > > We're not copying but building skb around the head page.
> > > >
> > > > Will fix.
> > > >
> > > > Thanks.
> > >
> > > Thanks
> > >
> > > >
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
next prev parent reply other threads:[~2024-04-19 8:15 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 2:51 [PATCH vhost 0/6] virtio_net: rx enable premapped mode by default Xuan Zhuo
2024-04-11 2:51 ` [PATCH vhost 1/6] virtio_ring: introduce dma map api for page Xuan Zhuo
2024-04-11 11:45 ` Alexander Lobakin
2024-04-12 3:48 ` Xuan Zhuo
2024-04-18 6:08 ` Jason Wang
2024-04-11 2:51 ` [PATCH vhost 2/6] virtio_ring: enable premapped mode whatever use_dma_api Xuan Zhuo
2024-04-18 6:09 ` Jason Wang
2024-04-18 6:13 ` Jason Wang
2024-04-11 2:51 ` [PATCH vhost 3/6] virtio_net: replace private by pp struct inside page Xuan Zhuo
2024-04-12 4:47 ` Jason Wang
2024-04-12 5:35 ` Xuan Zhuo
2024-04-12 5:49 ` Jason Wang
2024-04-12 6:02 ` Xuan Zhuo
2024-04-15 2:08 ` Xuan Zhuo
2024-04-15 6:43 ` Jason Wang
2024-04-15 8:36 ` Xuan Zhuo
2024-04-15 8:56 ` Jason Wang
2024-04-15 8:59 ` Xuan Zhuo
2024-04-16 3:24 ` Jason Wang
2024-04-17 1:30 ` Xuan Zhuo
2024-04-17 4:08 ` Jason Wang
2024-04-17 8:20 ` Xuan Zhuo
2024-04-18 4:15 ` Jason Wang
2024-04-18 4:16 ` Jason Wang
2024-04-18 20:19 ` Jesper Dangaard Brouer
2024-04-18 21:56 ` Matthew Wilcox
2024-04-19 7:11 ` Xuan Zhuo
2024-04-18 6:11 ` Jason Wang
2024-04-11 2:51 ` [PATCH vhost 4/6] virtio_net: big mode support premapped Xuan Zhuo
2024-04-11 16:34 ` kernel test robot
2024-04-11 20:11 ` kernel test robot
2024-04-14 9:48 ` Dan Carpenter
2024-04-18 6:25 ` Jason Wang
2024-04-18 8:29 ` Xuan Zhuo
2024-04-19 0:43 ` Jason Wang
2024-04-19 4:21 ` Xuan Zhuo
2024-04-19 5:46 ` Jason Wang
2024-04-19 7:03 ` Xuan Zhuo
2024-04-19 7:24 ` Jason Wang
2024-04-19 7:26 ` Xuan Zhuo
2024-04-19 8:12 ` Jason Wang
2024-04-19 8:14 ` Xuan Zhuo [this message]
2024-04-19 7:52 ` Xuan Zhuo
2024-04-11 2:51 ` [PATCH vhost 5/6] virtio_net: enable premapped by default Xuan Zhuo
2024-04-18 6:26 ` Jason Wang
2024-04-18 8:35 ` Xuan Zhuo
2024-04-19 0:44 ` Jason Wang
2024-04-11 2:51 ` [PATCH vhost 6/6] virtio_net: rx remove premapped failover code Xuan Zhuo
2024-04-18 6:31 ` Jason Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1713514442.7951615-1-xuanzhuo@linux.alibaba.com \
--to=xuanzhuo@linux.alibaba.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jasowang@redhat.com \
--cc=kuba@kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=virtualization@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).