Received-SPF: pass (google.com: domain of linux-kernel+bounces-120426-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <b4f73aa3-1d47-47c9-993f-43a7a7f27d7e@intel.com>
Date: Wed, 27 Mar 2024 08:23:12 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: Re [patch RFC] mm/slab: introduce KZALLOC_FREE() cleanup-ed
 allocation macro
Content-Language: en-US
To: Dan Williams <dan.j.williams@intel.com>, <jiri@resnulli.us>
CC: <42.hyeyoo@gmail.com>, <akpm@linux-foundation.org>, <cl@linux.com>,
	<danielj@nvidia.com>, <iamjoonsoo.kim@lge.com>,
	<linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>, <mingo@kernel.org>,
	<penberg@kernel.org>, <peterz@infradead.org>, <rientjes@google.com>,
	<roman.gushchin@linux.dev>, <vbabka@suse.cz>, <torvalds@linux-foundation.org>
References: <20240315132249.2515468-1-jiri@resnulli.us>
 <20240321162648.23693-1-przemyslaw.kitszel@intel.com>
 <6601c9e161df3_2690d294e5@dwillia2-mobl3.amr.corp.intel.com.notmuch>
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <6601c9e161df3_2690d294e5@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R1I5eUFzR1RZekVEOTZLeDNUMGhTeXQ5YmJERk9xNmtXNmlXR09reERXOWI2?=
 =?utf-8?B?NlJ6MllvVmN2emVCSGw4YnViWFYvYmozQWpkLzVQejZHUVRibEs4d3g1aTNa?=
 =?utf-8?B?dTFyMGtJdkFBdkpIOTBWeFZXcHR4YTB5dW9uUFlDeER6bi9pRXJ2enpJS0Nz?=
 =?utf-8?B?STkvQXpiOXdLM0lnRkNvQVFwQzRjb05LRTVtQjFKOGFseFdOUzhQdXNxUW1G?=
 =?utf-8?B?S1ZvTndmSG01UTBUK0hvYlg1d3BzL0FFcHp6Zk5TZTJ4Y0tORjd3cWtYakNw?=
 =?utf-8?B?QXdEanZJeS9EeWxSbFlxOVdQakRSOWluN0hSRHF3YnVWMFl2MmdoNjFLSndD?=
 =?utf-8?B?ekdiSmtGaFRiS2NJeWNPTHg1V2tiWDM4dzZsS2RRbWUwajBvOTBpOUk3c1dw?=
 =?utf-8?B?SldNWmxLcy9VQjlrdTU5RVI4bXE3NXlYc2wwY3loMFV1dnRLckNxVVQybVJn?=
 =?utf-8?B?ZCtWUXk2SGxKbFBxUlYrNTNSbmI3b3lwdWtyOWgwTEY3VlNzQ24wclc2eVRZ?=
 =?utf-8?B?djVZaEZRQ1daL1JzV1FHWGpLZFRESFBDczgzS1A3QWlCdU9wVTBTWHY4ckVl?=
 =?utf-8?B?eGlPVUtrY3pPdE5ZV2lBRERJa1hSZUhraEZVSjA1a2ZsZmRjbU5EWFpYSitH?=
 =?utf-8?B?OFkyQ0Y1U3NmbVVNakJSaXhQR2IvYi8yYTB1Uk1KOTNmS2tHRWl0M2ZDT09R?=
 =?utf-8?B?bStwTGRjV3d5YWdYb28zUjRhQUQra0NZSzBWRnkyQjhtdEtuMkF3N20xN0ZT?=
 =?utf-8?B?dVg2Q0RqMUkyMmdhNjhkYVg3YXRyMkVpY1NVcCt3c2x6Tm5nd0J2WEJLWW95?=
 =?utf-8?B?bitsb0dKaUZmUEw4WWNSa3hBb0FQSkJ1d0JpbzljMXAyNURhM2FDdWtiQzVs?=
 =?utf-8?B?M2dDK1daQitNcHJIdzRFY0ZzSEFHTmQrMStIVzRyTlBvRFRKZnZoUFE1V2hD?=
 =?utf-8?B?WmNFUDdKOG9OTnNwazdIOSs1a3RONmpGbWdnbTFLQUVaL3JhY0IzQUJDMElm?=
 =?utf-8?B?Z3RDK1FsYjF0enpDOGdmYVd3UzI5c1ZkQWFUUmZjR3NoS2xUNUloeTJZbWow?=
 =?utf-8?B?b1lWMFc0Vld6b1RjRGF3WXNsYm43bjJ1aEtjOEcyV2JqSE9MVWVDeHVBQWMv?=
 =?utf-8?B?RkFLalhpS0k2OC9BaUNqekprWThZbWtGNnNqcTRBYmhmZWlrSGFqQ0FFWSsz?=
 =?utf-8?B?ekcveWE4VjhpVVNrTm1MWUprOXEzSUEwSWF0c29pNlEwcDZZS2VXdWV6RnRS?=
 =?utf-8?B?WEVDaSs0Q1lISFpRSnJ3bGp6V1R3VXduaUdZY3dSdi92MitTWC9CckZKM1ov?=
 =?utf-8?B?VC9XS3g1cjhqK3BNTHlPKzkybldlQ2xtY3JscWhhbTRLQVMyWk5OdGEyQzRV?=
 =?utf-8?B?dDdySGl6eCtTN3JKNjZ4MUNja1h6VXJBa3V1SktVUU11dFB3WWZGZjJCaVdw?=
 =?utf-8?B?eElFeEJTMnRDR1RISHQ4YjRoREpPSWtraFNiYk5XKzFkK2hvMzRCcVV1NjFK?=
 =?utf-8?B?MWVlVFA3QmIwS0d0aE9pTmNCZFFBRzdiQ05pWVJZRHVJWWdFSWZxbjBwbUFS?=
 =?utf-8?B?T0luSzNNTmQrajNweUNVZ3dnVG1WSi9ud0ZiakpvSEMzNHJSUDBVeGNDamI0?=
 =?utf-8?B?dHFyeFo1ZTJDYVJwQTdjUjlQcjhJREIxTzN0cDhxbjJ4YVJKcXFJdGQxMFZh?=
 =?utf-8?B?LzNGek1tTFVtWHNHRUNTRFNCcWhHL2dWTGt2NlNhdVhZc1hmV3hWdDdUUkFE?=
 =?utf-8?B?WlJoYnBjVmVUZzFvdk9HaFcxRWt2cDJRQVlZbXRhMk53eG9uT1NmU0oyUjZv?=
 =?utf-8?B?RGR5T2FOaFNzL0QxOTU3aGxiYmpzbHFJazlCZlhkTVJNVmlsb3hIY0NFTTdE?=
 =?utf-8?B?OHNHTXBva3dMZmRxTlBjRGdhbldpZllxVzlXVFl2YndtVForNlg5S3RnMFRU?=
 =?utf-8?B?c1JjN0FHTURSd0FUVi9FN3dVR09aQ20xZUlVN045SGw4VUpXaUM1YzRTQTNy?=
 =?utf-8?B?a2JDcnQ2YTZjRll6SmNFZzNocFVNKy91bEwzZmFsN2ozejE5YjdjaVgzWit1?=
 =?utf-8?B?RE11K1I1V21xamkxZXlNUkR6RVNuQ21YR0tzSTVhdjlDc3VyL3B6NVYwbjdK?=
 =?utf-8?B?dFhFTlRvak9aN2xoR2UzZm1NSW5PVGFJcm9MbGhsanFoeGxYN1JWVkw5ZUdz?=
 =?utf-8?B?L1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f62f67a-39cd-45c6-615e-08dc4e2ec61f
X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8102.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Mar 2024 07:23:18.1088
 (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: 5WCA4MrhoGzIWZwESJhKe7b0RpEoX6VHWEDR/zU/8ja87uRCbSuWZ0vAlacp0WrMe9iL/BXdS2Zzt9JNlydUIsnEIoAmzlw1/qHM61ubrNM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7100
X-OriginatorOrg: intel.com

On 3/25/24 20:00, Dan Williams wrote:
> Przemek Kitszel wrote:
>>> From: Jiri Pirko <jiri@nvidia.com>
>>>
>>> With introduction of __free() macro using cleanup infrastructure, it
>>> will very likely become quite common to see following pattern:
>>> 	type *var __free(kfree) = kzalloc(sizeof(*var), GFP_KERNEL);
>>>
>>> To follow the CLASS() flow from cleanup.h, introduce a simple macro
>>> KZALLOC_FREE() to wrap this over and allow the same flow.
>>>
>>> Show an example usage in gpio-sim driver.
>>>
>>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>>> ---
>>>   drivers/gpio/gpio-sim.c | 3 +--
>>>   include/linux/slab.h    | 3 +++
>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
>>> index c4106e37e6db..997237b3d80c 100644
>>> --- a/drivers/gpio/gpio-sim.c
>>> +++ b/drivers/gpio/gpio-sim.c
>>> @@ -1496,8 +1496,7 @@ gpio_sim_config_make_device_group(struct config_group *group, const char *name)
>>>   {
>>>   	int id;
>>>   
>>> -	struct gpio_sim_device *dev __free(kfree) = kzalloc(sizeof(*dev),
>>> -							    GFP_KERNEL);
>>> +	KZALLOC_FREE(struct gpio_sim_device *, dev, GFP_KERNEL);
>>>   	if (!dev)
>>>   		return ERR_PTR(-ENOMEM);
>>>   
>>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>>> index b5f5ee8308d0..baee6acd58d3 100644
>>> --- a/include/linux/slab.h
>>> +++ b/include/linux/slab.h
>>> @@ -711,6 +711,9 @@ static inline __alloc_size(1) void *kzalloc(size_t size, gfp_t flags)
>>>   	return kmalloc(size, flags | __GFP_ZERO);
>>>   }
>>>   
>>> +#define KZALLOC_FREE(_type, var, _gfp_t)				\
>>> +	_type var __free(kfree) = kzalloc(sizeof(*var), _gfp_t)
>>> +
>>
>> Nice, but I would rather see this wrapper in the cleanup.h file, that have all
>> of the rest of related stuff.
>>
>> On top of that, I want to propose also a wrapper that is simpler in that it
>> does not allocate but just assigns null, with that in mind `_FREE` part of your
>> proposed name does not sound right.
> 
> No, do not hide assignments within macros

As most general advice I agree, but here we have a specific case:
declare variable via macro; and that, (given the macro name would be
clearer), is expected to have assignment (or default (un)init).
I would even go one step further and remove also the asterisk from the
call site (and *hide* it in the macro definition).

See _DEFINE_FLEX() as example:
(there we change on-stack instead $this_thread on-heap-autofree)
https://elixir.bootlin.com/linux/v6.9-rc1/source/include/linux/overflow.h#L401 


> 
> http://lore.kernel.org/r/CAHk-=whYxkfLVtBW_B-PgNqhKOAThTbfoH5CxtOTkwOB6VOt6w@mail.gmail.com

Your thread is a more complex thing to what we have here.
And BTW, your original proposed solution is nice, and even if it hides
flow inside, it's almost obvious (the `return -EINTR` statement
is verbatim at call site). Allowing `else return -EINTR;` solution
proposed by @Linus is nicer, makes a good idiom, but is less obvious:
Imagine two developers that don't know the API (well), one writes:
`scoped_cond_guard(args);` and forgets to handle the error case,
the other by just looking at the code have no idea to append
`else handle_err();`.

> 
> I.e. the amount of incremenal cleverness that include/linux/cleanup.h
> will tolerate is low. Any helper should look like typical C