| Message ID | 1505302814-19313-9-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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 */
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(+)