| Message ID | 1506083824-4024-10-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 22, 2017 at 2:36 PM, 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> > +config MMC_MQ_DEFAULT > + bool "MMC: use blk-mq I/O path by default" > + depends on MMC && BLOCK I would say: default y Why not. SCSI is starting to enable this by default so IMO we should not take the intermediate step of having this as optional. Otherwise it never gets tested. Set it to default y and after two kernel releases, if nothing happens, we simply delete the old block layer path. > +#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); Are people really modprobing this so it needs to be a module parameter? Maybe I'm the only developer stupid enough to just recompile and reboot the whole kernel, I guess this makes sense if you're testing on the same machine you're developing on (no cross-compilation and remote target) which I guess is what some Intel people are doing with their laptops. Yours, Linus Walleij
On 27/09/17 02:42, Linus Walleij wrote: > On Fri, Sep 22, 2017 at 2:36 PM, 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> > >> +config MMC_MQ_DEFAULT >> + bool "MMC: use blk-mq I/O path by default" >> + depends on MMC && BLOCK > > I would say: > default y > > Why not. SCSI is starting to enable this by default so IMO we should SCSI didn't manage it yet. > not take the > intermediate step of having this as optional. Otherwise it never gets tested. The argument that we don't have to take any responsibility for getting things tested is a poor one. Anyway, you can always send a patch later to change the default, so why the hurry to do it now? > > Set it to default y and after two kernel releases, if nothing happens, we simply > delete the old block layer path. > >> +#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); > > Are people really modprobing this so it needs to be a module parameter? Irrespective of modprobe, the parameter can be changed without re-compiling. > > Maybe I'm the only developer stupid enough to just recompile and reboot Just change the parameter and unbind and rebind the host controller.
> -----Original Message----- > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc- > owner@vger.kernel.org] On Behalf Of Linus Walleij > Sent: Wednesday, September 27, 2017 2:42 AM > To: Adrian Hunter <adrian.hunter@intel.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc <linux- > mmc@vger.kernel.org>; linux-block <linux-block@vger.kernel.org>; linux- > kernel <linux-kernel@vger.kernel.org>; Bough Chen <haibo.chen@nxp.com>; > Alex Lemberg <Alex.Lemberg@wdc.com>; Mateusz Nowak > <mateusz.nowak@intel.com>; Yuliy Izrailov <Yuliy.Izrailov@wdc.com>; > Jaehoon Chung <jh80.chung@samsung.com>; Dong Aisheng > <dongas86@gmail.com>; Das Asutosh <asutoshd@codeaurora.org>; Zhangfei > Gao <zhangfei.gao@gmail.com>; Sahitya Tummala > <stummala@codeaurora.org>; Harjani Ritesh <riteshh@codeaurora.org>; > Venu Byravarasu <vbyravarasu@nvidia.com>; Shawn Lin <shawn.lin@rock- > chips.com>; Christoph Hellwig <hch@lst.de> > Subject: Re: [PATCH V9 09/15] mmc: core: Add parameter use_blk_mq > > On Fri, Sep 22, 2017 at 2:36 PM, 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> > > > +config MMC_MQ_DEFAULT > > + bool "MMC: use blk-mq I/O path by default" > > + depends on MMC && BLOCK > > I would say: > default y > > Why not. SCSI is starting to enable this by default so IMO we should not take > the intermediate step of having this as optional. Otherwise it never gets > tested. > > Set it to default y and after two kernel releases, if nothing happens, we simply > delete the old block layer path. > > > +#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); > > Are people really modprobing this so it needs to be a module parameter? Module param can be changed in runtime > > Maybe I'm the only developer stupid enough to just recompile and reboot the > whole kernel, I guess this makes sense if you're testing on the same machine > you're developing on (no cross-compilation and remote target) which I guess > is what some Intel people are doing with their laptops. > > Yours, > Linus Walleij > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the > body of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
On Wed, Sep 27, 2017 at 2:02 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 27/09/17 02:42, Linus Walleij wrote: >> On Fri, Sep 22, 2017 at 2:36 PM, 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> >> >>> +config MMC_MQ_DEFAULT >>> + bool "MMC: use blk-mq I/O path by default" >>> + depends on MMC && BLOCK >> >> I would say: >> default y >> >> Why not. SCSI is starting to enable this by default so IMO we should > > SCSI didn't manage it yet. > >> not take the >> intermediate step of having this as optional. Otherwise it never gets tested. > > The argument that we don't have to take any responsibility for getting > things tested is a poor one. > > Anyway, you can always send a patch later to change the default, so why the > hurry to do it now? I think it should be the default so we smoke out bugs by throwing it at users during the -rc phase and the non-mq path should be the fallback. The -rc phase is for finding problems like this IMO. It is partly a personality trait I guess, I'm not very cautious in general, for good and for bad. >>> +module_param_named(use_blk_mq, mmc_use_blk_mq, bool, S_IWUSR | S_IRUGO); >> >> Are people really modprobing this so it needs to be a module parameter? > > Irrespective of modprobe, the parameter can be changed without re-compiling. > >> >> Maybe I'm the only developer stupid enough to just recompile and reboot > > Just change the parameter and unbind and rebind the host controller. Ah clever. I don't do such things, I guess I should try it out. Yours, Linus Walleij
On 22 September 2017 at 14:36, 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> Thanks, applied for next! Kind regards Uffe > --- > 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 > > source "drivers/mmc/core/Kconfig" > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 2ff614d4ffac..10d7101164e4 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 ba5a8fea0dc2..159b2ca301ec 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 c296f4351c1d..9109265fe529 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 >
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 2ff614d4ffac..10d7101164e4 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 ba5a8fea0dc2..159b2ca301ec 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 c296f4351c1d..9109265fe529 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(+)