| Message ID | 20211018135559.244400-3-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: Allow to complete requests directly | expand |
+ 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
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 >
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 >
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
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;
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(-)