Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6f8147ec-b8ad-3905-5279-16817ed6f5ae@intel.com>
Date:   Fri, 28 Jul 2023 16:03:53 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH net-next 5/9] page_pool: don't use driver-set flags field
 directly
Content-Language: en-US
To:     Yunsheng Lin <linyunsheng@huawei.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Alexander Duyck <alexanderduyck@fb.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        "Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
        Simon Horman <simon.horman@corigine.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230727144336.1646454-1-aleksander.lobakin@intel.com>
 <20230727144336.1646454-6-aleksander.lobakin@intel.com>
 <a0be882e-558a-9b1d-7514-0aad0080e08c@huawei.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <a0be882e-558a-9b1d-7514-0aad0080e08c@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ci9adENiMzQrbnU4SldJQ3M1SmdUZzNEU2c0cTB0a3FqSERhbi84VERoNTVT?=
 =?utf-8?B?ZUF6OFZhamxHNTlHWmFNRFVyTTBWZHFrVDd2N3A3ZkFZUmllTlo1bnV6L25r?=
 =?utf-8?B?NDU2elBHWjFZNkJBaW9BMVpGdTRuNlZXR0VUb2VNUGFmVzY3Z1RKaGp0Y2N6?=
 =?utf-8?B?djJhZWsranBEOHhnYVRpeWdJdUJKYVY2L29MUlYyQnJoK0gvbW5sWUpHVkUx?=
 =?utf-8?B?YTNTZVFWMmluU096RzZRbXc1UVVZZXlKUjYyL3EyZ09ueEJobGx1eThwcDRI?=
 =?utf-8?B?MzFLa1VhSERwZk93L21sbG95eFFuNWM5SFhsTTRqbTY4UTF5ZUtNUDc2VVNK?=
 =?utf-8?B?bE9vTnVLT2I0cENMMi9CZjJhQURVbkRUZnlwWmpmK05jTnhuU0xFRlFLdTZa?=
 =?utf-8?B?Zm9CbllUT0lkZ3daMk5GUGhZaGQ4bmpTdXU2YVZkcVNEQWF5NFFqWUJpQ3ph?=
 =?utf-8?B?S2h4UE1QVjg4bGJFNTIwREN3cGVOeFk5MHBEZXd0US9RazBKSlpBTS9QL2x6?=
 =?utf-8?B?eHZrR3l3WXV5aWgvc05lL3ZUaUlwT0oxN0g3TXFUYlp2dlpqaHJUWFYvNjhP?=
 =?utf-8?B?RFpTenBtVXBjOVFrR1Y5TDFHZE9lMExmTHlSUUJFbk1CcFVRem0wQ3ZwVEcv?=
 =?utf-8?B?MlRnTU1sOXByWmV1VzZldklSbG0weW9YaitYODRJbGtGNWpBamhtSWhtblVC?=
 =?utf-8?B?SmNYTXJZWVZRc2dYVE1vNjJDYVVPeSsvQVpHdEV0ZFN6bmZmTllkd1RYUDU2?=
 =?utf-8?B?WVprNzE4TUJJQk5wZ2NCeCtaVktlK1ZGWHNQSFpSQ0dlZzZyRzJsZzlDQU1v?=
 =?utf-8?B?R0tNL0tIYWFoQU4rVGhqMFE5T1h6TTBhaGpLWlRMb1hKTnRyU3B4OXlMbWJx?=
 =?utf-8?B?MFd1SzhmTWJBZS9jeFB2a2tZVGdia0JCazBDVkV5azlKT2IvcitTWjNMRk1P?=
 =?utf-8?B?czdJa3hkendCYlQ3WWZDUjFsTzhlNjU1R1NPTENmUFFoZXFLajlJbVpKYUQz?=
 =?utf-8?B?TC91WDUvNWJhKzlnNVB2WWRmcTY5QWU2VkxMTGprVURJVEtnR2tBbmYxSUZE?=
 =?utf-8?B?QXhQampha1A3NGd1RUROaFRHbHQrc1h2QVh2dERvMU1OdUNtNVRHbE5sOGZt?=
 =?utf-8?B?UUdtODd5RGZITHVGRnplMW9kQmdadE5ScnI1NFdrMFBpSHE3YXJSNEpoa3Bz?=
 =?utf-8?B?RG4xUjNjV296VkUvVGtKbFM0ZnllSHE5SndHMFR0dmE4K1BLQm9kV2tVS1Nn?=
 =?utf-8?B?Yk0wNTM3UWlkdTcrbTgxZU10Mnh1eFVKbnRuNUhzamcwMlZsUk1sWXJGUzQr?=
 =?utf-8?B?Vy9QUmFNZzZFNzdDeWR1M1M2aS9qbmdCUmtLamxnRXU0ek9mc1FwYVNIOWp3?=
 =?utf-8?B?Yml2WjR3Ni9jWFR5ZlBqV0R2eU9YekNTdDF6MVQ5S0EwMGkvZEs0d0cyZnZ5?=
 =?utf-8?B?TnFlQVcxUkVXVGRwbWVSYk50VlpYQ1ZzWU93aWltek9QUVJ6VjQwUFZUSnVO?=
 =?utf-8?B?YjhTTkQrb0ZPSzZvbmcvcWxmTnBpZXpwSWRkckVjbUF3ckcxcXR0M1hwTS84?=
 =?utf-8?B?c2pjcDZ6VWU2WldqeGFvZzQwZENTOHNUaEgxQ2JGN1ZNT2JjTlZoYXltY256?=
 =?utf-8?B?bmQxam1OMHVWZDZ3cFM1Z3lRdXlzZGZwMEpaVUpPZkJlL3o0Ny8zZklvakpa?=
 =?utf-8?B?M2hiQ0MwUERXVlhzSmlyOVFydyt1S08zZnJmbEpWTkx4Wm5RblNKVUVCVnRn?=
 =?utf-8?B?d1RMWWxneTVXT05Xekk3VFRBSFhHZDhCK055L01KK1dHZk5nbUVPM0F3S29M?=
 =?utf-8?B?L3lxYlVvU05zS1JYclIvbnM4dTl4Q3BhNmI1OVRvbW10VElGWjFOV2g1OHg0?=
 =?utf-8?B?eENaOGkwam4wZFdGdjVJQ1BqaDU0ZE9Fdlh3ejdHTmtHbnB1dFo2VUpYUCs0?=
 =?utf-8?B?MFR3MmdSRW93VGpEalhWek9VYkZTR3pjaWNhMTl3SGtJcHB4UTUzc04wZWV0?=
 =?utf-8?B?K2QzOGhaNnhYSzJCTFFvRDdkd05hbURNUmVUNnF4dkdpNjJxRkh2ajFEcVRG?=
 =?utf-8?B?clYvL04yNXgzdzhDTEp3dHJFNVNURXZqWU8xRTZZSGMzQkxVSmQyQkhOTWg0?=
 =?utf-8?B?T0ZsNjcxNDF2b1M2c3JtbHJ2citEZ0wrdDZLUkhWMmI0UmdxSzl3dlBaVjdt?=
 =?utf-8?B?aFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 23bf8009-92a8-46f9-e1a1-08db8f73b6ac
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jul 2023 14:05:35.4423
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0P3MbfGhLHo80QjtqnSpoYa3nNAZklEjRqYHZgwbwk45jKI3AVFG8zKK/bBND0iHgdp/JR1O3SKV4YFP5bgm+pzmIEchAiQt4HxAmwGIKNY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8740
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Yunsheng Lin <linyunsheng@huawei.com>
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