From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-133.freemail.mail.aliyun.com (out30-133.freemail.mail.aliyun.com [115.124.30.133]) (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 BC570128369 for ; Wed, 17 Apr 2024 08:43:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=115.124.30.133 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713343428; cv=none; b=WPiiFVumNVEapUfjiSOshEl5nqQ3gjWbYAE2qrYwAmVqJcYnonidWBByNilzmrN3Z7tMRQnIsZ2Vl2xOG6sRfhSWFfDkldAvtAKaWjC9AhQjY2LaBvP6Jn/VNwX3OJy69ys2eMvDHmyhrLptQHZROL2S3P4y6FFSek7r7+Ob/1k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713343428; c=relaxed/simple; bh=SOXPVTmdIjLElqi0N/V3VvsZR5YPltP9iAFDZBVYaKw=; h=Message-ID:Subject:Date:From:To:Cc:References:In-Reply-To: Content-Type; b=J5sDT2520uTIf2ncXH9o2IcHT33MVJ8+NF795YtuSjTO4LrE/yUZG/4ZvLcCBkgCAg8aRD5TYDCXLlWlLmaRcoN0jgAtlQqwHdYGqrbK5cbxesoVfmBPRw6IBX41XgvXO91tTigdSr3AOPTIknjHvSTM1s2nZ8WcDSTv8nfrjPA= 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=EMy0QAdP; arc=none smtp.client-ip=115.124.30.133 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="EMy0QAdP" DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1713343418; h=Message-ID:Subject:Date:From:To:Content-Type; bh=y85km5+HMurzqVXhju9N5x2VHlJebnvhG955a1BKfUk=; b=EMy0QAdP8qKyMUjLiQ+ua9RkFmeoSF5d/EHX5lKDQHbUJDIuleRewgRvBx0Z5w8Z2nTvUxIOaS+3LDypveegBDYgTUWXpkB7viqFQpIH7Jcyo7EJsO80FQIIfcBwlH3p6jzzYLgH+ELmGR6VVaXaou5NCjgvXfYyFWZy4F47P3k= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R151e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018045176;MF=xuanzhuo@linux.alibaba.com;NM=1;PH=DS;RN=9;SR=0;TI=SMTPD_---0W4kxH6M_1713343416; Received: from localhost(mailfrom:xuanzhuo@linux.alibaba.com fp:SMTPD_---0W4kxH6M_1713343416) by smtp.aliyun-inc.com; Wed, 17 Apr 2024 16:43:37 +0800 Message-ID: <1713342055.436048-1-xuanzhuo@linux.alibaba.com> Subject: Re: [PATCH vhost 3/6] virtio_net: replace private by pp struct inside page Date: Wed, 17 Apr 2024 16:20:55 +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, Jesper Dangaard Brouer References: <20240411025127.51945-1-xuanzhuo@linux.alibaba.com> <20240411025127.51945-4-xuanzhuo@linux.alibaba.com> <1712900153.3715405-1-xuanzhuo@linux.alibaba.com> <1713146919.8867755-1-xuanzhuo@linux.alibaba.com> <1713170201.06163-2-xuanzhuo@linux.alibaba.com> <1713171554.2423792-1-xuanzhuo@linux.alibaba.com> <1713317444.7698638-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 Wed, 17 Apr 2024 12:08:10 +0800, Jason Wang wrote: > On Wed, Apr 17, 2024 at 9:38=E2=80=AFAM Xuan Zhuo wrote: > > > > On Tue, 16 Apr 2024 11:24:53 +0800, Jason Wang wr= ote: > > > On Mon, Apr 15, 2024 at 5:04=E2=80=AFPM Xuan Zhuo wrote: > > > > > > > > On Mon, 15 Apr 2024 16:56:45 +0800, Jason Wang wrote: > > > > > On Mon, Apr 15, 2024 at 4:50=E2=80=AFPM Xuan Zhuo wrote: > > > > > > > > > > > > On Mon, 15 Apr 2024 14:43:24 +0800, Jason Wang wrote: > > > > > > > On Mon, Apr 15, 2024 at 10:35=E2=80=AFAM Xuan Zhuo wrote: > > > > > > > > > > > > > > > > On Fri, 12 Apr 2024 13:49:12 +0800, Jason Wang wrote: > > > > > > > > > On Fri, Apr 12, 2024 at 1:39=E2=80=AFPM Xuan Zhuo wrote: > > > > > > > > > > > > > > > > > > > > On Fri, 12 Apr 2024 12:47:55 +0800, Jason Wang wrote: > > > > > > > > > > > On Thu, Apr 11, 2024 at 10:51=E2=80=AFAM Xuan Zhuo wrote: > > > > > > > > > > > > > > > > > > > > > > > > Now, we chain the pages of big mode by the page's p= rivate variable. > > > > > > > > > > > > But a subsequent patch aims to make the big mode to= support > > > > > > > > > > > > premapped mode. This requires additional space to s= tore the dma addr. > > > > > > > > > > > > > > > > > > > > > > > > Within the sub-struct that contains the 'private', = there is no suitable > > > > > > > > > > > > variable for storing the DMA addr. > > > > > > > > > > > > > > > > > > > > > > > > struct { /* Page cache and a= nonymous pages */ > > > > > > > > > > > > /** > > > > > > > > > > > > * @lru: Pageout list, eg. = active_list protected by > > > > > > > > > > > > * lruvec->lru_lock. Somet= imes used as a generic list > > > > > > > > > > > > * by the page owner. > > > > > > > > > > > > */ > > > > > > > > > > > > union { > > > > > > > > > > > > struct list_head lr= u; > > > > > > > > > > > > > > > > > > > > > > > > /* Or, for the Unev= ictable "LRU list" slot */ > > > > > > > > > > > > struct { > > > > > > > > > > > > /* Always e= ven, to negate PageTail */ > > > > > > > > > > > > void *__fil= ler; > > > > > > > > > > > > /* Count pa= ge's or folio's mlocks */ > > > > > > > > > > > > unsigned in= t mlock_count; > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > /* Or, free page */ > > > > > > > > > > > > struct list_head bu= ddy_list; > > > > > > > > > > > > struct list_head pc= p_list; > > > > > > > > > > > > }; > > > > > > > > > > > > /* See page-flags.h for PAG= E_MAPPING_FLAGS */ > > > > > > > > > > > > struct address_space *mappi= ng; > > > > > > > > > > > > union { > > > > > > > > > > > > pgoff_t index; = /* Our offset within mapping. */ > > > > > > > > > > > > unsigned long share= ; /* share count for fsdax */ > > > > > > > > > > > > }; > > > > > > > > > > > > /** > > > > > > > > > > > > * @private: Mapping-privat= e opaque data. > > > > > > > > > > > > * Usually used for buffer_= heads if PagePrivate. > > > > > > > > > > > > * Used for swp_entry_t if = PageSwapCache. > > > > > > > > > > > > * Indicates order in the b= uddy system if PageBuddy. > > > > > > > > > > > > */ > > > > > > > > > > > > unsigned long private; > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > But within the page pool struct, we have a variable= called > > > > > > > > > > > > dma_addr that is appropriate for storing dma addr. > > > > > > > > > > > > And that struct is used by netstack. That works to = our advantage. > > > > > > > > > > > > > > > > > > > > > > > > struct { /* page_pool used b= y netstack */ > > > > > > > > > > > > /** > > > > > > > > > > > > * @pp_magic: magic value t= o avoid recycling non > > > > > > > > > > > > * page_pool allocated page= s. > > > > > > > > > > > > */ > > > > > > > > > > > > unsigned long pp_magic; > > > > > > > > > > > > struct page_pool *pp; > > > > > > > > > > > > unsigned long _pp_mapping_p= ad; > > > > > > > > > > > > unsigned long dma_addr; > > > > > > > > > > > > atomic_long_t pp_ref_count; > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > On the other side, we should use variables from the= same sub-struct. > > > > > > > > > > > > So this patch replaces the "private" with "pp". > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > > > Instead of doing a customized version of page pool, c= an we simply > > > > > > > > > > > switch to use page pool for big mode instead? Then we= don't need to > > > > > > > > > > > bother the dma stuffs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The page pool needs to do the dma by the DMA APIs. > > > > > > > > > > So we can not use the page pool directly. > > > > > > > > > > > > > > > > > > I found this: > > > > > > > > > > > > > > > > > > define PP_FLAG_DMA_MAP BIT(0) /* Should page_pool= do the DMA > > > > > > > > > * map/unmap > > > > > > > > > > > > > > > > > > It seems to work here? > > > > > > > > > > > > > > > > > > > > > > > > I have studied the page pool mechanism and believe that we = cannot use it > > > > > > > > directly. We can make the page pool to bypass the DMA opera= tions. > > > > > > > > This allows us to handle DMA within virtio-net for pages al= located from the page > > > > > > > > pool. Furthermore, we can utilize page pool helpers to asso= ciate the DMA address > > > > > > > > to the page. > > > > > > > > > > > > > > > > However, the critical issue pertains to unmapping. Ideally,= we want to return > > > > > > > > the mapped pages to the page pool and reuse them. In doing = so, we can omit the > > > > > > > > unmapping and remapping steps. > > > > > > > > > > > > > > > > Currently, there's a caveat: when the page pool cache is fu= ll, it disconnects > > > > > > > > and releases the pages. When the pool hits its capacity, pa= ges are relinquished > > > > > > > > without a chance for unmapping. > > > > > > > > > > > > > > Technically, when ptr_ring is full there could be a fallback,= but then > > > > > > > it requires expensive synchronization between producer and co= nsumer. > > > > > > > For virtio-net, it might not be a problem because add/get has= been > > > > > > > synchronized. (It might be relaxed in the future, actually we= 've > > > > > > > already seen a requirement in the past for virito-blk). > > > > > > > > > > > > The point is that the page will be released by page pool direct= ly, > > > > > > we will have no change to unmap that, if we work with page pool. > > > > > > > > > > I mean if we have a fallback, there would be no need to release t= hese > > > > > pages but put them into a link list. > > > > > > > > > > > > What fallback? > > > > > > https://lore.kernel.org/netdev/1519607771-20613-1-git-send-email-mst@= redhat.com/ > > > > > > > > > > > If we put the pages to the link list, why we use the page pool? > > > > > > The size of the cache and ptr_ring needs to be fixed. > > > > > > Again, as explained above, it needs more benchmarks and looks like a > > > separate topic. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > If we were to unmap pages each time before > > > > > > > > returning them to the pool, we would negate the benefits of= bypassing the > > > > > > > > mapping and unmapping process altogether. > > > > > > > > > > > > > > Yes, but the problem in this approach is that it creates a co= rner > > > > > > > exception where dma_addr is used outside the page pool. > > > > > > > > > > > > YES. This is a corner exception. We need to introduce this case= to the page > > > > > > pool. > > > > > > > > > > > > So for introducing the page-pool to virtio-net(not only for big= mode), > > > > > > we may need to push the page-pool to support dma by drivers. > > > > > > > > > > Adding Jesper for some comments. > > > > > > > > > > > > > > > > > Back to this patch set, I think we should keep the virtio-net t= o manage > > > > > > the pages. > > > > > > > > > > > > What do you think? > > > > > > > > > > I might be wrong, but I think if we need to either > > > > > > > > > > 1) seek a way to manage the pages by yourself but not touching pa= ge > > > > > pool metadata (or Jesper is fine with this) > > > > > > > > Do you mean working with page pool or not? > > > > > > > > > > I meant if Jesper is fine with reusing page pool metadata like this p= atch. > > > > > > > If we manage the pages by self(no page pool), we do not care the me= tadata is for > > > > page pool or not. We just use the space of pages like the "private". > > > > > > That's also fine. > > > > > > > > > > > > > > > > 2) optimize the unmap for page pool > > > > > > > > > > or even > > > > > > > > > > 3) just do dma_unmap before returning the page back to the page p= ool, > > > > > we don't get all the benefits of page pool but we end up with sim= ple > > > > > codes (no fallback for premapping). > > > > > > > > I am ok for this. > > > > > > Right, we just need to make sure there's no performance regression, > > > then it would be fine. > > > > > > I see for example mana did this as well. > > > > I think we should not use page pool directly now, > > because the mana does not need a space to store the dma address. > > We need to store the dma address for unmapping. > > > > If we use page pool without PP_FLAG_DMA_MAP, then store the dma address= by > > page.dma_addr, I think that is not safe. > > Jesper, could you comment on this? > > > > > I think the way of this patch set is fine. > > So it reuses page pool structure in the page structure for another use ca= se. > > > We just use the > > space of the page whatever it is page pool or not to store > > the link and dma address. > > Probably because we've already "abused" page->private. I would leave > it for other maintainers to decide. If we do not want to use the elements of the page directly, the page pool is a good way. But we must make the page pool to work without PP_FLAG_DMA_MAP, because the virtio-net must use the DMA APIs wrapped by virtio core. And we still need to store the dma address, because virtio-net can not access the descs directly. @Jesper can we setup the page pool without PP_FLAG_DMA_MAP, and call page_pool_set_dma_addr() from the virtio-net driver? Thanks. > > Thanks > > > > > Thanks. > > > > > > > > Thanks > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > Maybe for big mode it doesn't matter too much if there's no > > > > > > > performance improvement. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >