Re: [PATCH net-next 5/9] page_pool: don't use driver-set flags field directly

[Alexander Lobakin]

From: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
Date: Fri, 28 Jul 2023 20:36:50 +0800

> On 2023/7/27 22:43, Alexander Lobakin wrote:
> 
>>  
>>  struct page_pool {
>>  	struct page_pool_params p;
>> -	long pad;
>> +
>> +	bool dma_map:1;				/* Perform DMA mapping */
>> +	enum {
>> +		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
>> +		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
>> +	} dma_sync_act:1;
>> +	bool page_frag:1;			/* Allow page fragments */
>>  
> 
> Isn't it more common or better to just remove the flags field in
> 'struct page_pool_params' and pass the flags by parameter like
> below, so that patch 4 is not needed?
> 
> struct page_pool *page_pool_create(const struct page_pool_params *params,
> 				   unsigned int	flags);

You would need a separate patch to convert all the page_pool_create()
users then either way.
And it doesn't look really natural to me to pass both driver-set params
and driver-set flags as separate function arguments. Someone may then
think "why aren't flags just put in the params itself". The fact that
Page Pool copies the whole params in the page_pool struct after
allocating it is internals, page_pool_create() prototype however isn't.
Thoughts?

Thanks,
Olek