[PATCH v1] module: Add support for default value for module async_probe

[PATCH v1] module: Add support for default value for module async_probe

[Saravana Kannan]

Add a module.async_probe kernel command line option that allows enabling
async probing for all modules. When this command line option is used,
there might still be some modules for which we want to explicitly force
synchronous probing, so extend <modulename>.async_probe to take an
optional bool input so that async probing can be disabled for a specific
module.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  8 ++++++--
 kernel/module/main.c                            | 11 ++++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 710b52d87bdd..32083056bd25 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1147,8 +1147,12 @@
 	nopku		[X86] Disable Memory Protection Keys CPU feature found
 			in some Intel CPUs.
 
-	<module>.async_probe [KNL]
-			Enable asynchronous probe on this module.
+	<module>.async_probe[=<bool>] [KNL]
+			If no <bool> value is specified or if the value
+			specified is not a valid <bool>, enable asynchronous
+			probe on this module.  Otherwise, enable/disable
+			asynchronous probe on this module as indicated by the
+			<bool> value.
 
 	early_ioremap_debug [KNL]
 			Enable debug messages in early_ioremap support. This
diff --git a/kernel/module/main.c b/kernel/module/main.c
index fed58d30725d..47085795f037 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2410,6 +2410,12 @@ static void do_free_init(struct work_struct *w)
 	}
 }
 
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "module."
+/* Default value for module->async_probe_requested */
+static bool async_probe;
+module_param(async_probe, bool, 0644);
+
 /*
  * This is where the real work happens.
  *
@@ -2630,7 +2636,8 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname,
 	int ret;
 
 	if (strcmp(param, "async_probe") == 0) {
-		mod->async_probe_requested = true;
+		if (strtobool(val, &mod->async_probe_requested))
+			mod->async_probe_requested = true;
 		return 0;
 	}
 
@@ -2797,6 +2804,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto bug_cleanup;
 
+	mod->async_probe_requested = async_probe;
+
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
 				  -32768, 32767, mod,
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH v1] module: Add support for default value for module async_probe

[Luis Chamberlain]

On Thu, Jun 02, 2022 at 10:54:41PM -0700, Saravana Kannan wrote:
> Add a module.async_probe kernel command line option that allows enabling
> async probing for all modules. When this command line option is used,
> there might still be some modules for which we want to explicitly force
> synchronous probing, so extend <modulename>.async_probe to take an
> optional bool input so that async probing can be disabled for a specific
> module.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  8 ++++++--
>  kernel/module/main.c                            | 11 ++++++++++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 710b52d87bdd..32083056bd25 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1147,8 +1147,12 @@
>  	nopku		[X86] Disable Memory Protection Keys CPU feature found
>  			in some Intel CPUs.
>  
> -	<module>.async_probe [KNL]
> -			Enable asynchronous probe on this module.
> +	<module>.async_probe[=<bool>] [KNL]
> +			If no <bool> value is specified or if the value
> +			specified is not a valid <bool>, enable asynchronous
> +			probe on this module.  Otherwise, enable/disable
> +			asynchronous probe on this module as indicated by the
> +			<bool> value.

The commit log says a bit more. Can you clarify this on the
documentation?

We should strive slowly towards more async probes. This will take
time. To help with further then a Kconfig option which sets this
to a default to true if enabled would be useful so that no kernel
parameter is needed at all to set the default. Then you can
override the default, and blacklist each driver as well.

  Luis

Re: [PATCH v1] module: Add support for default value for module async_probe

[Saravana Kannan]

On Fri, Jun 3, 2022 at 8:01 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Thu, Jun 02, 2022 at 10:54:41PM -0700, Saravana Kannan wrote:
> > Add a module.async_probe kernel command line option that allows enabling
> > async probing for all modules. When this command line option is used,
> > there might still be some modules for which we want to explicitly force
> > synchronous probing, so extend <modulename>.async_probe to take an
> > optional bool input so that async probing can be disabled for a specific
> > module.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  8 ++++++--
> >  kernel/module/main.c                            | 11 ++++++++++-
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 710b52d87bdd..32083056bd25 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -1147,8 +1147,12 @@
> >       nopku           [X86] Disable Memory Protection Keys CPU feature found
> >                       in some Intel CPUs.
> >
> > -     <module>.async_probe [KNL]
> > -                     Enable asynchronous probe on this module.
> > +     <module>.async_probe[=<bool>] [KNL]
> > +                     If no <bool> value is specified or if the value
> > +                     specified is not a valid <bool>, enable asynchronous
> > +                     probe on this module.  Otherwise, enable/disable
> > +                     asynchronous probe on this module as indicated by the
> > +                     <bool> value.
>
> The commit log says a bit more. Can you clarify this on the
> documentation?

Oh yeah, forgot to add module.async_probe there! Will do.

> We should strive slowly towards more async probes. This will take
> time.

Agreed.

> To help with further then a Kconfig option which sets this
> to a default to true if enabled would be useful so that no kernel
> parameter is needed at all to set the default. Then you can
> override the default, and blacklist each driver as well.

Based on Linus's view in this thread [1] (I see his point), I don't
think we'll ever enable default async probes for modules  as a compile
time config. I think it has to be an explicit decision by whoever
decides the list of modules being loaded in the system (OEMs in the
case of Android, end user in the case of a PC?) to enable the default
to be async probe and then the same entity can decide which modules to
force sync probe on. So, I'm not sure we want to add a Kconfig for
this or enable it by default. Let me know what you think. I'll send
out a v2 with the doc fixes in the meantime.

On a related note, I'm working on default async probes for built-in
drivers, but that's feasible to turn on by default because we can
synchronize everything before we jump to init. And then
<module>.async_probe needs to be passed explicitly for any modules we
want to allow async on.

-Saravana

[1] - https://lore.kernel.org/lkml/CA+55aFxV40V2WvNtJY3EC0F-B9wPk8CV2o1TTTyoF4CoWH7rhQ@mail.gmail.com/

Re: [PATCH v1] module: Add support for default value for module async_probe

[Luis Chamberlain]

On Fri, Jun 03, 2022 at 04:16:43PM -0700, Saravana Kannan wrote:
> On Fri, Jun 3, 2022 at 8:01 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Thu, Jun 02, 2022 at 10:54:41PM -0700, Saravana Kannan wrote:
> > > Add a module.async_probe kernel command line option that allows enabling
> > > async probing for all modules. When this command line option is used,
> > > there might still be some modules for which we want to explicitly force
> > > synchronous probing, so extend <modulename>.async_probe to take an
> > > optional bool input so that async probing can be disabled for a specific
> > > module.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  Documentation/admin-guide/kernel-parameters.txt |  8 ++++++--
> > >  kernel/module/main.c                            | 11 ++++++++++-
> > >  2 files changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 710b52d87bdd..32083056bd25 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -1147,8 +1147,12 @@
> > >       nopku           [X86] Disable Memory Protection Keys CPU feature found
> > >                       in some Intel CPUs.
> > >
> > > -     <module>.async_probe [KNL]
> > > -                     Enable asynchronous probe on this module.
> > > +     <module>.async_probe[=<bool>] [KNL]
> > > +                     If no <bool> value is specified or if the value
> > > +                     specified is not a valid <bool>, enable asynchronous
> > > +                     probe on this module.  Otherwise, enable/disable
> > > +                     asynchronous probe on this module as indicated by the
> > > +                     <bool> value.
> >
> > The commit log says a bit more. Can you clarify this on the
> > documentation?
> 
> Oh yeah, forgot to add module.async_probe there! Will do.
> 
> > We should strive slowly towards more async probes. This will take
> > time.
> 
> Agreed.
> 
> > To help with further then a Kconfig option which sets this
> > to a default to true if enabled would be useful so that no kernel
> > parameter is needed at all to set the default. Then you can
> > override the default, and blacklist each driver as well.
> 
> Based on Linus's view in this thread [1] (I see his point), I don't
> think we'll ever enable default async probes for modules  as a compile
> time config. 

OK that's fair. But the position there was in reference to *not*
regress userspace.

If we have new tech we can do things differently if the expectation
is set from the beginning, ie if userspace *does* expect dependencies
to be dealt with differently. And systems which are old can be
deprecated with ACPI legacy flags.

> I think it has to be an explicit decision by whoever
> decides the list of modules being loaded in the system (OEMs in the
> case of Android, end user in the case of a PC?) to enable the default
> to be async probe and then the same entity can decide which modules to
> force sync probe on. So, I'm not sure we want to add a Kconfig for
> this or enable it by default. Let me know what you think. I'll send
> out a v2 with the doc fixes in the meantime.

Yeah makes sense.

> On a related note, I'm working on default async probes for built-in
> drivers,

Cool! I'm very excited to hear how that goes!

> but that's feasible to turn on by default because we can
> synchronize everything before we jump to init.

That's cheap, in that I think we can do better. Sure it works.

> And then
> <module>.async_probe needs to be passed explicitly for any modules we
> want to allow async on.

Sure..

  Luis