Message-ID: <87iktkuxch.fsf@intel.com>
Date: Tue, 22 Oct 2024 15:38:06 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>, Rodrigo Vivi
 <rodrigo.vivi@...el.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>, Tvrtko Ursulin <tursulin@...ulin.net>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Cc: linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/i915/display: Remove kstrdup_const() and
 kfree_const() usage

On Fri, 04 Oct 2024, Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> Le 04/10/2024 à 11:35, Jani Nikula a écrit :
>> On Thu, 03 Oct 2024, Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
>>> kstrdup_const() and kfree_const() can be confusing in code built as a
>>> module. In such a case, it does not do what one could expect from the name
>>> of the functions.
>>>
>>> The code is not wrong by itself, but in such a case, it is equivalent to
>>> kstrdup() and kfree().
>>>
>>> So, keep thinks simple and straightforward.
>>>
>>> This reverts commit 379b63e7e682 ("drm/i915/display: Save a few bytes of
>>> memory in intel_backlight_device_register()")
>> 
>> Sorry, I guess I'm confused here. Or I just didn't read the commit
>> message to [1] properly. Or both.
>> 
>> So the whole point of [1] was that the _const versions can be confusing
>> if i915 is builtin? But not wrong?
>
> I'll try to explain the whole story and (try to) be clearer.

Thanks for the thorough explanations, pushed to drm-intel-next.

BR,
Jani.

>
>
> [2] the intent of this initial patch was a micro-optimization which was 
> expected to save a few bytes of memory. The naming of the function 
> looked promising. However kstrdup_const() only saves the allocation 
> within the rodata section of the kernel [5,6]. The mechanism does not 
> work for code built as module.
>
> This patch *is not* broken by itself, it is just pointless most of the 
> time. So keeping it as-is is just fine, from my point of view.
>
> If built as a module, kstrdup_const() is just a plain kstrdup() and 
> kfree_const() is just kfree().
>
>
>
> [3] was a variation that tried to avoid the allocation in all cases, 
> should it be built as a module or not.
> Being a micro-optimization of a slow path, your argument of keeping 
> things simple is just fine for me.
>
>
>
> [4] just revert [2].
> [2] was not broken, so [4] does not fix anything. It just makes things 
> simpler and as before.
>
>
> So the whole point of [1,3] was that the _const versions can be 
> confusing if i915 is *NOT* builtin.
> But it *is* not wrong, just likely useless in such a case.
>
> So, from my point of view, keeping [2] as is, or applying [3] or [4] on 
> top of it does not change things much, and each solution is correct.
>
>
>
> The idea behind removing some usage of _const() function in modules is 
> related to the patch proposal [7] and more precisely the response of 
> Christoph Hellwig [8]. The patch [7] will not be applied because it 
> breaks things.
> So, should this API be removed one day, or at least removed for modules, 
> the more preparation work is already done (up to now: 4,9,10] the better 
> it is.
>
> CJ
>
>
>
> [2]: 379b63e7e682 ("drm/i915/display: Save a few bytes of memory in 
> intel_backlight_device_register()")
>
> [3]: 
> https://lore.kernel.org/all/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@wanadoo.fr/
>
> [4]: 
> https://lore.kernel.org/all/f82be2ee3ac7d18dd9982b5368a88a5bf2aeb777.1727977199.git.christophe.jaillet@wanadoo.fr/
>
> [5]: https://elixir.bootlin.com/linux/v6.12-rc1/source/mm/util.c#L84
> [6]: 
> https://elixir.bootlin.com/linux/v6.12-rc1/source/include/asm-generic/sections.h#L177
>
> [7]: 
> https://lore.kernel.org/all/20240924050937.697118-1-senozhatsky@chromium.org/
> [8]: https://lore.kernel.org/all/ZvJfhDrv-eArtU8Y@infradead.org/
>
> [9]: 
> https://lore.kernel.org/all/63ac20f64234b7c9ea87a7fa9baf41e8255852f7.1727374631.git.christophe.jaillet@wanadoo.fr/
> [10]: 
> https://lore.kernel.org/all/06630f9ec3e153d0e7773b8d97a17e7c53e0d606.1727375615.git.christophe.jaillet@wanadoo.fr/
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> [1] https://lore.kernel.org/r/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@wanadoo.fr
>> 
>> 
>> 
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_backlight.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
>>> index 9e05745d797d..3f81a726cc7d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
>>> @@ -949,7 +949,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>>   	else
>>>   		props.power = BACKLIGHT_POWER_OFF;
>>>   
>>> -	name = kstrdup_const("intel_backlight", GFP_KERNEL);
>>> +	name = kstrdup("intel_backlight", GFP_KERNEL);
>>>   	if (!name)
>>>   		return -ENOMEM;
>>>   
>>> @@ -963,7 +963,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>>   		 * compatibility. Use unique names for subsequent backlight devices as a
>>>   		 * fallback when the default name already exists.
>>>   		 */
>>> -		kfree_const(name);
>>> +		kfree(name);
>>>   		name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>>>   				 i915->drm.primary->index, connector->base.name);
>>>   		if (!name)
>>> @@ -987,7 +987,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>>>   		    connector->base.base.id, connector->base.name, name);
>>>   
>>>   out:
>>> -	kfree_const(name);
>>> +	kfree(name);
>>>   
>>>   	return ret;
>>>   }
>> 
>

-- 
Jani Nikula, Intel

Powered by blists - more mailing lists