Messages in this thread | | | Date | Thu, 18 May 2023 07:56:43 -0700 | From | Jakub Kicinski <> | Subject | Re: [Intel-wired-lan] [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU inline helpers |
| |
On Thu, 18 May 2023 15:45:33 +0200 Alexander Lobakin wrote: > >> 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.
Or maybe we can do both? I think that separating types, defines and simple wrappers from helpers should be considered good code hygiene.
> >> #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?
Oh, I see, the polarity of the flag is awkward. Hm. Maybe just rename things, drop the "maybe_" and prefix the non-checking version with __ ? We expect drivers to call the version which check the flag mostly (AFAIU), so it should have the most obvious name. Plus perhaps a sentence in the kdoc explaining why __ exists would be good, if it wasn't obvious to me it may not be obvious to others..
| |