| Message ID | 1505302814-19313-2-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: > Currently the host can be claimed by a task. Change this so that the host > can be claimed by a context that may or may not be a task. This provides > for the host to be claimed by a block driver queue to support blk-mq, while > maintaining compatibility with the existing use of mmc_claim_host(). As stated earlier, I think this is a good approach, which allows us to move forward. Some comments below, most related how to avoid us from adding more wrapper functions. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/core.c | 92 ++++++++++++++++++++++++++++++++++++++++++++---- > drivers/mmc/core/core.h | 6 ++++ > include/linux/mmc/host.h | 7 +++- > 3 files changed, 97 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 66c9cf49ad2f..09fdc467b804 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -832,9 +832,38 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz) > } > EXPORT_SYMBOL(mmc_align_data_size); > > +/* > + * Allow claiming an already claimed host if the context is the same or there is > + * no context but the task is the same. > + */ > +static inline bool mmc_ctx_matches(struct mmc_host *host, struct mmc_ctx *ctx, > + struct task_struct *task) > +{ > + return host->claimer == ctx || > + (!ctx && task && host->claimer->task == task); > +} > + > +static inline void mmc_ctx_set_claimer(struct mmc_host *host, > + struct mmc_ctx *ctx, > + struct task_struct *task) > +{ > + if (!host->claimer) { > + if (ctx) > + host->claimer = ctx; > + else > + host->claimer = &host->default_ctx; > + } > + if (task) > + host->claimer->task = task; > +} > + > /** > - * __mmc_claim_host - exclusively claim a host > + * __mmc_ctx_task_claim_host - exclusively claim a host > * @host: mmc host to claim > + * @ctx: context that claims the host or NULL in which case the default > + * context will be used > + * @task: task that claims the host or NULL in which case @ctx must be > + * provided > * @abort: whether or not the operation should be aborted > * > * Claim a host for a set of operations. If @abort is non null and > @@ -842,7 +871,8 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz) > * that non-zero value without acquiring the lock. Returns zero > * with the lock held otherwise. > */ > -int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) > +static int __mmc_ctx_task_claim_host(struct mmc_host *host, struct mmc_ctx *ctx, > + struct task_struct *task, atomic_t *abort) > { > DECLARE_WAITQUEUE(wait, current); > unsigned long flags; > @@ -856,7 +886,7 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) > while (1) { > set_current_state(TASK_UNINTERRUPTIBLE); > stop = abort ? atomic_read(abort) : 0; > - if (stop || !host->claimed || host->claimer == current) > + if (stop || !host->claimed || mmc_ctx_matches(host, ctx, task)) > break; > spin_unlock_irqrestore(&host->lock, flags); > schedule(); > @@ -865,7 +895,7 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) > set_current_state(TASK_RUNNING); > if (!stop) { > host->claimed = 1; > - host->claimer = current; > + mmc_ctx_set_claimer(host, ctx, task); > host->claim_cnt += 1; > if (host->claim_cnt == 1) > pm = true; > @@ -879,8 +909,19 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) > > return stop; > } > + > +int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) > +{ > + return __mmc_ctx_task_claim_host(host, NULL, current, abort); > +} > EXPORT_SYMBOL(__mmc_claim_host); There is currently only two users of __mmc_claim_host(). Instead of adding yet another layer of wrapper functions, let's keep the existing __mmc_claim_host(), but change its definition to take the new parameters. Then change the existing two users. > > +void mmc_ctx_claim_host(struct mmc_host *host, struct mmc_ctx *ctx) > +{ > + __mmc_ctx_task_claim_host(host, ctx, NULL, NULL); > +} > +EXPORT_SYMBOL(mmc_ctx_claim_host); There is no need to make this an exported function, better to keep it static and only to core.c. As a matter of fact, I suggest you remove this function altogether, and just call __mmc_claim_host(), with the new parameters, from mmc_ctx_get_card(). > + > /** > * mmc_release_host - release a host > * @host: mmc host to release > @@ -888,18 +929,17 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) > * Release a MMC host, allowing others to claim the host > * for their operations. > */ > -void mmc_release_host(struct mmc_host *host) > +static void __mmc_release_host(struct mmc_host *host) > { > unsigned long flags; > > - WARN_ON(!host->claimed); > - > spin_lock_irqsave(&host->lock, flags); > if (--host->claim_cnt) { > /* Release for nested claim */ > spin_unlock_irqrestore(&host->lock, flags); > } else { > host->claimed = 0; > + host->claimer->task = NULL; > host->claimer = NULL; > spin_unlock_irqrestore(&host->lock, flags); > wake_up(&host->wq); > @@ -907,8 +947,23 @@ void mmc_release_host(struct mmc_host *host) > pm_runtime_put_autosuspend(mmc_dev(host)); > } > } > + > +void mmc_release_host(struct mmc_host *host) > +{ > + WARN_ON(!host->claimed); > + > + __mmc_release_host(host); > +} > EXPORT_SYMBOL(mmc_release_host); > > +void mmc_ctx_release_host(struct mmc_host *host, struct mmc_ctx *ctx) > +{ > + WARN_ON(!host->claimed || host->claimer != ctx); > + > + __mmc_release_host(host); > +} > +EXPORT_SYMBOL(mmc_ctx_release_host); From the same reasoning as above, I think this should be converted to a static function. Or more preferably, remove it altogether, and just call __mmc_release_host() from mmc_ctx_put_card(). > + > /* > * This is a helper function, which fetches a runtime pm reference for the > * card device and also claims the host. > @@ -921,6 +976,17 @@ void mmc_get_card(struct mmc_card *card) > EXPORT_SYMBOL(mmc_get_card); > > /* > + * This is a helper function, which fetches a runtime pm reference for the > + * card device and also claims the host for the mmc context. > + */ > +void mmc_ctx_get_card(struct mmc_card *card, struct mmc_ctx *ctx) > +{ > + pm_runtime_get_sync(&card->dev); > + mmc_ctx_claim_host(card->host, ctx); > +} > +EXPORT_SYMBOL(mmc_ctx_get_card); Again, please avoid adding yet another layer of wrapper functions. Instead change the existing mmc_get_card() to take the parameter and then change the existing three users of it. > + > +/* > * This is a helper function, which releases the host and drops the runtime > * pm reference for the card device. > */ > @@ -933,6 +999,18 @@ void mmc_put_card(struct mmc_card *card) > EXPORT_SYMBOL(mmc_put_card); > > /* > + * This is a helper function, which releases the host and drops the runtime > + * pm reference for the card device. > + */ > +void mmc_ctx_put_card(struct mmc_card *card, struct mmc_ctx *ctx) > +{ > + mmc_ctx_release_host(card->host, ctx); > + pm_runtime_mark_last_busy(&card->dev); > + pm_runtime_put_autosuspend(&card->dev); > +} > +EXPORT_SYMBOL(mmc_ctx_put_card); Ditto. > + > +/* > * Internal function that does the actual ios call to the host driver, > * optionally printing some debug output. > */ > diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h > index ca861091a776..a294c836a143 100644 > --- a/drivers/mmc/core/core.h > +++ b/drivers/mmc/core/core.h > @@ -144,4 +144,10 @@ static inline void mmc_claim_host(struct mmc_host *host) > __mmc_claim_host(host, NULL); > } > > +void mmc_ctx_claim_host(struct mmc_host *host, struct mmc_ctx *ctx); > +void mmc_ctx_release_host(struct mmc_host *host, struct mmc_ctx *ctx); > + > +void mmc_ctx_get_card(struct mmc_card *card, struct mmc_ctx *ctx); > +void mmc_ctx_put_card(struct mmc_card *card, struct mmc_ctx *ctx); > + > #endif > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index f3f2d07feb2a..228a493c3b25 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -255,6 +255,10 @@ struct mmc_supply { > struct regulator *vqmmc; /* Optional Vccq supply */ > }; > > +struct mmc_ctx { > + struct task_struct *task; > +}; > + > struct mmc_host { > struct device *parent; > struct device class_dev; > @@ -388,8 +392,9 @@ struct mmc_host { > struct mmc_card *card; /* device attached to this host */ > > wait_queue_head_t wq; > - struct task_struct *claimer; /* task that has host claimed */ > + struct mmc_ctx *claimer; /* context that has host claimed */ > int claim_cnt; /* "claim" nesting count */ > + struct mmc_ctx default_ctx; /* default context */ > > struct delayed_work detect; > int detect_change; /* card detect flag */ > -- > 1.9.1 > Kind regards Uffe
On Wed, Sep 13, 2017 at 1:40 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > Currently the host can be claimed by a task. Change this so that the host > can be claimed by a context that may or may not be a task. This provides > for the host to be claimed by a block driver queue to support blk-mq, while > maintaining compatibility with the existing use of mmc_claim_host(). > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> I think this is a reasonable intermediate step as well. I am working to kill off the "big MMC host lock" by the following plan: DONE: - Convert all ioctl() ops to custom block layer requests (done) - Convert all debugfs ops to custom block layer requests (done) - Convert RPMB access to a character device to avoid one partition switch (just merged) REMAINS: - Convert boot and "general" partitions to be part of the main block device. Because they are. (This is really tricky and may require changes in the core kernel partition handling.) - Delete the dual mode card support. (No-one seems to be using it.) - Split the world in block access and SDIO with something like a simple mutex: either the host is a block device OR it is SDIO, and that state has the same lifetime as a card, since they are now either block devices or SDIO not both at the same time. - Implement a lighter lock for SDIO host claiming. - Now the lock should be gone... As you can see it is not entirely trivial :/ Linus
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 66c9cf49ad2f..09fdc467b804 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -832,9 +832,38 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz) } EXPORT_SYMBOL(mmc_align_data_size); +/* + * Allow claiming an already claimed host if the context is the same or there is + * no context but the task is the same. + */ +static inline bool mmc_ctx_matches(struct mmc_host *host, struct mmc_ctx *ctx, + struct task_struct *task) +{ + return host->claimer == ctx || + (!ctx && task && host->claimer->task == task); +} + +static inline void mmc_ctx_set_claimer(struct mmc_host *host, + struct mmc_ctx *ctx, + struct task_struct *task) +{ + if (!host->claimer) { + if (ctx) + host->claimer = ctx; + else + host->claimer = &host->default_ctx; + } + if (task) + host->claimer->task = task; +} + /** - * __mmc_claim_host - exclusively claim a host + * __mmc_ctx_task_claim_host - exclusively claim a host * @host: mmc host to claim + * @ctx: context that claims the host or NULL in which case the default + * context will be used + * @task: task that claims the host or NULL in which case @ctx must be + * provided * @abort: whether or not the operation should be aborted * * Claim a host for a set of operations. If @abort is non null and @@ -842,7 +871,8 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz) * that non-zero value without acquiring the lock. Returns zero * with the lock held otherwise. */ -int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) +static int __mmc_ctx_task_claim_host(struct mmc_host *host, struct mmc_ctx *ctx, + struct task_struct *task, atomic_t *abort) { DECLARE_WAITQUEUE(wait, current); unsigned long flags; @@ -856,7 +886,7 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) while (1) { set_current_state(TASK_UNINTERRUPTIBLE); stop = abort ? atomic_read(abort) : 0; - if (stop || !host->claimed || host->claimer == current) + if (stop || !host->claimed || mmc_ctx_matches(host, ctx, task)) break; spin_unlock_irqrestore(&host->lock, flags); schedule(); @@ -865,7 +895,7 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) set_current_state(TASK_RUNNING); if (!stop) { host->claimed = 1; - host->claimer = current; + mmc_ctx_set_claimer(host, ctx, task); host->claim_cnt += 1; if (host->claim_cnt == 1) pm = true; @@ -879,8 +909,19 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) return stop; } + +int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) +{ + return __mmc_ctx_task_claim_host(host, NULL, current, abort); +} EXPORT_SYMBOL(__mmc_claim_host); +void mmc_ctx_claim_host(struct mmc_host *host, struct mmc_ctx *ctx) +{ + __mmc_ctx_task_claim_host(host, ctx, NULL, NULL); +} +EXPORT_SYMBOL(mmc_ctx_claim_host); + /** * mmc_release_host - release a host * @host: mmc host to release @@ -888,18 +929,17 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) * Release a MMC host, allowing others to claim the host * for their operations. */ -void mmc_release_host(struct mmc_host *host) +static void __mmc_release_host(struct mmc_host *host) { unsigned long flags; - WARN_ON(!host->claimed); - spin_lock_irqsave(&host->lock, flags); if (--host->claim_cnt) { /* Release for nested claim */ spin_unlock_irqrestore(&host->lock, flags); } else { host->claimed = 0; + host->claimer->task = NULL; host->claimer = NULL; spin_unlock_irqrestore(&host->lock, flags); wake_up(&host->wq); @@ -907,8 +947,23 @@ void mmc_release_host(struct mmc_host *host) pm_runtime_put_autosuspend(mmc_dev(host)); } } + +void mmc_release_host(struct mmc_host *host) +{ + WARN_ON(!host->claimed); + + __mmc_release_host(host); +} EXPORT_SYMBOL(mmc_release_host); +void mmc_ctx_release_host(struct mmc_host *host, struct mmc_ctx *ctx) +{ + WARN_ON(!host->claimed || host->claimer != ctx); + + __mmc_release_host(host); +} +EXPORT_SYMBOL(mmc_ctx_release_host); + /* * This is a helper function, which fetches a runtime pm reference for the * card device and also claims the host. @@ -921,6 +976,17 @@ void mmc_get_card(struct mmc_card *card) EXPORT_SYMBOL(mmc_get_card); /* + * This is a helper function, which fetches a runtime pm reference for the + * card device and also claims the host for the mmc context. + */ +void mmc_ctx_get_card(struct mmc_card *card, struct mmc_ctx *ctx) +{ + pm_runtime_get_sync(&card->dev); + mmc_ctx_claim_host(card->host, ctx); +} +EXPORT_SYMBOL(mmc_ctx_get_card); + +/* * This is a helper function, which releases the host and drops the runtime * pm reference for the card device. */ @@ -933,6 +999,18 @@ void mmc_put_card(struct mmc_card *card) EXPORT_SYMBOL(mmc_put_card); /* + * This is a helper function, which releases the host and drops the runtime + * pm reference for the card device. + */ +void mmc_ctx_put_card(struct mmc_card *card, struct mmc_ctx *ctx) +{ + mmc_ctx_release_host(card->host, ctx); + pm_runtime_mark_last_busy(&card->dev); + pm_runtime_put_autosuspend(&card->dev); +} +EXPORT_SYMBOL(mmc_ctx_put_card); + +/* * Internal function that does the actual ios call to the host driver, * optionally printing some debug output. */ diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index ca861091a776..a294c836a143 100644 --- a/drivers/mmc/core/core.h +++ b/drivers/mmc/core/core.h @@ -144,4 +144,10 @@ static inline void mmc_claim_host(struct mmc_host *host) __mmc_claim_host(host, NULL); } +void mmc_ctx_claim_host(struct mmc_host *host, struct mmc_ctx *ctx); +void mmc_ctx_release_host(struct mmc_host *host, struct mmc_ctx *ctx); + +void mmc_ctx_get_card(struct mmc_card *card, struct mmc_ctx *ctx); +void mmc_ctx_put_card(struct mmc_card *card, struct mmc_ctx *ctx); + #endif diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index f3f2d07feb2a..228a493c3b25 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -255,6 +255,10 @@ struct mmc_supply { struct regulator *vqmmc; /* Optional Vccq supply */ }; +struct mmc_ctx { + struct task_struct *task; +}; + struct mmc_host { struct device *parent; struct device class_dev; @@ -388,8 +392,9 @@ struct mmc_host { struct mmc_card *card; /* device attached to this host */ wait_queue_head_t wq; - struct task_struct *claimer; /* task that has host claimed */ + struct mmc_ctx *claimer; /* context that has host claimed */ int claim_cnt; /* "claim" nesting count */ + struct mmc_ctx default_ctx; /* default context */ struct delayed_work detect; int detect_change; /* card detect flag */
Currently the host can be claimed by a task. Change this so that the host can be claimed by a context that may or may not be a task. This provides for the host to be claimed by a block driver queue to support blk-mq, while maintaining compatibility with the existing use of mmc_claim_host(). Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/core.c | 92 ++++++++++++++++++++++++++++++++++++++++++++---- drivers/mmc/core/core.h | 6 ++++ include/linux/mmc/host.h | 7 +++- 3 files changed, 97 insertions(+), 8 deletions(-)