[RFC PATCH 0/3] mmc: core: Prepare mmc host locking for blk-mq

[RFC PATCH 0/3] mmc: core: Prepare mmc host locking for blk-mq

[Ulf Hansson]

The current APIs, mmc_claim|release_host() doesn't play well when the mmc block
device driver tries to convert to blk-mq. In principle we need a re-claiming
possibility of the host from the mmc block device driver, and to know when to
release the host, need to keep track of a reference count instead.

This series, so far untested, implements the idea from above. Sent as an RFC
just to get peoples opinion of whether this is an approach that could work.

Ulf Hansson (3):
  mmc: sdio: Don't use abort-able claim host method from SDIO IRQ thread
  mmc: core: Remove redundant abort-able claim host API
  mmc: core: Allow mmc block device to re-claim the host

 drivers/mmc/core/core.c     | 33 ++++++++++++++-------------------
 drivers/mmc/core/core.h     | 10 ++--------
 drivers/mmc/core/sdio_irq.c | 10 ++++++----
 include/linux/mmc/host.h    |  2 +-
 4 files changed, 23 insertions(+), 32 deletions(-)

-- 
2.7.4

[RFC PATCH 1/3] mmc: sdio: Don't use abort-able claim host method from SDIO IRQ thread

[Ulf Hansson]

In a step to simplify the use of the lock, mmc_claim|release_host(), let's
change the SDIO IRQ thread to move away from using the abort-able claim
host method.

In the SDIO IRQ thread case, we can instead check the numbers of SDIO IRQs
that are currently claimed via host->sdio_irqs, as this field is protected
from updates unless the host is claimed. When host->sdio_irqs has become
zero, it means the SDIO func driver(s) has released the SDIO IRQ, then we
can bail out and stop running the SDIO IRQ thread.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 10 ++++++----
 include/linux/mmc/host.h    |  1 -
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 6d4b720..95f7a09 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -137,9 +137,13 @@ static int sdio_irq_thread(void *_host)
 		 * holding of the host lock does not cover too much work
 		 * that doesn't require that lock to be held.
 		 */
-		ret = __mmc_claim_host(host, &host->sdio_irq_thread_abort);
-		if (ret)
+		mmc_claim_host(host);
+		if (!host->sdio_irqs) {
+			ret = 1;
+			mmc_release_host(host);
 			break;
+		}
+
 		ret = process_sdio_pending_irqs(host);
 		host->sdio_irq_pending = false;
 		mmc_release_host(host);
@@ -195,7 +199,6 @@ static int sdio_card_irq_get(struct mmc_card *card)
 
 	if (!host->sdio_irqs++) {
 		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
-			atomic_set(&host->sdio_irq_thread_abort, 0);
 			host->sdio_irq_thread =
 				kthread_run(sdio_irq_thread, host,
 					    "ksdioirqd/%s", mmc_hostname(host));
@@ -223,7 +226,6 @@ static int sdio_card_irq_put(struct mmc_card *card)
 
 	if (!--host->sdio_irqs) {
 		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
-			atomic_set(&host->sdio_irq_thread_abort, 1);
 			kthread_stop(host->sdio_irq_thread);
 		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
 			host->ops->enable_sdio_irq(host, 0);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 21385ac..8a4131f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -359,7 +359,6 @@ struct mmc_host {
 	unsigned int		sdio_irqs;
 	struct task_struct	*sdio_irq_thread;
 	bool			sdio_irq_pending;
-	atomic_t		sdio_irq_thread_abort;
 
 	mmc_pm_flag_t		pm_flags;	/* requested pm features */
 
-- 
2.7.4

Re: [RFC PATCH 1/3] mmc: sdio: Don't use abort-able claim host method from SDIO IRQ thread

[Adrian Hunter]

On 11/05/17 15:39, Ulf Hansson wrote:
> In a step to simplify the use of the lock, mmc_claim|release_host(), let's
> change the SDIO IRQ thread to move away from using the abort-able claim
> host method.
> 
> In the SDIO IRQ thread case, we can instead check the numbers of SDIO IRQs
> that are currently claimed via host->sdio_irqs, as this field is protected
> from updates unless the host is claimed. When host->sdio_irqs has become
> zero, it means the SDIO func driver(s) has released the SDIO IRQ, then we
> can bail out and stop running the SDIO IRQ thread.

Does that work?  It looks a like kthread_stop() will wait on the thread,
which is waiting to claim the host, which is claimed by the caller of
sdio_card_irq_put() which called kthread_stop() i.e. deadlock.

> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 10 ++++++----
>  include/linux/mmc/host.h    |  1 -
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index 6d4b720..95f7a09 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -137,9 +137,13 @@ static int sdio_irq_thread(void *_host)
>  		 * holding of the host lock does not cover too much work
>  		 * that doesn't require that lock to be held.
>  		 */
> -		ret = __mmc_claim_host(host, &host->sdio_irq_thread_abort);
> -		if (ret)
> +		mmc_claim_host(host);
> +		if (!host->sdio_irqs) {
> +			ret = 1;
> +			mmc_release_host(host);
>  			break;
> +		}
> +
>  		ret = process_sdio_pending_irqs(host);
>  		host->sdio_irq_pending = false;
>  		mmc_release_host(host);
> @@ -195,7 +199,6 @@ static int sdio_card_irq_get(struct mmc_card *card)
>  
>  	if (!host->sdio_irqs++) {
>  		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
> -			atomic_set(&host->sdio_irq_thread_abort, 0);
>  			host->sdio_irq_thread =
>  				kthread_run(sdio_irq_thread, host,
>  					    "ksdioirqd/%s", mmc_hostname(host));
> @@ -223,7 +226,6 @@ static int sdio_card_irq_put(struct mmc_card *card)
>  
>  	if (!--host->sdio_irqs) {
>  		if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
> -			atomic_set(&host->sdio_irq_thread_abort, 1);
>  			kthread_stop(host->sdio_irq_thread);
>  		} else if (host->caps & MMC_CAP_SDIO_IRQ) {
>  			host->ops->enable_sdio_irq(host, 0);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 21385ac..8a4131f 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -359,7 +359,6 @@ struct mmc_host {
>  	unsigned int		sdio_irqs;
>  	struct task_struct	*sdio_irq_thread;
>  	bool			sdio_irq_pending;
> -	atomic_t		sdio_irq_thread_abort;
>  
>  	mmc_pm_flag_t		pm_flags;	/* requested pm features */
>  
> 

Re: [RFC PATCH 1/3] mmc: sdio: Don't use abort-able claim host method from SDIO IRQ thread

[Ulf Hansson]

On 12 May 2017 at 09:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 11/05/17 15:39, Ulf Hansson wrote:
>> In a step to simplify the use of the lock, mmc_claim|release_host(), let's
>> change the SDIO IRQ thread to move away from using the abort-able claim
>> host method.
>>
>> In the SDIO IRQ thread case, we can instead check the numbers of SDIO IRQs
>> that are currently claimed via host->sdio_irqs, as this field is protected
>> from updates unless the host is claimed. When host->sdio_irqs has become
>> zero, it means the SDIO func driver(s) has released the SDIO IRQ, then we
>> can bail out and stop running the SDIO IRQ thread.
>
> Does that work?  It looks a like kthread_stop() will wait on the thread,
> which is waiting to claim the host, which is claimed by the caller of
> sdio_card_irq_put() which called kthread_stop() i.e. deadlock.

Adrian, you are right and thanks for your analyze!

Maybe it's better that I first continue with my other series,
converting the kthread to a work/work-queue. As that makes it easier
to move away from using the abort-able claim host API.

Kind regards
Uffe

>
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/sdio_irq.c | 10 ++++++----
>>  include/linux/mmc/host.h    |  1 -
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
>> index 6d4b720..95f7a09 100644
>> --- a/drivers/mmc/core/sdio_irq.c
>> +++ b/drivers/mmc/core/sdio_irq.c
>> @@ -137,9 +137,13 @@ static int sdio_irq_thread(void *_host)
>>                * holding of the host lock does not cover too much work
>>                * that doesn't require that lock to be held.
>>                */
>> -             ret = __mmc_claim_host(host, &host->sdio_irq_thread_abort);
>> -             if (ret)
>> +             mmc_claim_host(host);
>> +             if (!host->sdio_irqs) {
>> +                     ret = 1;
>> +                     mmc_release_host(host);
>>                       break;
>> +             }
>> +
>>               ret = process_sdio_pending_irqs(host);
>>               host->sdio_irq_pending = false;
>>               mmc_release_host(host);
>> @@ -195,7 +199,6 @@ static int sdio_card_irq_get(struct mmc_card *card)
>>
>>       if (!host->sdio_irqs++) {
>>               if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
>> -                     atomic_set(&host->sdio_irq_thread_abort, 0);
>>                       host->sdio_irq_thread =
>>                               kthread_run(sdio_irq_thread, host,
>>                                           "ksdioirqd/%s", mmc_hostname(host));
>> @@ -223,7 +226,6 @@ static int sdio_card_irq_put(struct mmc_card *card)
>>
>>       if (!--host->sdio_irqs) {
>>               if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
>> -                     atomic_set(&host->sdio_irq_thread_abort, 1);
>>                       kthread_stop(host->sdio_irq_thread);
>>               } else if (host->caps & MMC_CAP_SDIO_IRQ) {
>>                       host->ops->enable_sdio_irq(host, 0);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 21385ac..8a4131f 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -359,7 +359,6 @@ struct mmc_host {
>>       unsigned int            sdio_irqs;
>>       struct task_struct      *sdio_irq_thread;
>>       bool                    sdio_irq_pending;
>> -     atomic_t                sdio_irq_thread_abort;
>>
>>       mmc_pm_flag_t           pm_flags;       /* requested pm features */
>>
>>
>

[RFC PATCH 2/3] mmc: core: Remove redundant abort-able claim host API

[Ulf Hansson]

The only user of the abort-able claim host API was the SDIO IRQ thread, but
as that use has now been removed, let's simplify the code and remove the
API.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c | 31 ++++++++++---------------------
 drivers/mmc/core/core.h | 13 +------------
 2 files changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 82c45dd..0701e30 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1019,20 +1019,15 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz)
 EXPORT_SYMBOL(mmc_align_data_size);
 
 /**
- *	__mmc_claim_host - exclusively claim a host
+ *	mmc_claim_host - exclusively claim a host
  *	@host: mmc host to claim
- *	@abort: whether or not the operation should be aborted
  *
- *	Claim a host for a set of operations.  If @abort is non null and
- *	dereference a non-zero value then this will return prematurely with
- *	that non-zero value without acquiring the lock.  Returns zero
- *	with the lock held otherwise.
+ *	Claim a host for a set of operations.
  */
-int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
+void mmc_claim_host(struct mmc_host *host)
 {
 	DECLARE_WAITQUEUE(wait, current);
 	unsigned long flags;
-	int stop;
 	bool pm = false;
 
 	might_sleep();
@@ -1041,31 +1036,25 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 	spin_lock_irqsave(&host->lock, flags);
 	while (1) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		stop = abort ? atomic_read(abort) : 0;
-		if (stop || !host->claimed || host->claimer == current)
+		if (!host->claimed || host->claimer == current)
 			break;
 		spin_unlock_irqrestore(&host->lock, flags);
 		schedule();
 		spin_lock_irqsave(&host->lock, flags);
 	}
 	set_current_state(TASK_RUNNING);
-	if (!stop) {
-		host->claimed = 1;
-		host->claimer = current;
-		host->claim_cnt += 1;
-		if (host->claim_cnt == 1)
-			pm = true;
-	} else
-		wake_up(&host->wq);
+	host->claimed = 1;
+	host->claimer = current;
+	host->claim_cnt += 1;
+	if (host->claim_cnt == 1)
+		pm = true;
 	spin_unlock_irqrestore(&host->lock, flags);
 	remove_wait_queue(&host->wq, &wait);
 
 	if (pm)
 		pm_runtime_get_sync(mmc_dev(host));
-
-	return stop;
 }
-EXPORT_SYMBOL(__mmc_claim_host);
+EXPORT_SYMBOL(mmc_claim_host);
 
 /**
  *	mmc_release_host - release a host
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 55f543f..b247b1f 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -122,20 +122,9 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen);
 int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
 			bool is_rel_write);
 
-int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
+void mmc_claim_host(struct mmc_host *host);
 void mmc_release_host(struct mmc_host *host);
 void mmc_get_card(struct mmc_card *card);
 void mmc_put_card(struct mmc_card *card);
 
-/**
- *	mmc_claim_host - exclusively claim a host
- *	@host: mmc host to claim
- *
- *	Claim a host for a set of operations.
- */
-static inline void mmc_claim_host(struct mmc_host *host)
-{
-	__mmc_claim_host(host, NULL);
-}
-
 #endif
-- 
2.7.4

[RFC PATCH 3/3] mmc: core: Allow mmc block device to re-claim the host

[Ulf Hansson]

The current mmc block device implementation is tricky when it comes to
claim and release of the host, while processing I/O requests. In principle
we need to claim the host at the first request entering the queue and then
we need to release the host, as soon as the queue becomes empty. This
complexity relates to the asynchronous request mechanism that the mmc block
device driver implements.

For the legacy block interface that we currently implements, the above
issue can be addressed, as we can find out when the queue really becomes
empty.

However, to find out whether the queue is empty, isn't really an applicable
method when using the new blk-mq interface, as requests are instead pushed
to us via the struct struct blk_mq_ops and its function pointers.

Being able to support the asynchronous request method using the blk-mq
interface, means we have to allow the mmc block device driver to re-claim
the host from different tasks/contexts, as we may have > 1 request to
operate upon.

Therefore, let's extend the mmc_claim_host() API to support reference
counting for the mmc block device.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c  | 14 ++++++++++----
 drivers/mmc/core/core.h  |  7 ++++++-
 include/linux/mmc/host.h |  1 +
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0701e30..3633699 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1019,12 +1019,12 @@ unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz)
 EXPORT_SYMBOL(mmc_align_data_size);
 
 /**
- *	mmc_claim_host - exclusively claim a host
+ *	__mmc_claim_host - exclusively claim a host
  *	@host: mmc host to claim
  *
  *	Claim a host for a set of operations.
  */
-void mmc_claim_host(struct mmc_host *host)
+void __mmc_claim_host(struct mmc_host *host, bool is_blkdev)
 {
 	DECLARE_WAITQUEUE(wait, current);
 	unsigned long flags;
@@ -1036,7 +1036,11 @@ void mmc_claim_host(struct mmc_host *host)
 	spin_lock_irqsave(&host->lock, flags);
 	while (1) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (!host->claimed || host->claimer == current)
+		if (!host->claimed)
+			break;
+		if (host->claimer_is_blkdev && is_blkdev)
+			break;
+		if (host->claimer == current)
 			break;
 		spin_unlock_irqrestore(&host->lock, flags);
 		schedule();
@@ -1045,6 +1049,7 @@ void mmc_claim_host(struct mmc_host *host)
 	set_current_state(TASK_RUNNING);
 	host->claimed = 1;
 	host->claimer = current;
+	host->claimer_is_blkdev = is_blkdev;
 	host->claim_cnt += 1;
 	if (host->claim_cnt == 1)
 		pm = true;
@@ -1054,7 +1059,7 @@ void mmc_claim_host(struct mmc_host *host)
 	if (pm)
 		pm_runtime_get_sync(mmc_dev(host));
 }
-EXPORT_SYMBOL(mmc_claim_host);
+EXPORT_SYMBOL(__mmc_claim_host);
 
 /**
  *	mmc_release_host - release a host
@@ -1076,6 +1081,7 @@ void mmc_release_host(struct mmc_host *host)
 	} else {
 		host->claimed = 0;
 		host->claimer = NULL;
+		host->claimer_is_blkdev = 0;
 		spin_unlock_irqrestore(&host->lock, flags);
 		wake_up(&host->wq);
 		pm_runtime_mark_last_busy(mmc_dev(host));
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index b247b1f..1598a37 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -122,9 +122,14 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen);
 int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
 			bool is_rel_write);
 
-void mmc_claim_host(struct mmc_host *host);
+void __mmc_claim_host(struct mmc_host *host, bool is_blkdev);
 void mmc_release_host(struct mmc_host *host);
 void mmc_get_card(struct mmc_card *card);
 void mmc_put_card(struct mmc_card *card);
 
+static inline void mmc_claim_host(struct mmc_host *host)
+{
+	__mmc_claim_host(host, 0);
+}
+
 #endif
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 8a4131f..7199817 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -347,6 +347,7 @@ struct mmc_host {
 
 	wait_queue_head_t	wq;
 	struct task_struct	*claimer;	/* task that has host claimed */
+	bool			claimer_is_blkdev; /* claimer is blkdev */
 	int			claim_cnt;	/* "claim" nesting count */
 
 	struct delayed_work	detect;
-- 
2.7.4

Re: [RFC PATCH 3/3] mmc: core: Allow mmc block device to re-claim the host

[Adrian Hunter]

On 11/05/17 15:39, Ulf Hansson wrote:
> The current mmc block device implementation is tricky when it comes to
> claim and release of the host, while processing I/O requests. In principle
> we need to claim the host at the first request entering the queue and then
> we need to release the host, as soon as the queue becomes empty. This
> complexity relates to the asynchronous request mechanism that the mmc block
> device driver implements.
> 
> For the legacy block interface that we currently implements, the above
> issue can be addressed, as we can find out when the queue really becomes
> empty.
> 
> However, to find out whether the queue is empty, isn't really an applicable
> method when using the new blk-mq interface, as requests are instead pushed
> to us via the struct struct blk_mq_ops and its function pointers.

That is not entirely true.  We can pull requests by running the queue i.e.
blk_mq_run_hw_queues(q, false), returning BLK_MQ_RQ_QUEUE_BUSY and stopping
/ starting the queue as needed.

But, as I have written before, we could start with the most trivial
implementation.  ->queue_rq() puts the requests in a list and then the
thread removes them from the list.

That would be a good start because it would avoid having to deal with other
issues at the same time.

> 
> Being able to support the asynchronous request method using the blk-mq
> interface, means we have to allow the mmc block device driver to re-claim
> the host from different tasks/contexts, as we may have > 1 request to
> operate upon.
> 
> Therefore, let's extend the mmc_claim_host() API to support reference
> counting for the mmc block device.

Aren't you overlooking the possibility that there are many block devices per
host. i.e. one per eMMC internal partition.

Re: [RFC PATCH 3/3] mmc: core: Allow mmc block device to re-claim the host

[Ulf Hansson]

On 12 May 2017 at 10:36, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 11/05/17 15:39, Ulf Hansson wrote:
>> The current mmc block device implementation is tricky when it comes to
>> claim and release of the host, while processing I/O requests. In principle
>> we need to claim the host at the first request entering the queue and then
>> we need to release the host, as soon as the queue becomes empty. This
>> complexity relates to the asynchronous request mechanism that the mmc block
>> device driver implements.
>>
>> For the legacy block interface that we currently implements, the above
>> issue can be addressed, as we can find out when the queue really becomes
>> empty.
>>
>> However, to find out whether the queue is empty, isn't really an applicable
>> method when using the new blk-mq interface, as requests are instead pushed
>> to us via the struct struct blk_mq_ops and its function pointers.
>
> That is not entirely true.  We can pull requests by running the queue i.e.
> blk_mq_run_hw_queues(q, false), returning BLK_MQ_RQ_QUEUE_BUSY and stopping
> / starting the queue as needed.

I am not sure how that would work. It doesn't sound very effective to
me, but I may be wrong.

>
> But, as I have written before, we could start with the most trivial
> implementation.  ->queue_rq() puts the requests in a list and then the
> thread removes them from the list.

That would work...

>
> That would be a good start because it would avoid having to deal with other
> issues at the same time.

...however this doesn't seem like a step in the direction we want to
take when porting to blkmq.

There will be an extra context switch for each an every request, won't there?

My point is, to be able to convert to blkmq, we must also avoid
performance regression - at leas as long as possible.

>
>>
>> Being able to support the asynchronous request method using the blk-mq
>> interface, means we have to allow the mmc block device driver to re-claim
>> the host from different tasks/contexts, as we may have > 1 request to
>> operate upon.
>>
>> Therefore, let's extend the mmc_claim_host() API to support reference
>> counting for the mmc block device.
>
> Aren't you overlooking the possibility that there are many block devices per
> host. i.e. one per eMMC internal partition.

Right now, yes you are right. I hope soon not. :-)

These internal eMMC partitions are today suffering from the similar
problems as we have for mmc ioctls. That means, requests are being I/O
scheduled separately for each internal partition. Meaning requests for
one partition could starve requests for another.

I really hope we can fix this in some way or the other. Probably
building upon Linus Walleij's series for fixing the problems for mmc
ioctls [1] is the way to go.

Then, when we have managed to fix these issues, I think my approach
for extending the mmc_claim_host() API could be a possible
intermediate step when trying to complete the blkmq port. Then we can
continue to try to remove/re-work the claim host lock altogether as an
optimization task.

Kind regards
Uffe

[1]
https://www.spinics.net/lists/linux-block/msg12677.html

Re: [RFC PATCH 3/3] mmc: core: Allow mmc block device to re-claim the host

[Adrian Hunter]

On 15/05/17 17:05, Ulf Hansson wrote:
> On 12 May 2017 at 10:36, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 11/05/17 15:39, Ulf Hansson wrote:
>>> The current mmc block device implementation is tricky when it comes to
>>> claim and release of the host, while processing I/O requests. In principle
>>> we need to claim the host at the first request entering the queue and then
>>> we need to release the host, as soon as the queue becomes empty. This
>>> complexity relates to the asynchronous request mechanism that the mmc block
>>> device driver implements.
>>>
>>> For the legacy block interface that we currently implements, the above
>>> issue can be addressed, as we can find out when the queue really becomes
>>> empty.
>>>
>>> However, to find out whether the queue is empty, isn't really an applicable
>>> method when using the new blk-mq interface, as requests are instead pushed
>>> to us via the struct struct blk_mq_ops and its function pointers.
>>
>> That is not entirely true.  We can pull requests by running the queue i.e.
>> blk_mq_run_hw_queues(q, false), returning BLK_MQ_RQ_QUEUE_BUSY and stopping
>> / starting the queue as needed.
> 
> I am not sure how that would work. It doesn't sound very effective to
> me, but I may be wrong.

The queue depth is not the arbiter of whether we can issue a request.  That
means there will certainly be times when we have to return
BLK_MQ_RQ_QUEUE_BUSY from ->queue_rq() and perhaps stop the queue as well.

We could start with ->queue_rq() feeding every request to the existing
thread and work towards having it submit requests immediately when possible.
 Currently mmc core cannot submit mmc_requests without waiting, but the
command queue implementation can for read/write requests when the host
controller and card are runtime resumed and the card is switched to the
correct internal partition (and we are not currently discarding flushing or
recovering), so it might be simpler to start with cmdq ;-)

> 
>>
>> But, as I have written before, we could start with the most trivial
>> implementation.  ->queue_rq() puts the requests in a list and then the
>> thread removes them from the list.
> 
> That would work...
> 
>>
>> That would be a good start because it would avoid having to deal with other
>> issues at the same time.
> 
> ...however this doesn't seem like a step in the direction we want to
> take when porting to blkmq.
> 
> There will be an extra context switch for each an every request, won't there?

No, for synchronous requests, it would be the same as now. ->queue_rq()
would be called in the context of the submitter and would wake the thread
just like ->request_fn() does now.

> 
> My point is, to be able to convert to blkmq, we must also avoid
> performance regression - at leas as long as possible.

It would still be better to start simple, and measure the performance, than
to guess where the bottlenecks are.

> 
>>
>>>
>>> Being able to support the asynchronous request method using the blk-mq
>>> interface, means we have to allow the mmc block device driver to re-claim
>>> the host from different tasks/contexts, as we may have > 1 request to
>>> operate upon.
>>>
>>> Therefore, let's extend the mmc_claim_host() API to support reference
>>> counting for the mmc block device.
>>
>> Aren't you overlooking the possibility that there are many block devices per
>> host. i.e. one per eMMC internal partition.
> 
> Right now, yes you are right. I hope soon not. :-)
> 
> These internal eMMC partitions are today suffering from the similar
> problems as we have for mmc ioctls. That means, requests are being I/O
> scheduled separately for each internal partition. Meaning requests for
> one partition could starve requests for another.
> 
> I really hope we can fix this in some way or the other. Probably
> building upon Linus Walleij's series for fixing the problems for mmc
> ioctls [1] is the way to go.
> 
> Then, when we have managed to fix these issues, I think my approach
> for extending the mmc_claim_host() API could be a possible
> intermediate step when trying to complete the blkmq port. Then we can
> continue to try to remove/re-work the claim host lock altogether as an
> optimization task.
> 
> Kind regards
> Uffe
> 
> [1]
> https://www.spinics.net/lists/linux-block/msg12677.html
> 

Re: [RFC PATCH 3/3] mmc: core: Allow mmc block device to re-claim the host

[Ulf Hansson]

On 16 May 2017 at 15:24, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 15/05/17 17:05, Ulf Hansson wrote:
>> On 12 May 2017 at 10:36, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 11/05/17 15:39, Ulf Hansson wrote:
>>>> The current mmc block device implementation is tricky when it comes to
>>>> claim and release of the host, while processing I/O requests. In principle
>>>> we need to claim the host at the first request entering the queue and then
>>>> we need to release the host, as soon as the queue becomes empty. This
>>>> complexity relates to the asynchronous request mechanism that the mmc block
>>>> device driver implements.
>>>>
>>>> For the legacy block interface that we currently implements, the above
>>>> issue can be addressed, as we can find out when the queue really becomes
>>>> empty.
>>>>
>>>> However, to find out whether the queue is empty, isn't really an applicable
>>>> method when using the new blk-mq interface, as requests are instead pushed
>>>> to us via the struct struct blk_mq_ops and its function pointers.
>>>
>>> That is not entirely true.  We can pull requests by running the queue i.e.
>>> blk_mq_run_hw_queues(q, false), returning BLK_MQ_RQ_QUEUE_BUSY and stopping
>>> / starting the queue as needed.
>>
>> I am not sure how that would work. It doesn't sound very effective to
>> me, but I may be wrong.
>
> The queue depth is not the arbiter of whether we can issue a request.  That
> means there will certainly be times when we have to return
> BLK_MQ_RQ_QUEUE_BUSY from ->queue_rq() and perhaps stop the queue as well.
>
> We could start with ->queue_rq() feeding every request to the existing
> thread and work towards having it submit requests immediately when possible.
>  Currently mmc core cannot submit mmc_requests without waiting, but the
> command queue implementation can for read/write requests when the host
> controller and card are runtime resumed and the card is switched to the
> correct internal partition (and we are not currently discarding flushing or
> recovering), so it might be simpler to start with cmdq ;-)

In the end I guess the only thing to do is to compare the patchsets to
see how the result would look like. :-)

My current observation is that our current implementation of the mmc
block device and corresponding mmc queue, is still rather messy, even
if you and Linus recently has worked hard to improve the situation.

Moreover it looks quite different compared to other block device
drivers and in the way of striving to make it more robust and
maintainable, that's not good.

Therefore, I am not really comfortable with replacing one mmc hack for
block device management with yet another, as that seems to be what
your approach would do - unless I am mistaken of course.

Instead I would like us to move into a more generic blk device
approach. Whatever that means. :-)

>
>>
>>>
>>> But, as I have written before, we could start with the most trivial
>>> implementation.  ->queue_rq() puts the requests in a list and then the
>>> thread removes them from the list.
>>
>> That would work...
>>
>>>
>>> That would be a good start because it would avoid having to deal with other
>>> issues at the same time.
>>
>> ...however this doesn't seem like a step in the direction we want to
>> take when porting to blkmq.
>>
>> There will be an extra context switch for each an every request, won't there?
>
> No, for synchronous requests, it would be the same as now. ->queue_rq()
> would be called in the context of the submitter and would wake the thread
> just like ->request_fn() does now.

You are right!

However, in my comparison I was thinking of how it *can* work if we
were able to submit/prepare request in the context of the caller.

>
>>
>> My point is, to be able to convert to blkmq, we must also avoid
>> performance regression - at leas as long as possible.
>
> It would still be better to start simple, and measure the performance, than
> to guess where the bottlenecks are.

Yes, starting simple is always good!

Although, even if simple, we need to stop adding more mmc specific
hacks into the mmc block device layer.

[...]

Kind regards
Uffe