[PATCH v2 0/2] blk-mq: Allow to complete requests directly

[PATCH v2 0/2] blk-mq: Allow to complete requests directly

[Sebastian Andrzej Siewior]

v1…v2:
 - Drop the SCSI patch for now.
 - Make blk_mq_complete_request_direct() call the completion handler
   directly instead going through struct chain (Jens and hch might had
   the same in mind).

This series converts a part from the MMC layer which completes the
requests from kworker/ preemptible context. Its intention is to avoid
going through the softirq stack in preemptible context which would
involve the ksoftirqd in this case.

Sebastian

[PATCH v2 1/2] blk-mq: Add blk_mq_complete_request_direct()

[Sebastian Andrzej Siewior]

Add blk_mq_complete_request_direct() which completes the block request
directly instead deferring it to softirq for single queue devices.

This is useful for devices which complete the requests in preemptible
context and raising softirq from means scheduling ksoftirqd.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/blk-mq.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 13ba1861e688f..93780c890b479 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -521,6 +521,17 @@ static inline void blk_mq_set_request_complete(struct request *rq)
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
 }
 
+/*
+ * Complete the request directly instead of deferring it to softirq or
+ * completing it another CPU. Useful in preemptible instead of an interrupt.
+ */
+static inline void blk_mq_complete_request_direct(struct request *rq,
+						  void (*complete)(struct request *rq))
+{
+	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
+	complete(rq);
+}
+
 void blk_mq_start_request(struct request *rq);
 void blk_mq_end_request(struct request *rq, blk_status_t error);
 void __blk_mq_end_request(struct request *rq, blk_status_t error);
-- 
2.33.0

[PATCH v2 2/2] mmc: core: Use blk_mq_complete_request_direct().

[Sebastian Andrzej Siewior]

The completion callback for the sdhci-pci device is invoked from a
kworker.
I couldn't identify in which context is mmc_blk_mq_req_done() invoke but
the remaining caller are from invoked from preemptible context. Here it
would make sense to complete the request directly instead scheduling
ksoftirqd for its completion.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mmc/core/block.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 431af5e8be2f8..7d6b43fe52e8a 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2051,7 +2051,8 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)
 		mmc_put_card(mq->card, &mq->ctx);
 }
 
-static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
+static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req,
+				bool can_sleep)
 {
 	struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
 	struct mmc_request *mrq = &mqrq->brq.mrq;
@@ -2063,10 +2064,14 @@ static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
 	 * Block layer timeouts race with completions which means the normal
 	 * completion path cannot be used during recovery.
 	 */
-	if (mq->in_recovery)
+	if (mq->in_recovery) {
 		mmc_blk_mq_complete_rq(mq, req);
-	else if (likely(!blk_should_fake_timeout(req->q)))
-		blk_mq_complete_request(req);
+	} else if (likely(!blk_should_fake_timeout(req->q))) {
+		if (can_sleep)
+			blk_mq_complete_request_direct(req, mmc_blk_mq_complete);
+		else
+			blk_mq_complete_request(req);
+	}
 
 	mmc_blk_mq_dec_in_flight(mq, req);
 }
@@ -2087,7 +2092,7 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq)
 
 	mmc_blk_urgent_bkops(mq, mqrq);
 
-	mmc_blk_mq_post_req(mq, req);
+	mmc_blk_mq_post_req(mq, req, true);
 }
 
 static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
@@ -2106,7 +2111,7 @@ static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
 	if (prev_req)
 		*prev_req = mq->complete_req;
 	else
-		mmc_blk_mq_post_req(mq, mq->complete_req);
+		mmc_blk_mq_post_req(mq, mq->complete_req, true);
 
 	mq->complete_req = NULL;
 
@@ -2178,7 +2183,8 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)
 	mq->rw_wait = false;
 	wake_up(&mq->wait);
 
-	mmc_blk_mq_post_req(mq, req);
+	/* context unknown */
+	mmc_blk_mq_post_req(mq, req, false);
 }
 
 static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)
@@ -2238,7 +2244,7 @@ static int mmc_blk_mq_issue_rw_rq(struct mmc_queue *mq,
 	err = mmc_start_request(host, &mqrq->brq.mrq);
 
 	if (prev_req)
-		mmc_blk_mq_post_req(mq, prev_req);
+		mmc_blk_mq_post_req(mq, prev_req, true);
 
 	if (err)
 		mq->rw_wait = false;
-- 
2.33.0

Re: [PATCH v2 1/2] blk-mq: Add blk_mq_complete_request_direct()

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 03:55:58PM +0200, Sebastian Andrzej Siewior wrote:
> +static inline void blk_mq_complete_request_direct(struct request *rq,
> +						  void (*complete)(struct request *rq))

Pleae avoid the overly long line.

ote that by doing the normal two tab indent for the continuation that
would be super trivial and way more readable.

Re: [PATCH v2 2/2] mmc: core: Use blk_mq_complete_request_direct().

[Ulf Hansson]

+ Adrian

On Mon, 18 Oct 2021 at 15:56, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> The completion callback for the sdhci-pci device is invoked from a
> kworker.
> I couldn't identify in which context is mmc_blk_mq_req_done() invoke but
> the remaining caller are from invoked from preemptible context. Here it
> would make sense to complete the request directly instead scheduling
> ksoftirqd for its completion.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks for working on this!

I have looped in Adrian, to allow him to provide us with his input too.

> ---
>  drivers/mmc/core/block.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 431af5e8be2f8..7d6b43fe52e8a 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2051,7 +2051,8 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)
>                 mmc_put_card(mq->card, &mq->ctx);
>  }
>
> -static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
> +static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req,
> +                               bool can_sleep)
>  {
>         struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
>         struct mmc_request *mrq = &mqrq->brq.mrq;
> @@ -2063,10 +2064,14 @@ static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
>          * Block layer timeouts race with completions which means the normal
>          * completion path cannot be used during recovery.
>          */
> -       if (mq->in_recovery)
> +       if (mq->in_recovery) {
>                 mmc_blk_mq_complete_rq(mq, req);
> -       else if (likely(!blk_should_fake_timeout(req->q)))
> -               blk_mq_complete_request(req);
> +       } else if (likely(!blk_should_fake_timeout(req->q))) {
> +               if (can_sleep)
> +                       blk_mq_complete_request_direct(req, mmc_blk_mq_complete);
> +               else
> +                       blk_mq_complete_request(req);
> +       }
>
>         mmc_blk_mq_dec_in_flight(mq, req);
>  }
> @@ -2087,7 +2092,7 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq)
>
>         mmc_blk_urgent_bkops(mq, mqrq);
>
> -       mmc_blk_mq_post_req(mq, req);
> +       mmc_blk_mq_post_req(mq, req, true);
>  }
>
>  static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
> @@ -2106,7 +2111,7 @@ static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
>         if (prev_req)
>                 *prev_req = mq->complete_req;
>         else
> -               mmc_blk_mq_post_req(mq, mq->complete_req);
> +               mmc_blk_mq_post_req(mq, mq->complete_req, true);
>
>         mq->complete_req = NULL;
>
> @@ -2178,7 +2183,8 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)
>         mq->rw_wait = false;
>         wake_up(&mq->wait);
>
> -       mmc_blk_mq_post_req(mq, req);
> +       /* context unknown */
> +       mmc_blk_mq_post_req(mq, req, false);

So it seems we would benefit from knowing the context here, right?

At this point, what you suggest seems like a reasonable way forward
(assuming atomic context), but in a next step we could potentially add
a non-atomic helper function for mmc host drivers to call, when that
is suitable. Would that make sense you think?

>  }
>
>  static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)
> @@ -2238,7 +2244,7 @@ static int mmc_blk_mq_issue_rw_rq(struct mmc_queue *mq,
>         err = mmc_start_request(host, &mqrq->brq.mrq);
>
>         if (prev_req)
> -               mmc_blk_mq_post_req(mq, prev_req);
> +               mmc_blk_mq_post_req(mq, prev_req, true);
>
>         if (err)
>                 mq->rw_wait = false;

Kind regards
Uffe

Re: [PATCH v2 2/2] mmc: core: Use blk_mq_complete_request_direct().

[Adrian Hunter]

On 19/10/2021 14:32, Ulf Hansson wrote:
> + Adrian
> 
> On Mon, 18 Oct 2021 at 15:56, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>>
>> The completion callback for the sdhci-pci device is invoked from a
>> kworker.
>> I couldn't identify in which context is mmc_blk_mq_req_done() invoke but
>> the remaining caller are from invoked from preemptible context. Here it
>> would make sense to complete the request directly instead scheduling
>> ksoftirqd for its completion.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Thanks for working on this!
> 
> I have looped in Adrian, to allow him to provide us with his input too.

Thanks!

Looks good to me.

Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> 
>> ---
>>  drivers/mmc/core/block.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index 431af5e8be2f8..7d6b43fe52e8a 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -2051,7 +2051,8 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)
>>                 mmc_put_card(mq->card, &mq->ctx);
>>  }
>>
>> -static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
>> +static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req,
>> +                               bool can_sleep)
>>  {
>>         struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
>>         struct mmc_request *mrq = &mqrq->brq.mrq;
>> @@ -2063,10 +2064,14 @@ static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
>>          * Block layer timeouts race with completions which means the normal
>>          * completion path cannot be used during recovery.
>>          */
>> -       if (mq->in_recovery)
>> +       if (mq->in_recovery) {
>>                 mmc_blk_mq_complete_rq(mq, req);
>> -       else if (likely(!blk_should_fake_timeout(req->q)))
>> -               blk_mq_complete_request(req);
>> +       } else if (likely(!blk_should_fake_timeout(req->q))) {
>> +               if (can_sleep)
>> +                       blk_mq_complete_request_direct(req, mmc_blk_mq_complete);
>> +               else
>> +                       blk_mq_complete_request(req);
>> +       }
>>
>>         mmc_blk_mq_dec_in_flight(mq, req);
>>  }
>> @@ -2087,7 +2092,7 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq)
>>
>>         mmc_blk_urgent_bkops(mq, mqrq);
>>
>> -       mmc_blk_mq_post_req(mq, req);
>> +       mmc_blk_mq_post_req(mq, req, true);
>>  }
>>
>>  static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
>> @@ -2106,7 +2111,7 @@ static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
>>         if (prev_req)
>>                 *prev_req = mq->complete_req;
>>         else
>> -               mmc_blk_mq_post_req(mq, mq->complete_req);
>> +               mmc_blk_mq_post_req(mq, mq->complete_req, true);
>>
>>         mq->complete_req = NULL;
>>
>> @@ -2178,7 +2183,8 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)
>>         mq->rw_wait = false;
>>         wake_up(&mq->wait);
>>
>> -       mmc_blk_mq_post_req(mq, req);
>> +       /* context unknown */
>> +       mmc_blk_mq_post_req(mq, req, false);
> 
> So it seems we would benefit from knowing the context here, right?
> 
> At this point, what you suggest seems like a reasonable way forward
> (assuming atomic context), but in a next step we could potentially add
> a non-atomic helper function for mmc host drivers to call, when that
> is suitable. Would that make sense you think?
> 
>>  }
>>
>>  static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)
>> @@ -2238,7 +2244,7 @@ static int mmc_blk_mq_issue_rw_rq(struct mmc_queue *mq,
>>         err = mmc_start_request(host, &mqrq->brq.mrq);
>>
>>         if (prev_req)
>> -               mmc_blk_mq_post_req(mq, prev_req);
>> +               mmc_blk_mq_post_req(mq, prev_req, true);
>>
>>         if (err)
>>                 mq->rw_wait = false;
> 
> Kind regards
> Uffe
> 

Re: [PATCH v2 2/2] mmc: core: Use blk_mq_complete_request_direct().

[Ulf Hansson]

On Mon, 18 Oct 2021 at 15:56, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> The completion callback for the sdhci-pci device is invoked from a
> kworker.
> I couldn't identify in which context is mmc_blk_mq_req_done() invoke but
> the remaining caller are from invoked from preemptible context. Here it
> would make sense to complete the request directly instead scheduling
> ksoftirqd for its completion.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Jens, will you funnel this via your tree?

Kind regards
Uffe

> ---
>  drivers/mmc/core/block.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 431af5e8be2f8..7d6b43fe52e8a 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2051,7 +2051,8 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)
>                 mmc_put_card(mq->card, &mq->ctx);
>  }
>
> -static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
> +static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req,
> +                               bool can_sleep)
>  {
>         struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
>         struct mmc_request *mrq = &mqrq->brq.mrq;
> @@ -2063,10 +2064,14 @@ static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
>          * Block layer timeouts race with completions which means the normal
>          * completion path cannot be used during recovery.
>          */
> -       if (mq->in_recovery)
> +       if (mq->in_recovery) {
>                 mmc_blk_mq_complete_rq(mq, req);
> -       else if (likely(!blk_should_fake_timeout(req->q)))
> -               blk_mq_complete_request(req);
> +       } else if (likely(!blk_should_fake_timeout(req->q))) {
> +               if (can_sleep)
> +                       blk_mq_complete_request_direct(req, mmc_blk_mq_complete);
> +               else
> +                       blk_mq_complete_request(req);
> +       }
>
>         mmc_blk_mq_dec_in_flight(mq, req);
>  }
> @@ -2087,7 +2092,7 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq)
>
>         mmc_blk_urgent_bkops(mq, mqrq);
>
> -       mmc_blk_mq_post_req(mq, req);
> +       mmc_blk_mq_post_req(mq, req, true);
>  }
>
>  static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
> @@ -2106,7 +2111,7 @@ static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
>         if (prev_req)
>                 *prev_req = mq->complete_req;
>         else
> -               mmc_blk_mq_post_req(mq, mq->complete_req);
> +               mmc_blk_mq_post_req(mq, mq->complete_req, true);
>
>         mq->complete_req = NULL;
>
> @@ -2178,7 +2183,8 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)
>         mq->rw_wait = false;
>         wake_up(&mq->wait);
>
> -       mmc_blk_mq_post_req(mq, req);
> +       /* context unknown */
> +       mmc_blk_mq_post_req(mq, req, false);
>  }
>
>  static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)
> @@ -2238,7 +2244,7 @@ static int mmc_blk_mq_issue_rw_rq(struct mmc_queue *mq,
>         err = mmc_start_request(host, &mqrq->brq.mrq);
>
>         if (prev_req)
> -               mmc_blk_mq_post_req(mq, prev_req);
> +               mmc_blk_mq_post_req(mq, prev_req, true);
>
>         if (err)
>                 mq->rw_wait = false;
> --
> 2.33.0
>

Re: [PATCH v2 2/2] mmc: core: Use blk_mq_complete_request_direct().

[Sebastian Andrzej Siewior]

On 2021-10-19 13:32:42 [+0200], Ulf Hansson wrote:
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 431af5e8be2f8..7d6b43fe52e8a 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -2178,7 +2183,8 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)
> >         mq->rw_wait = false;
> >         wake_up(&mq->wait);
> >
> > -       mmc_blk_mq_post_req(mq, req);
> > +       /* context unknown */
> > +       mmc_blk_mq_post_req(mq, req, false);
> 
> So it seems we would benefit from knowing the context here, right?

Yes.

> At this point, what you suggest seems like a reasonable way forward
> (assuming atomic context), but in a next step we could potentially add
> a non-atomic helper function for mmc host drivers to call, when that
> is suitable. Would that make sense you think?

Sure. I didn't mange to figure where this can be called from so I
assumed atomic so it behaves the way it did. If you can provide
additional information here then the additional scheduling of ksoftirqd
could be avoided. Also, if there are drivers which complete their
requests in a threaded-irq handler, then the softirq could be also
avoided (there should be no irq-disabling then).

I *think* there were other completion paths, I just touched the one I
managed to reproduce.

> 
> Kind regards
> Uffe

Sebastian

Re: [PATCH v2 1/2] blk-mq: Add blk_mq_complete_request_direct()

[Sebastian Andrzej Siewior]

On 2021-10-18 08:20:50 [-0700], Christoph Hellwig wrote:
> On Mon, Oct 18, 2021 at 03:55:58PM +0200, Sebastian Andrzej Siewior wrote:
> > +static inline void blk_mq_complete_request_direct(struct request *rq,
> > +						  void (*complete)(struct request *rq))
> 
> Pleae avoid the overly long line.
> 
> ote that by doing the normal two tab indent for the continuation that
> would be super trivial and way more readable.

Oki.

Sebastian