* Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h
2025-02-04 5:22 ` [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h Shyam Saini
@ 2025-02-04 5:27 ` Christoph Hellwig
2025-02-04 9:28 ` Christophe Leroy
2025-02-05 8:43 ` Rasmus Villemoes
2 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2025-02-04 5:27 UTC (permalink / raw)
To: Shyam Saini
Cc: linux-kernel, linux-modules, code, linux, mcgrof, frkaya, vijayb,
petr.pavlu, linux, samitolvanen, da.gomez, gregkh, rafael, dakr
> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
Crazily unreadable line.
Either way this is not a function that should be inline in a header.
Both due to sheer size, but also due to what it does and what it pulls
in.
> +{
> + struct module_kobject *mk;
> + struct kobject *kobj;
> + int err;
> +
> + kobj = kset_find_obj(module_kset, name);
> + if (kobj) {
> + mk = to_module_kobject(kobj);
> + } else {
And I know this is just moving the code, but an ealy return here
would significanty clean up the function..
> + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> + BUG_ON(!mk);
.. and a BUG_ON on allocation failure is a no-go.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h
2025-02-04 5:22 ` [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h Shyam Saini
2025-02-04 5:27 ` Christoph Hellwig
@ 2025-02-04 9:28 ` Christophe Leroy
2025-02-05 8:43 ` Rasmus Villemoes
2 siblings, 0 replies; 8+ messages in thread
From: Christophe Leroy @ 2025-02-04 9:28 UTC (permalink / raw)
To: Shyam Saini, linux-kernel, linux-modules
Cc: code, linux, mcgrof, frkaya, vijayb, petr.pavlu, linux,
samitolvanen, da.gomez, gregkh, rafael, dakr
Le 04/02/2025 à 06:22, Shyam Saini a écrit :
> [Vous ne recevez pas souvent de courriers de shyamsaini@linux.microsoft.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
>
> Move the following to module.h to allow common usages:
> - lookup_or_create_module_kobject()
> - to_module_attr
> - to_module_kobject
>
> This makes lookup_or_create_module_kobject() as inline.
Why an inline ? It looks quite big for an inline. Why not just make it
global ?
>
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> ---
> include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
> kernel/params.c | 39 ---------------------------------------
> 2 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..ba5f5e6c3927 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
> #endif /* CONFIG_MODULES */
>
> #ifdef CONFIG_SYSFS
> +/* sysfs output in /sys/modules/XYZ/parameters/ */
> +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> extern struct kset *module_kset;
> extern const struct kobj_type module_ktype;
> +
> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
> +{
> + struct module_kobject *mk;
> + struct kobject *kobj;
> + int err;
> +
> + kobj = kset_find_obj(module_kset, name);
> + if (kobj) {
> + mk = to_module_kobject(kobj);
Would look cleaner without the else. Something like:
if (kobj)
return to_module_kobject(kobj);
mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
...
> + } else {
> + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> + BUG_ON(!mk);
Why a BUG_ON() ? Why not just return NULL and let the caller handle the
failure ?
> +
> + mk->mod = THIS_MODULE;
> + mk->kobj.kset = module_kset;
> + err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> + "%s", name);
> +#ifdef CONFIG_MODULES
This #ifdef is not needed, module_uevent is declared at all time in
module.h, IS_ENABLED(CONFIG_MODULES) should be enough.
> + if (!err)
> + err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> +#endif
> + if (err) {
> + kobject_put(&mk->kobj);
> + pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> + name, err);
> + return NULL;
> + }
> +
> + /* So that we hold reference in both cases. */
> + kobject_get(&mk->kobj);
> + }
> +
> + return mk;
> +}
> +
> #endif /* CONFIG_SYSFS */
>
> #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
> diff --git a/kernel/params.c b/kernel/params.c
> index 4b43baaf7c83..3c0798b79f76 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -537,10 +537,6 @@ const struct kernel_param_ops param_ops_string = {
> };
> EXPORT_SYMBOL(param_ops_string);
>
> -/* sysfs output in /sys/modules/XYZ/parameters/ */
> -#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> -#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> -
> struct param_attribute
> {
> struct module_attribute mattr;
> @@ -763,41 +759,6 @@ void destroy_params(const struct kernel_param *params, unsigned num)
> params[i].ops->free(params[i].arg);
> }
>
> -static struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
> -{
> - struct module_kobject *mk;
> - struct kobject *kobj;
> - int err;
> -
> - kobj = kset_find_obj(module_kset, name);
> - if (kobj) {
> - mk = to_module_kobject(kobj);
> - } else {
> - mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> - BUG_ON(!mk);
> -
> - mk->mod = THIS_MODULE;
> - mk->kobj.kset = module_kset;
> - err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> - "%s", name);
> -#ifdef CONFIG_MODULES
> - if (!err)
> - err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> -#endif
> - if (err) {
> - kobject_put(&mk->kobj);
> - pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> - name, err);
> - return NULL;
> - }
> -
> - /* So that we hold reference in both cases. */
> - kobject_get(&mk->kobj);
> - }
> -
> - return mk;
> -}
> -
> static void __init kernel_add_sysfs_param(const char *name,
> const struct kernel_param *kparam,
> unsigned int name_skip)
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h
2025-02-04 5:22 ` [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h Shyam Saini
2025-02-04 5:27 ` Christoph Hellwig
2025-02-04 9:28 ` Christophe Leroy
@ 2025-02-05 8:43 ` Rasmus Villemoes
2025-02-07 5:43 ` Shyam Saini
2 siblings, 1 reply; 8+ messages in thread
From: Rasmus Villemoes @ 2025-02-05 8:43 UTC (permalink / raw)
To: Shyam Saini
Cc: linux-kernel, linux-modules, code, mcgrof, frkaya, vijayb,
petr.pavlu, linux, samitolvanen, da.gomez, gregkh, rafael, dakr
On Mon, Feb 03 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote:
> Move the following to module.h to allow common usages:
> - lookup_or_create_module_kobject()
> - to_module_attr
> - to_module_kobject
>
> This makes lookup_or_create_module_kobject() as inline.
>
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> ---
> include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
> kernel/params.c | 39 ---------------------------------------
> 2 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..ba5f5e6c3927 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
> #endif /* CONFIG_MODULES */
>
> #ifdef CONFIG_SYSFS
> +/* sysfs output in /sys/modules/XYZ/parameters/ */
> +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> extern struct kset *module_kset;
> extern const struct kobj_type module_ktype;
> +
> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
As others have said, this is way too big for an inline. Also, __init is
at best harmless (if it is indeed inlined into all callers), but if for
whatever reason the compiler decides to emit an OOL version, we
definitely do not want that in .init.text as we are now calling it from
non-init code.
> +{
> + struct module_kobject *mk;
> + struct kobject *kobj;
> + int err;
> +
> + kobj = kset_find_obj(module_kset, name);
> + if (kobj) {
> + mk = to_module_kobject(kobj);
> + } else {
> + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> + BUG_ON(!mk);
> +
And while the BUG_ON() is borderline acceptable in the current,
only-used-during-init, state, that definitely must go away once the
function can be called from non-init code.
This is what I alluded to when I said that the function might need some
refactoring before module_add_driver can start using it.
I'd say that the BUG_ON can simply be removed and replaced by a return
NULL; an "impossible" error case that can already be hit by some of the
code below, so callers have to be prepared for it anyway. If the
allocation fails (during early boot or later), the allocator already
makes a lot of noise, so there's no reason to even do a WARN_ON, and
since it "only" affects certain /sys objects, it's probably better to
let the machine try to complete boot instead of killing it.
Also, I agree with the suggestion to drop/outdent the whole else{} branch by
changing the if() to do "return to_module_kobject(kobj);".
To summarize:
- refactor to do an early return
- drop BUG_ON, replace by return NULL.
- drop static and __init, perhaps change __init to __modinit (which is a
pre-existing #define in params.c, so the function remains __init if
!CONFIG_MODULES), add an appropriate declaration somewhere, and if you
like, also do the rename
- make use of it from module_add_driver()
Rasmus
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h
2025-02-05 8:43 ` Rasmus Villemoes
@ 2025-02-07 5:43 ` Shyam Saini
0 siblings, 0 replies; 8+ messages in thread
From: Shyam Saini @ 2025-02-07 5:43 UTC (permalink / raw)
To: Rasmus Villemoes, hch, christophe.leroy
Cc: linux-kernel, linux-modules, code, mcgrof, frkaya, vijayb,
petr.pavlu, linux, samitolvanen, da.gomez, gregkh, rafael, dakr
Hi Everyone,
On Wed, Feb 05, 2025 at 09:43:12AM +0100, Rasmus Villemoes wrote:
> On Mon, Feb 03 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote:
>
> > Move the following to module.h to allow common usages:
> > - lookup_or_create_module_kobject()
> > - to_module_attr
> > - to_module_kobject
> >
> > This makes lookup_or_create_module_kobject() as inline.
> >
> > Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> > ---
> > include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
> > kernel/params.c | 39 ---------------------------------------
> > 2 files changed, 39 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 12f8a7d4fc1c..ba5f5e6c3927 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
> > #endif /* CONFIG_MODULES */
> >
> > #ifdef CONFIG_SYSFS
> > +/* sysfs output in /sys/modules/XYZ/parameters/ */
> > +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> > +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> > extern struct kset *module_kset;
> > extern const struct kobj_type module_ktype;
> > +
> > +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
>
> As others have said, this is way too big for an inline. Also, __init is
> at best harmless (if it is indeed inlined into all callers), but if for
> whatever reason the compiler decides to emit an OOL version, we
> definitely do not want that in .init.text as we are now calling it from
> non-init code.
>
> > +{
> > + struct module_kobject *mk;
> > + struct kobject *kobj;
> > + int err;
> > +
> > + kobj = kset_find_obj(module_kset, name);
> > + if (kobj) {
> > + mk = to_module_kobject(kobj);
> > + } else {
> > + mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> > + BUG_ON(!mk);
> > +
>
> And while the BUG_ON() is borderline acceptable in the current,
> only-used-during-init, state, that definitely must go away once the
> function can be called from non-init code.
>
> This is what I alluded to when I said that the function might need some
> refactoring before module_add_driver can start using it.
>
> I'd say that the BUG_ON can simply be removed and replaced by a return
> NULL; an "impossible" error case that can already be hit by some of the
> code below, so callers have to be prepared for it anyway. If the
> allocation fails (during early boot or later), the allocator already
> makes a lot of noise, so there's no reason to even do a WARN_ON, and
> since it "only" affects certain /sys objects, it's probably better to
> let the machine try to complete boot instead of killing it.
>
> Also, I agree with the suggestion to drop/outdent the whole else{} branch by
> changing the if() to do "return to_module_kobject(kobj);".
>
> To summarize:
>
> - refactor to do an early return
>
> - drop BUG_ON, replace by return NULL.
>
> - drop static and __init, perhaps change __init to __modinit (which is a
> pre-existing #define in params.c, so the function remains __init if
> !CONFIG_MODULES), add an appropriate declaration somewhere, and if you
> like, also do the rename
>
> - make use of it from module_add_driver()
I have addressed these feedback in v2, thank you for the reviews.
Thanks,
Shyam
^ permalink raw reply [flat|nested] 8+ messages in thread