| Message ID | f82be2ee3ac7d18dd9982b5368a88a5bf2aeb777.1727977199.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Remove kstrdup_const() and kfree_const() usage | expand |
On Thu, 03 Oct 2024, Christophe JAILLET <christophe.jaillet@wanadoo.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? BR, Jani. [1] https://lore.kernel.org/r/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@wanadoo.fr > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.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; > }
Le 04/10/2024 à 11:35, Jani Nikula a écrit : > On Thu, 03 Oct 2024, Christophe JAILLET <christophe.jaillet@wanadoo.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. [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@wanadoo.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; >> } >
On Fri, 04 Oct 2024, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Le 04/10/2024 à 11:35, Jani Nikula a écrit : >> On Thu, 03 Oct 2024, Christophe JAILLET <christophe.jaillet@wanadoo.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@wanadoo.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; >>> } >> >
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; }
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()") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/gpu/drm/i915/display/intel_backlight.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)