From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-110.freemail.mail.aliyun.com (out30-110.freemail.mail.aliyun.com [115.124.30.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CAAA63B1AB for ; Fri, 19 Apr 2024 07:07:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=115.124.30.110 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713510468; cv=none; b=HQLKQthTN5vXofezY0NmwtwOJL6gMODH+Srgi/IQCmP50DnbLVO2/M0eEyhPLoqEgOmF66TBab6thlEjEimiCkIrpDlZgI4X/MG947RjXGEY5BuvK1FXcwEc13KZuPG2cSvEq8RI8E6wXVaSgto7S+kOpNVPiw8jLNYWcHer3eQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713510468; c=relaxed/simple; bh=Af3of//fhVVBCy0jgPsI0dnIT4lNEJzb8RaGBRwDQeg=; h=Message-ID:Subject:Date:From:To:Cc:References:In-Reply-To: Content-Type; b=SGe+JxkwJhQyg50d+n73HHrmSGasIMowvXXzAJqnlZ3id2s4jEsVtQMiXRk74s5ROpZWcMnXoVMfitUko+zNeQB3EsJWXr02C8F625r11d3jDsD8RgH+ngTwRE+h7PSWsLU2j5iAY6RxI3qMPqIu9E5/ncK7MoPpuZgLGn5IrH0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com; spf=pass smtp.mailfrom=linux.alibaba.com; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b=uuUEDZlq; arc=none smtp.client-ip=115.124.30.110 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b="uuUEDZlq" DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1713510457; h=Message-ID:Subject:Date:From:To:Content-Type; bh=fpUBZCIrFs1jQudHgNgNR5nOs2GDzJZXHL6D2IH1L2Y=; b=uuUEDZlq6GTXl6Tl5nrgkSZwTyKWo7euwVmQm0syhRiJX2uKnL4AWYK4EknzdhdwSvajlTjGknPPcKr8VTb1Rr7iPKaTn1FUNUc8gZ+iavpdP9J5VSknT1EH9faQ5981XDpqgDXQmKOH64dJEtTBlfri2DBNijDw1vDhTWatPsw= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R171e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046051;MF=xuanzhuo@linux.alibaba.com;NM=1;PH=DS;RN=8;SR=0;TI=SMTPD_---0W4rSZK6_1713510455; Received: from localhost(mailfrom:xuanzhuo@linux.alibaba.com fp:SMTPD_---0W4rSZK6_1713510455) by smtp.aliyun-inc.com; Fri, 19 Apr 2024 15:07:36 +0800 Message-ID: <1713510204.1357317-1-xuanzhuo@linux.alibaba.com> Subject: Re: [PATCH vhost 4/6] virtio_net: big mode support premapped Date: Fri, 19 Apr 2024 15:03:24 +0800 From: Xuan Zhuo To: Jason Wang Cc: virtualization@lists.linux.dev, "Michael S. Tsirkin" , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , netdev@vger.kernel.org References: <20240411025127.51945-1-xuanzhuo@linux.alibaba.com> <20240411025127.51945-5-xuanzhuo@linux.alibaba.com> <1713428960.80807-1-xuanzhuo@linux.alibaba.com> <1713500472.3614385-1-xuanzhuo@linux.alibaba.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk X-Mailing-List: virtualization@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: On Fri, 19 Apr 2024 13:46:25 +0800, Jason Wang wrote: > On Fri, Apr 19, 2024 at 12:23=E2=80=AFPM Xuan Zhuo wrote: > > > > On Fri, 19 Apr 2024 08:43:43 +0800, Jason Wang wr= ote: > > > On Thu, Apr 18, 2024 at 4:35=E2=80=AFPM Xuan Zhuo wrote: > > > > > > > > On Thu, 18 Apr 2024 14:25:06 +0800, Jason Wang wrote: > > > > > On Thu, Apr 11, 2024 at 10:51=E2=80=AFAM Xuan Zhuo 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_a= ddr to > > > > > > store the DMA address from the pp structure inside the page. > > > > > > > > > > > > Every page retrieved from get_a_page() is mapped, and its DMA a= ddress 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 un= mapped), > > > > > > we remap it before returning it to the chain. > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > --- > > > > > > 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 =3D (void *)n) > > > > > > +#define page_dma_addr(p) ((p)->dma_addr) > > > > > > > > > > > > /* RX packet size EWMA. The average packet size is used to det= ermine 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 add= r, u32 len) > > > > > > +{ > > > > > > + sg->dma_address =3D addr; > > > > > > + sg->length =3D 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) =3D DMA_MAPPING_ERROR; > > > > > > +} > > > > > > + > > > > > > +static int page_chain_map(struct receive_queue *rq, struct pag= e *p) > > > > > > +{ > > > > > > + dma_addr_t addr; > > > > > > + > > > > > > + addr =3D virtqueue_dma_map_page_attrs(rq->vq, p, 0, PAG= E_SIZE, DMA_FROM_DEVICE, 0); > > > > > > + if (virtqueue_dma_mapping_error(rq->vq, addr)) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + page_dma_addr(p) =3D addr; > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static void page_chain_release(struct receive_queue *rq) > > > > > > +{ > > > > > > + struct page *p, *n; > > > > > > + > > > > > > + for (p =3D rq->pages; p; p =3D n) { > > > > > > + n =3D page_chain_next(p); > > > > > > + > > > > > > + page_chain_unmap(rq, p); > > > > > > + __free_pages(p, 0); > > > > > > + } > > > > > > + > > > > > > + rq->pages =3D NULL; > > > > > > +} > > > > > > + > > > > > > /* > > > > > > * put the whole most recent used list in the beginning for re= use > > > > > > */ > > > > > > @@ -441,6 +482,13 @@ static void give_pages(struct receive_queu= e *rq, struct page *page) > > > > > > { > > > > > > struct page *end; > > > > > > > > > > > > + if (page_dma_addr(page) =3D=3D DMA_MAPPING_ERROR) { > > > > > > > > > > This looks strange, the map should be done during allocation. Und= er > > > > > 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 =3D page; page_chain_next(end); end =3D page_c= hain_next(end)); > > > > > > > > > > > > @@ -456,8 +504,15 @@ static struct page *get_a_page(struct rece= ive_queue *rq, gfp_t gfp_mask) > > > > > > rq->pages =3D page_chain_next(p); > > > > > > /* clear chain here, it is used to chain pages = */ > > > > > > page_chain_add(p, NULL); > > > > > > - } else > > > > > > + } else { > > > > > > p =3D 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 v= irtnet_info *vi, > > > > > > return NULL; > > > > > > > > > > > > page =3D page_chain_next(page); > > > > > > - if (page) > > > > > > - give_pages(rq, page); > > > > > > goto ok; > > > > > > } > > > > > > > > > > > > @@ -640,6 +693,7 @@ static struct sk_buff *page_to_skb(struct v= irtnet_info *vi, > > > > > > skb_add_rx_frag(skb, 0, page, offset, l= en, truesize); > > > > > > else > > > > > > page_to_free =3D page; > > > > > > + page =3D NULL; > > > > > > goto ok; > > > > > > } > > > > > > > > > > > > @@ -657,6 +711,11 @@ static struct sk_buff *page_to_skb(struct = virtnet_info *vi, > > > > > > BUG_ON(offset >=3D PAGE_SIZE); > > > > > > while (len) { > > > > > > unsigned int frag_size =3D min((unsigned)PAGE_S= IZE - 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) =3D=3D 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 think we can remove the code "if (page_dma_addr(page) =3D=3D DMA_MAPPING_= ERROR)" from give_pages(). We just do unmap when the page is leaving virtio-net. > > And I think we should do something similar for the mergeable case? Do what? We have used the sync api for mergeable case. > > 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 >=3D shinfo_= size) { > > We're not copying but building skb around the head page. Will fix. Thanks. > > Thanks > > > > > Thanks. > > > > > > > > > > Thanks > > > > > >