[v3] nvme: make core.nvme_multipath configurable

[PATCH v3 3/5] nvme: add the NVME_ENABLE_MULTIPATH_PARAM config option

Posted by John Meneghini 8 months, 3 weeks ago

The CONFIG_NVME_ENABLE_MULTIPATH_PARAM option controls the
core_nvme.multipath parameter. When CONFIG_NVME_ENABLE_MULTIPATH_PARAM=n
the multipath parameter is removed from the kernel and nvme multipathing
is permanently enabled.  When CONFIG_NVME_ENABLE_MULTIPATH_PARAM=y the
nvme multipath parameter is added to the kernel and nvme multipath
support is controlled by the core_nvme.multipath parameter.

By default CONFIG_NVME_ENABLE_MULTIPATH_PARAM=y

Signed-off-by: John Meneghini <jmeneghi@redhat.com>
Tested-by: John Meneghini <jmeneghi@redhat.com>
Reviewed-by: Bryan Gurney <bgurney@redhat.com>
---
 drivers/nvme/host/Kconfig     | 11 +++++++++++
 drivers/nvme/host/multipath.c |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index d47dfa80fb95..8c04b6b93982 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -28,6 +28,17 @@ config NVME_MULTIPATH
 
 	  If unsure, say Y.
 
+config NVME_ENABLE_MULTIPATH_PARAM
+	bool "NVMe enable multipath param"
+	depends on NVME_CORE && NVME_MULTIPATH
+	default y
+	help
+	  This option enables the core_nvme.multipath parameter.
+	  If this option is disabled the core_nvme.multipath parameter
+	  is excluded from the kernel.
+
+	  If unsure, say Y.
+
 config NVME_VERBOSE_ERRORS
 	bool "NVMe verbose error reporting"
 	depends on NVME_CORE
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 6b12ca80aa27..83084093e8db 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -10,9 +10,11 @@
 #include "nvme.h"
 
 bool multipath = true;
+#ifdef CONFIG_NVME_ENABLE_MULTIPATH_PARAM
 module_param(multipath, bool, 0444);
 MODULE_PARM_DESC(multipath,
 	"turn on native support for multiple controllers per subsystem");
+#endif
 
 static const char *nvme_iopolicy_names[] = {
 	[NVME_IOPOLICY_NUMA]	= "numa",
-- 
2.48.1

Re: [PATCH v3 3/5] nvme: add the NVME_ENABLE_MULTIPATH_PARAM config option

Posted by Caleb Sander Mateos 8 months, 3 weeks ago

On Fri, Mar 28, 2025 at 12:42 PM John Meneghini <jmeneghi@redhat.com> wrote:
>
> The CONFIG_NVME_ENABLE_MULTIPATH_PARAM option controls the
> core_nvme.multipath parameter. When CONFIG_NVME_ENABLE_MULTIPATH_PARAM=n
> the multipath parameter is removed from the kernel and nvme multipathing
> is permanently enabled.  When CONFIG_NVME_ENABLE_MULTIPATH_PARAM=y the
> nvme multipath parameter is added to the kernel and nvme multipath
> support is controlled by the core_nvme.multipath parameter.
>
> By default CONFIG_NVME_ENABLE_MULTIPATH_PARAM=y
>
> Signed-off-by: John Meneghini <jmeneghi@redhat.com>
> Tested-by: John Meneghini <jmeneghi@redhat.com>
> Reviewed-by: Bryan Gurney <bgurney@redhat.com>
> ---
>  drivers/nvme/host/Kconfig     | 11 +++++++++++
>  drivers/nvme/host/multipath.c |  2 ++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index d47dfa80fb95..8c04b6b93982 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -28,6 +28,17 @@ config NVME_MULTIPATH
>
>           If unsure, say Y.
>
> +config NVME_ENABLE_MULTIPATH_PARAM
> +       bool "NVMe enable multipath param"
> +       depends on NVME_CORE && NVME_MULTIPATH
> +       default y
> +       help
> +         This option enables the core_nvme.multipath parameter.
> +         If this option is disabled the core_nvme.multipath parameter
> +         is excluded from the kernel.
> +
> +         If unsure, say Y.
> +
>  config NVME_VERBOSE_ERRORS
>         bool "NVMe verbose error reporting"
>         depends on NVME_CORE
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 6b12ca80aa27..83084093e8db 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -10,9 +10,11 @@
>  #include "nvme.h"
>
>  bool multipath = true;
> +#ifdef CONFIG_NVME_ENABLE_MULTIPATH_PARAM
>  module_param(multipath, bool, 0444);
>  MODULE_PARM_DESC(multipath,
>         "turn on native support for multiple controllers per subsystem");
> +#endif

If CONFIG_NVME_ENABLE_MULTIPATH_PARAM is disabled, could you #define
multipath false in place of the global variable? That would allow the
compiler to optimize out the multipath checks and the resulting dead
code.

Best,
Caleb

Re: [PATCH v3 3/5] nvme: add the NVME_ENABLE_MULTIPATH_PARAM config option

Posted by John Meneghini 8 months, 2 weeks ago

On 3/28/25 3:49 PM, Caleb Sander Mateos wrote:
>>   bool multipath = true;
>> +#ifdef CONFIG_NVME_ENABLE_MULTIPATH_PARAM
>>   module_param(multipath, bool, 0444);
>>   MODULE_PARM_DESC(multipath,
>>          "turn on native support for multiple controllers per subsystem");
>> +#endif
> If CONFIG_NVME_ENABLE_MULTIPATH_PARAM is disabled, could you #define
> multipath false in place of the global variable? That would allow the
> compiler to optimize out the multipath checks and the resulting dead
> code.

If we #define multipath to be false that would permanently disable the multipath code
when CONFIG_NVME_ENABLE_MULTIPATH_PARAM=n, and this is not what we want. The purpose
of CONFIG_NVME_ENABLE_MULTIPATH_PARAM=n is to simply remove the core_nvme.multipath
parameter from the kernel so the user no longer has the ability to change
the bool multipath variable.  We want multipath to be true.

There should be no change in the multipathing policy or code by
enabling or disabling CONFIG_NVME_ENABLE_MULTIPATH_PARAM.

If you want to compile out the mulipath code you should set CONFIG_NVME_ENABLE_MULTIPATH=n.

/John

Re: [PATCH v3 3/5] nvme: add the NVME_ENABLE_MULTIPATH_PARAM config option

Posted by Caleb Sander Mateos 8 months, 2 weeks ago

On Mon, Mar 31, 2025 at 7:29 AM John Meneghini <jmeneghi@redhat.com> wrote:
>
> On 3/28/25 3:49 PM, Caleb Sander Mateos wrote:
> >>   bool multipath = true;
> >> +#ifdef CONFIG_NVME_ENABLE_MULTIPATH_PARAM
> >>   module_param(multipath, bool, 0444);
> >>   MODULE_PARM_DESC(multipath,
> >>          "turn on native support for multiple controllers per subsystem");
> >> +#endif
> > If CONFIG_NVME_ENABLE_MULTIPATH_PARAM is disabled, could you #define
> > multipath false in place of the global variable? That would allow the
> > compiler to optimize out the multipath checks and the resulting dead
> > code.
>
> If we #define multipath to be false that would permanently disable the multipath code
> when CONFIG_NVME_ENABLE_MULTIPATH_PARAM=n, and this is not what we want. The purpose
> of CONFIG_NVME_ENABLE_MULTIPATH_PARAM=n is to simply remove the core_nvme.multipath
> parameter from the kernel so the user no longer has the ability to change
> the bool multipath variable.  We want multipath to be true.

Sorry, I just meant to #define multipath to a constant. I mixed up the
boolean value. If CONFIG_NVME_MULTIPATH is set, of course it should be
true, not false. If CONFIG_NVME_MULTIPATH is not set, multipath is
already defined as false.

My point was just that the compiler will pessimistically assume a
global variable with extern visibility could be written from some
other translation unit at any time. But
CONFIG_NVME_ENABLE_MULTIPATH_PARAM removes the only way for the global
variable to change at runtime, so it can just be a constant.

Best,
Caleb