Re: [Intel-wired-lan] [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU inline helpers

[Alexander Lobakin]

From: Jakub Kicinski <kuba@xxxxxxxxxx>
Date: Thu, 18 May 2023 07:56:43 -0700

> 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.

I'll definitely take a look, I also like the idea of minimalistic and
lightweight headers.
page_pool.h and page_pool_drv.h? :D

> 
>>>>  #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..

Ah, +, good point.

Thanks,
Olek