lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <9feef136-7ff3-91a4-4198-237b07a91c0c@intel.com> Date: Thu, 18 May 2023 15:45:33 +0200 From: Alexander Lobakin <aleksander.lobakin@...el.com> To: Jakub Kicinski <kuba@...nel.org> CC: Jesper Dangaard Brouer <hawk@...nel.org>, Larysa Zaremba <larysa.zaremba@...el.com>, <netdev@...r.kernel.org>, Ilias Apalodimas <ilias.apalodimas@...aro.org>, <linux-kernel@...r.kernel.org>, "Christoph Hellwig" <hch@....de>, Eric Dumazet <edumazet@...gle.com>, Michal Kubiak <michal.kubiak@...el.com>, <intel-wired-lan@...ts.osuosl.org>, Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>, Magnus Karlsson <magnus.karlsson@...el.com> Subject: Re: [Intel-wired-lan] [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU inline helpers From: Jakub Kicinski <kuba@...nel.org> Date: Wed, 17 May 2023 21:12:11 -0700 > On Tue, 16 May 2023 18:18:37 +0200 Alexander Lobakin wrote: >> Each driver is responsible for syncing buffers written by HW for CPU >> before accessing them. Almost each PP-enabled driver uses the same >> pattern, which could be shorthanded into a static inline to make driver >> code a little bit more compact. >> Introduce a couple such functions. The first one takes the actual size >> of the data written by HW and is the main one to be used on Rx. The >> second does the same, but only if the PP performs DMA synchronizations >> at all. The last one picks max_len from the PP params and is designed >> for more extreme cases when the size is unknown, but the buffer still >> needs to be synced. >> Also constify pointer arguments of page_pool_get_dma_dir() and >> page_pool_get_dma_addr() to give a bit more room for optimization, >> as both of them are read-only. > > Very neat. > >> diff --git a/include/net/page_pool.h b/include/net/page_pool.h >> index 8435013de06e..f740c50b661f 100644 >> --- a/include/net/page_pool.h >> +++ b/include/net/page_pool.h >> @@ -32,7 +32,7 @@ >> >> #include <linux/mm.h> /* Needed by ptr_ring */ >> #include <linux/ptr_ring.h> >> -#include <linux/dma-direction.h> >> +#include <linux/dma-mapping.h> > > highly nit picky - but isn't dma-mapping.h pretty heavy? > And we include page_pool.h in skbuff.h. Not that it matters > today, but maybe one day we'll succeed putting skbuff.h > on a diet -- so perhaps it's better to put "inline helpers > with non-trivial dependencies" into a new header? Maybe we could rather stop including page_pool.h into skbuff.h? It's used there only for 1 external, which could be declared directly in skbuff.h. When Matteo was developing PP recycling, he was storing mem_info in skb as well, but then it was optimized and we don't do that anymore. It annoys sometimes to see the whole kernel rebuilt each time I edit pag_pool.h :D In fact, only PP-enabled drivers and core code need it. > >> #define PP_FLAG_DMA_MAP BIT(0) /* Should page_pool do the DMA >> * map/unmap > >> +/** >> + * page_pool_dma_sync_for_cpu - sync Rx page for CPU after it's written by HW >> + * @pool: page_pool which this page belongs to >> + * @page: page to sync >> + * @dma_sync_size: size of the data written to the page >> + * >> + * Can be used as a shorthand to sync Rx pages before accessing them in the >> + * driver. Caller must ensure the pool was created with %PP_FLAG_DMA_MAP. >> + */ >> +static inline void page_pool_dma_sync_for_cpu(const struct page_pool *pool, >> + const struct page *page, >> + u32 dma_sync_size) >> +{ >> + dma_sync_single_range_for_cpu(pool->p.dev, >> + page_pool_get_dma_addr(page), >> + pool->p.offset, dma_sync_size, >> + page_pool_get_dma_dir(pool)); > > Likely a dumb question but why does this exist? > Is there a case where the "maybe" version is not safe? If the driver doesn't set DMA_SYNC_DEV flag, then the "maybe" version will never do anything. But we may want to use these helpers in such drivers too? > >> +} >> + >> +/** >> + * page_pool_dma_maybe_sync_for_cpu - sync Rx page for CPU if needed >> + * @pool: page_pool which this page belongs to >> + * @page: page to sync >> + * @dma_sync_size: size of the data written to the page >> + * >> + * Performs DMA sync for CPU, but only when required (swiotlb, IOMMU etc.). >> + */ >> +static inline void >> +page_pool_dma_maybe_sync_for_cpu(const struct page_pool *pool, >> + const struct page *page, u32 dma_sync_size) >> +{ >> + if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV) >> + page_pool_dma_sync_for_cpu(pool, page, dma_sync_size); >> +} Thanks, Olek
Powered by blists - more mailing lists