Patchwork [V8,08/14] mmc: core: Add parameter use_blk_mq

login
register
mail settings
Submitter Adrian Hunter
Date Sept. 13, 2017, 11:40 a.m.
Message ID <1505302814-19313-9-git-send-email-adrian.hunter@intel.com>
Download mbox | patch
Permalink /patch/9951251/
State New
Headers show

Comments

Adrian Hunter - Sept. 13, 2017, 11:40 a.m.
Until mmc has blk-mq support fully implemented and tested, add a
parameter use_blk_mq, default to false unless config option MMC_MQ_DEFAULT
is selected.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/Kconfig      | 11 +++++++++++
 drivers/mmc/core/core.c  |  7 +++++++
 drivers/mmc/core/core.h  |  2 ++
 drivers/mmc/core/host.c  |  2 ++
 drivers/mmc/core/host.h  |  4 ++++
 include/linux/mmc/host.h |  1 +
 6 files changed, 27 insertions(+)
Ulf Hansson - Sept. 21, 2017, 9:47 a.m.
On 13 September 2017 at 13:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Until mmc has blk-mq support fully implemented and tested, add a
> parameter use_blk_mq, default to false unless config option MMC_MQ_DEFAULT
> is selected.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/Kconfig      | 11 +++++++++++
>  drivers/mmc/core/core.c  |  7 +++++++
>  drivers/mmc/core/core.h  |  2 ++
>  drivers/mmc/core/host.c  |  2 ++
>  drivers/mmc/core/host.h  |  4 ++++
>  include/linux/mmc/host.h |  1 +
>  6 files changed, 27 insertions(+)
>
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index ec21388311db..98202934bd29 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -12,6 +12,17 @@ menuconfig MMC
>           If you want MMC/SD/SDIO support, you should say Y here and
>           also to your specific host controller driver.
>
> +config MMC_MQ_DEFAULT
> +       bool "MMC: use blk-mq I/O path by default"
> +       depends on MMC && BLOCK
> +       ---help---
> +         This option enables the new blk-mq based I/O path for MMC block
> +         devices by default.  With the option the mmc_core.use_blk_mq
> +         module/boot option defaults to Y, without it to N, but it can
> +         still be overridden either way.
> +
> +         If unsure say N.
> +
>  if MMC

I asume the goal of adding this option is to enable us to move slowly
forward. In general that might be a good idea, however for this
particular case I am not sure.

The main reason is simply that I find it unlikely that people and
distributions will actually go in and change the default value, so in
the end we will just be adding new code, which isn't really going to
be much tested. That's what happened in scsi case.

As I also stated earlier, I do worry about the maintenance of the mmc
block device code, and this approach make it worse, at least short
term.

To me, the scsi case is also different, because the mq support was
added long time ago and at that point one could worry a bit of
maturity of the blkmq in general, that I assume have been sorted out
by know.

>
>  source "drivers/mmc/core/Kconfig"
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index ef2d8aa1e7d2..3638ed4f0f9e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -66,6 +66,13 @@
>  bool use_spi_crc = 1;
>  module_param(use_spi_crc, bool, 0);
>
> +#ifdef CONFIG_MMC_MQ_DEFAULT
> +bool mmc_use_blk_mq = true;
> +#else
> +bool mmc_use_blk_mq = false;
> +#endif
> +module_param_named(use_blk_mq, mmc_use_blk_mq, bool, S_IWUSR | S_IRUGO);
> +
>  static int mmc_schedule_delayed_work(struct delayed_work *work,
>                                      unsigned long delay)
>  {
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index e941342ed450..535539a9e7eb 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -35,6 +35,8 @@ struct mmc_bus_ops {
>         int (*reset)(struct mmc_host *);
>  };
>
> +extern bool mmc_use_blk_mq;
> +
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
>  void mmc_detach_bus(struct mmc_host *host);
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index ad88deb2e8f3..b624dbb6cd15 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -398,6 +398,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>         host->max_blk_size = 512;
>         host->max_blk_count = PAGE_SIZE / 512;
>
> +       host->use_blk_mq = mmc_use_blk_mq;
> +
>         return host;
>  }
>
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
> index 77d6f60d1bf9..170fe5947087 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -69,6 +69,10 @@ static inline bool mmc_card_hs400es(struct mmc_card *card)
>         return card->host->ios.enhanced_strobe;
>  }
>
> +static inline bool mmc_host_use_blk_mq(struct mmc_host *host)
> +{
> +       return host->use_blk_mq;
> +}
>
>  #endif
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 54b0463443bd..5d1bd10991f7 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -378,6 +378,7 @@ struct mmc_host {
>         unsigned int            doing_retune:1; /* re-tuning in progress */
>         unsigned int            retune_now:1;   /* do re-tuning at next req */
>         unsigned int            retune_paused:1; /* re-tuning is temporarily disabled */
> +       unsigned int            use_blk_mq:1;   /* use blk-mq */
>
>         int                     rescan_disable; /* disable card detection */
>         int                     rescan_entered; /* used with nonremovable devices */
> --
> 1.9.1
>

Kind regards
Uffe
Adrian Hunter - Sept. 22, 2017, 1:30 p.m.
On 21/09/17 12:47, Ulf Hansson wrote:
> On 13 September 2017 at 13:40, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Until mmc has blk-mq support fully implemented and tested, add a
>> parameter use_blk_mq, default to false unless config option MMC_MQ_DEFAULT
>> is selected.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/Kconfig      | 11 +++++++++++
>>  drivers/mmc/core/core.c  |  7 +++++++
>>  drivers/mmc/core/core.h  |  2 ++
>>  drivers/mmc/core/host.c  |  2 ++
>>  drivers/mmc/core/host.h  |  4 ++++
>>  include/linux/mmc/host.h |  1 +
>>  6 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index ec21388311db..98202934bd29 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -12,6 +12,17 @@ menuconfig MMC
>>           If you want MMC/SD/SDIO support, you should say Y here and
>>           also to your specific host controller driver.
>>
>> +config MMC_MQ_DEFAULT
>> +       bool "MMC: use blk-mq I/O path by default"
>> +       depends on MMC && BLOCK
>> +       ---help---
>> +         This option enables the new blk-mq based I/O path for MMC block
>> +         devices by default.  With the option the mmc_core.use_blk_mq
>> +         module/boot option defaults to Y, without it to N, but it can
>> +         still be overridden either way.
>> +
>> +         If unsure say N.
>> +
>>  if MMC
> 
> I asume the goal of adding this option is to enable us to move slowly
> forward. In general that might be a good idea, however for this
> particular case I am not sure.
> 
> The main reason is simply that I find it unlikely that people and
> distributions will actually go in and change the default value, so in
> the end we will just be adding new code, which isn't really going to
> be much tested. That's what happened in scsi case.

The argument that no one is going to test anyway so we shouldn't give them
the opportunity, is not a sustainable vision for the future.  Instead we
should reach out to relevant stakeholders and get them to do their testing
with blk-mq first.

> 
> As I also stated earlier, I do worry about the maintenance of the mmc
> block device code, and this approach make it worse, at least short
> term.

What maintenance?

> 
> To me, the scsi case is also different, because the mq support was
> added long time ago and at that point one could worry a bit of
> maturity of the blkmq in general, that I assume have been sorted out
> by know.

But blk-mq still isn't fully ready for all of scsi, so you seem to be
contradicting yourself.

> 
>>
>>  source "drivers/mmc/core/Kconfig"
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index ef2d8aa1e7d2..3638ed4f0f9e 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -66,6 +66,13 @@
>>  bool use_spi_crc = 1;
>>  module_param(use_spi_crc, bool, 0);
>>
>> +#ifdef CONFIG_MMC_MQ_DEFAULT
>> +bool mmc_use_blk_mq = true;
>> +#else
>> +bool mmc_use_blk_mq = false;
>> +#endif
>> +module_param_named(use_blk_mq, mmc_use_blk_mq, bool, S_IWUSR | S_IRUGO);
>> +
>>  static int mmc_schedule_delayed_work(struct delayed_work *work,
>>                                      unsigned long delay)
>>  {
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index e941342ed450..535539a9e7eb 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -35,6 +35,8 @@ struct mmc_bus_ops {
>>         int (*reset)(struct mmc_host *);
>>  };
>>
>> +extern bool mmc_use_blk_mq;
>> +
>>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
>>  void mmc_detach_bus(struct mmc_host *host);
>>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index ad88deb2e8f3..b624dbb6cd15 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -398,6 +398,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>>         host->max_blk_size = 512;
>>         host->max_blk_count = PAGE_SIZE / 512;
>>
>> +       host->use_blk_mq = mmc_use_blk_mq;
>> +
>>         return host;
>>  }
>>
>> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
>> index 77d6f60d1bf9..170fe5947087 100644
>> --- a/drivers/mmc/core/host.h
>> +++ b/drivers/mmc/core/host.h
>> @@ -69,6 +69,10 @@ static inline bool mmc_card_hs400es(struct mmc_card *card)
>>         return card->host->ios.enhanced_strobe;
>>  }
>>
>> +static inline bool mmc_host_use_blk_mq(struct mmc_host *host)
>> +{
>> +       return host->use_blk_mq;
>> +}
>>
>>  #endif
>>
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 54b0463443bd..5d1bd10991f7 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -378,6 +378,7 @@ struct mmc_host {
>>         unsigned int            doing_retune:1; /* re-tuning in progress */
>>         unsigned int            retune_now:1;   /* do re-tuning at next req */
>>         unsigned int            retune_paused:1; /* re-tuning is temporarily disabled */
>> +       unsigned int            use_blk_mq:1;   /* use blk-mq */
>>
>>         int                     rescan_disable; /* disable card detection */
>>         int                     rescan_entered; /* used with nonremovable devices */
>> --
>> 1.9.1
>>
> 
> Kind regards
> Uffe
>
Linus Walleij - Sept. 22, 2017, 2:01 p.m.
On Fri, Sep 22, 2017 at 3:30 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 21/09/17 12:47, Ulf Hansson wrote:

>> I asume the goal of adding this option is to enable us to move slowly
>> forward. In general that might be a good idea, however for this
>> particular case I am not sure.
>>
>> The main reason is simply that I find it unlikely that people and
>> distributions will actually go in and change the default value, so in
>> the end we will just be adding new code, which isn't really going to
>> be much tested. That's what happened in scsi case.
>
> The argument that no one is going to test anyway so we shouldn't give them
> the opportunity, is not a sustainable vision for the future.  Instead we
> should reach out to relevant stakeholders and get them to do their testing
> with blk-mq first.

We *could* simply invert the option then. Default it to "y" and
only leave it as a debugging aid so that people can set it to
"n" if they want to test with MQ disabled.

This is also simple to revert by a oneliner just removing "default y"
if there are problems with it.

Yours,
Linus Walleij

Patch

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index ec21388311db..98202934bd29 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -12,6 +12,17 @@  menuconfig MMC
 	  If you want MMC/SD/SDIO support, you should say Y here and
 	  also to your specific host controller driver.
 
+config MMC_MQ_DEFAULT
+	bool "MMC: use blk-mq I/O path by default"
+	depends on MMC && BLOCK
+	---help---
+	  This option enables the new blk-mq based I/O path for MMC block
+	  devices by default.  With the option the mmc_core.use_blk_mq
+	  module/boot option defaults to Y, without it to N, but it can
+	  still be overridden either way.
+
+	  If unsure say N.
+
 if MMC
 
 source "drivers/mmc/core/Kconfig"
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index ef2d8aa1e7d2..3638ed4f0f9e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -66,6 +66,13 @@ 
 bool use_spi_crc = 1;
 module_param(use_spi_crc, bool, 0);
 
+#ifdef CONFIG_MMC_MQ_DEFAULT
+bool mmc_use_blk_mq = true;
+#else
+bool mmc_use_blk_mq = false;
+#endif
+module_param_named(use_blk_mq, mmc_use_blk_mq, bool, S_IWUSR | S_IRUGO);
+
 static int mmc_schedule_delayed_work(struct delayed_work *work,
 				     unsigned long delay)
 {
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index e941342ed450..535539a9e7eb 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -35,6 +35,8 @@  struct mmc_bus_ops {
 	int (*reset)(struct mmc_host *);
 };
 
+extern bool mmc_use_blk_mq;
+
 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
 void mmc_detach_bus(struct mmc_host *host);
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index ad88deb2e8f3..b624dbb6cd15 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -398,6 +398,8 @@  struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	host->max_blk_size = 512;
 	host->max_blk_count = PAGE_SIZE / 512;
 
+	host->use_blk_mq = mmc_use_blk_mq;
+
 	return host;
 }
 
diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
index 77d6f60d1bf9..170fe5947087 100644
--- a/drivers/mmc/core/host.h
+++ b/drivers/mmc/core/host.h
@@ -69,6 +69,10 @@  static inline bool mmc_card_hs400es(struct mmc_card *card)
 	return card->host->ios.enhanced_strobe;
 }
 
+static inline bool mmc_host_use_blk_mq(struct mmc_host *host)
+{
+	return host->use_blk_mq;
+}
 
 #endif
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 54b0463443bd..5d1bd10991f7 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -378,6 +378,7 @@  struct mmc_host {
 	unsigned int		doing_retune:1;	/* re-tuning in progress */
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
 	unsigned int		retune_paused:1; /* re-tuning is temporarily disabled */
+	unsigned int		use_blk_mq:1;	/* use blk-mq */
 
 	int			rescan_disable;	/* disable card detection */
 	int			rescan_entered;	/* used with nonremovable devices */