Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731558 > unrolled thread

[PATCH V8 01/14] mmc: core: Introduce host claiming by context

Started byAdrian Hunter <adrian.hunter@intel.com>
First post2017-09-13 13:50 +0200
Last post2017-09-22 13:30 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V8 01/14] mmc: core: Introduce host claiming by context Adrian Hunter <adrian.hunter@intel.com> - 2017-09-13 13:50 +0200
    Re: [PATCH V8 01/14] mmc: core: Introduce host claiming by context Ulf Hansson <ulf.hansson@linaro.org> - 2017-09-20 11:10 +0200
    Re: [PATCH V8 01/14] mmc: core: Introduce host claiming by context Linus Walleij <linus.walleij@linaro.org> - 2017-09-22 13:30 +0200

#1731558 — [PATCH V8 01/14] mmc: core: Introduce host claiming by context

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-09-13 13:50 +0200
Subject[PATCH V8 01/14] mmc: core: Introduce host claiming by context
Message-ID<upel5-6QI-43@gated-at.bofh.it>
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(-)

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 */
-- 
1.9.1

[toc] | [next] | [standalone]

#1735655

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-09-20 11:10 +0200
Message-ID<urJb4-Y3-29@gated-at.bofh.it>
In reply to#1731558
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

[toc] | [prev] | [next] | [standalone]

#1737425

FromLinus Walleij <linus.walleij@linaro.org>
Date2017-09-22 13:30 +0200
Message-ID<usujE-5v6-19@gated-at.bofh.it>
In reply to#1731558
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web