[00/12,v5] Multiqueue for MMC/SD

Message ID 20171110100143.12256-1-linus.walleij@linaro.org
Headers show
Series
  • Multiqueue for MMC/SD
Related show

Message

Linus Walleij Nov. 10, 2017, 10:01 a.m.
This is the fifth iteration of this patch set.

I *HOPE* that we can scrap this patch set and merge Adrian's
patches instead, because they also bring CQE support which is
nice. I had some review comments on his series, mainly that
it needs to kill off the legacy block layer code path that
noone likes anyway.

So this is mainly an academic and inspirational exercise.
Whatever remains of this refactoring, if anything, I can
certainly do on top of Adrian's patches as well.

What changed since v4 is the error path, since Adrian pointed
out that the error handling seems to be fragile. It was indeed
fragile... To make sure things work properly I have run long
test rounds with fault injection, essentially:

Enable FAULT_INJECTION, FAULT_INJECTION_DEBUG_FS,
       FAIL_MMC_REQUEST
cd /debug/mmc3/fail_mmc_request/
echo 1 > probability
echo -1 > times

Then running a dd to the card, also increased the error rate
to 10% and completed tests successfully, but at this error
rate the MMC stack sometimes exceeds the retry limit and the
dd command fails (as is appropriate).

Removing a card during I/O does not work well however :/
So I guess I would need to work on that if this series should
continue. (Hopefully unlikely.)


Linus Walleij (12):
  mmc: core: move the asynchronous post-processing
  mmc: core: add a workqueue for completing requests
  mmc: core: replace waitqueue with worker
  mmc: core: do away with is_done_rcv
  mmc: core: do away with is_new_req
  mmc: core: kill off the context info
  mmc: queue: simplify queue logic
  mmc: block: shuffle retry and error handling
  mmc: queue: stop flushing the pipeline with NULL
  mmc: queue/block: pass around struct mmc_queue_req*s
  mmc: block: issue requests in massive parallel
  mmc: switch MMC/SD to use blk-mq multiqueueing v5

 drivers/mmc/core/block.c    | 557 +++++++++++++++++++++++---------------------
 drivers/mmc/core/block.h    |   5 +-
 drivers/mmc/core/bus.c      |   1 -
 drivers/mmc/core/core.c     | 217 ++++++++++-------
 drivers/mmc/core/core.h     |  11 +-
 drivers/mmc/core/host.c     |   1 -
 drivers/mmc/core/mmc_test.c |  31 +--
 drivers/mmc/core/queue.c    | 252 ++++++++------------
 drivers/mmc/core/queue.h    |  16 +-
 include/linux/mmc/core.h    |   3 +-
 include/linux/mmc/host.h    |  31 +--
 11 files changed, 557 insertions(+), 568 deletions(-)

-- 
2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Walleij Nov. 10, 2017, 1:39 p.m. | #1
On Fri, Nov 10, 2017 at 11:01 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:

> Removing a card during I/O does not work well however :/

> So I guess I would need to work on that if this series should

> continue. (Hopefully unlikely.)


I tested a bit more and it turns out this doesn't work on any of
the MQ patch sets.

Which matches Christoph's statement that this is not really
working. I haven't really analyzed why though, I can see that
the kernel crashes firmly into a brick wall, but on mainline
it does not.

I think it's just something we have to smoke out in the next
release cycle as we switch to MQ.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Nov. 10, 2017, 3:24 p.m. | #2
On 10 November 2017 at 11:01, Linus Walleij <linus.walleij@linaro.org> wrote:
> This is the fifth iteration of this patch set.

>

> I *HOPE* that we can scrap this patch set and merge Adrian's

> patches instead, because they also bring CQE support which is

> nice. I had some review comments on his series, mainly that

> it needs to kill off the legacy block layer code path that

> noone likes anyway.

>

> So this is mainly an academic and inspirational exercise.

> Whatever remains of this refactoring, if anything, I can

> certainly do on top of Adrian's patches as well.

>

> What changed since v4 is the error path, since Adrian pointed

> out that the error handling seems to be fragile. It was indeed

> fragile... To make sure things work properly I have run long

> test rounds with fault injection, essentially:


Please correct me if I am wrong, the issues was observed already in
patch 11, before the actual switch to mq was done, right?

>

> Enable FAULT_INJECTION, FAULT_INJECTION_DEBUG_FS,

>        FAIL_MMC_REQUEST

> cd /debug/mmc3/fail_mmc_request/

> echo 1 > probability

> echo -1 > times

>

> Then running a dd to the card, also increased the error rate

> to 10% and completed tests successfully, but at this error

> rate the MMC stack sometimes exceeds the retry limit and the

> dd command fails (as is appropriate).


That's great. I really appreciate that you have run these tests, that
gives me a good confidence from an overall point of view.

>

> Removing a card during I/O does not work well however :/

> So I guess I would need to work on that if this series should

> continue. (Hopefully unlikely.)


Yeah, this has actually been rather cumbersome to deal with also in
the legacy request path. Let's dive into this in more detail as soon
as possible.

>

>

> Linus Walleij (12):

>   mmc: core: move the asynchronous post-processing

>   mmc: core: add a workqueue for completing requests

>   mmc: core: replace waitqueue with worker

>   mmc: core: do away with is_done_rcv

>   mmc: core: do away with is_new_req

>   mmc: core: kill off the context info

>   mmc: queue: simplify queue logic

>   mmc: block: shuffle retry and error handling

>   mmc: queue: stop flushing the pipeline with NULL

>   mmc: queue/block: pass around struct mmc_queue_req*s

>   mmc: block: issue requests in massive parallel

>   mmc: switch MMC/SD to use blk-mq multiqueueing v5

>

>  drivers/mmc/core/block.c    | 557 +++++++++++++++++++++++---------------------

>  drivers/mmc/core/block.h    |   5 +-

>  drivers/mmc/core/bus.c      |   1 -

>  drivers/mmc/core/core.c     | 217 ++++++++++-------

>  drivers/mmc/core/core.h     |  11 +-

>  drivers/mmc/core/host.c     |   1 -

>  drivers/mmc/core/mmc_test.c |  31 +--

>  drivers/mmc/core/queue.c    | 252 ++++++++------------

>  drivers/mmc/core/queue.h    |  16 +-

>  include/linux/mmc/core.h    |   3 +-

>  include/linux/mmc/host.h    |  31 +--

>  11 files changed, 557 insertions(+), 568 deletions(-)


First, I haven't yet commented on the latest version of the mq patch
(patch12 in this series) and neither on Adrians (patch 3 in his
series), but before doing that, let me share my overall view of how we
could move forward, as to see if all of us can agree on that path.

So, what I really like in $subject series is the step by step method,
moving slowly forward enables an easy review, then the actual switch
to mq gets a diff of "3 files changed, 156 insertions(+), 181
deletions(-)". This shows to me, that it can be done! Great work!
Of course, I do realize that you may not have considered all
preparations needed for CQE, which Adrian may have thought of in his
mq patch from his series (patch 3), but still.

Moreover, for reasons brought up while reviewing Adrian's series,
regarding if mq is "ready", and because I see that the diff for patch
12 is small, I suggest that we just skip the step adding a Kconfig
option to allow an opt-in of the mq path. In other words, *the* patch
that makes the switch to mq, should also remove the entire left over
of rubbish code, from the legacy request path. That's also what you do
in patch12, nice!

Finally, I understand that you would be happy to scrap this series,
but instead let Adrian's series, when re-posted, to go first. Could
you perhaps re-consider that, because I wonder if it may not be
smother and less risky, to actually apply everything up to patch 11 in
this series?

I noticed that you reported issues with card removal during I/O (for
both yours and Adrian's mq patch), but does those problems exists at
patch 11 - or is those explicitly introduced with the mk patch (patch
12)?

Of course, I realize that if we apply everything up to patch11, that
would require a massive re-base of Adrian's mq/CQE series, but on the
other hand, then matter which mq patch we decide to go with, it should
be a rather small diff, thus easy to review and less risky.

Adrian, Linus - what do you think?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz Nov. 14, 2017, 12:17 p.m. | #3
Hi Linus,

On Friday, November 10, 2017 11:01:31 AM Linus Walleij wrote:
> This is the fifth iteration of this patch set.

> 

> I *HOPE* that we can scrap this patch set and merge Adrian's

> patches instead, because they also bring CQE support which is

> nice. I had some review comments on his series, mainly that

> it needs to kill off the legacy block layer code path that

> noone likes anyway.

> 

> So this is mainly an academic and inspirational exercise.

> Whatever remains of this refactoring, if anything, I can

> certainly do on top of Adrian's patches as well.

> 

> What changed since v4 is the error path, since Adrian pointed

> out that the error handling seems to be fragile. It was indeed

> fragile... To make sure things work properly I have run long

> test rounds with fault injection, essentially:

> 

> Enable FAULT_INJECTION, FAULT_INJECTION_DEBUG_FS,

>        FAIL_MMC_REQUEST

> cd /debug/mmc3/fail_mmc_request/

> echo 1 > probability

> echo -1 > times

> 

> Then running a dd to the card, also increased the error rate

> to 10% and completed tests successfully, but at this error

> rate the MMC stack sometimes exceeds the retry limit and the

> dd command fails (as is appropriate).

> 

> Removing a card during I/O does not work well however :/

> So I guess I would need to work on that if this series should

> continue. (Hopefully unlikely.)

> 

> 

> Linus Walleij (12):

>   mmc: core: move the asynchronous post-processing

>   mmc: core: add a workqueue for completing requests

>   mmc: core: replace waitqueue with worker

>   mmc: core: do away with is_done_rcv

>   mmc: core: do away with is_new_req

>   mmc: core: kill off the context info

>   mmc: queue: simplify queue logic

>   mmc: block: shuffle retry and error handling

>   mmc: queue: stop flushing the pipeline with NULL

>   mmc: queue/block: pass around struct mmc_queue_req*s

>   mmc: block: issue requests in massive parallel

>   mmc: switch MMC/SD to use blk-mq multiqueueing v5

> 

>  drivers/mmc/core/block.c    | 557 +++++++++++++++++++++++---------------------

>  drivers/mmc/core/block.h    |   5 +-

>  drivers/mmc/core/bus.c      |   1 -

>  drivers/mmc/core/core.c     | 217 ++++++++++-------

>  drivers/mmc/core/core.h     |  11 +-

>  drivers/mmc/core/host.c     |   1 -

>  drivers/mmc/core/mmc_test.c |  31 +--

>  drivers/mmc/core/queue.c    | 252 ++++++++------------

>  drivers/mmc/core/queue.h    |  16 +-

>  include/linux/mmc/core.h    |   3 +-

>  include/linux/mmc/host.h    |  31 +--

>  11 files changed, 557 insertions(+), 568 deletions(-)


This works much better than initial version and a simple dd read
test shows more consistent results than with vanilla kernel.

However there are still some issues:

1. 30 seconds delay on "Waiting for /dev to be fully populated..."
   during boot

2. reboot command no longer works (there is a livelock after
   "The system is going down for reboot NOW!" message)

Full log (together with SysRq-l & SysRq-t outputs) attached.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Bartlomiej Zolnierkiewicz Nov. 14, 2017, 1:30 p.m. | #4
On Tuesday, November 14, 2017 01:17:34 PM Bartlomiej Zolnierkiewicz wrote:

> This works much better than initial version and a simple dd read

> test shows more consistent results than with vanilla kernel.

> 

> However there are still some issues:

> 

> 1. 30 seconds delay on "Waiting for /dev to be fully populated..."

>    during boot

> 

> 2. reboot command no longer works (there is a livelock after

>    "The system is going down for reboot NOW!" message)

> 

> Full log (together with SysRq-l & SysRq-t outputs) attached.


BTW: these problems are not present in Adrian's V13 patchset
(with mmc-mq enabled by default).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Nov. 14, 2017, 9:17 p.m. | #5
On Fri, Nov 10, 2017 at 4:24 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 10 November 2017 at 11:01, Linus Walleij <linus.walleij@linaro.org> wrote:

>> This is the fifth iteration of this patch set.

>>

>> I *HOPE* that we can scrap this patch set and merge Adrian's

>> patches instead, because they also bring CQE support which is

>> nice. I had some review comments on his series, mainly that

>> it needs to kill off the legacy block layer code path that

>> noone likes anyway.

>>

>> So this is mainly an academic and inspirational exercise.

>> Whatever remains of this refactoring, if anything, I can

>> certainly do on top of Adrian's patches as well.

>>

>> What changed since v4 is the error path, since Adrian pointed

>> out that the error handling seems to be fragile. It was indeed

>> fragile... To make sure things work properly I have run long

>> test rounds with fault injection, essentially:

>

> Please correct me if I am wrong, the issues was observed already in

> patch 11, before the actual switch to mq was done, right?


Yes. That's where I fixed it up mostly.

> Moreover, for reasons brought up while reviewing Adrian's series,

> regarding if mq is "ready", and because I see that the diff for patch

> 12 is small, I suggest that we just skip the step adding a Kconfig

> option to allow an opt-in of the mq path. In other words, *the* patch

> that makes the switch to mq, should also remove the entire left over

> of rubbish code, from the legacy request path. That's also what you do

> in patch12, nice!


Partly true.

Adrian also pointed out the rubbishness of the error handling code
in the old stack, and my patch set does *not* fix that. It is also a part
of his patch set I like very much and a reason why I would prefer to
use Adrian's patches if possible.

We have the following risk factors:

- Observed performance degradation of 1% (on x86 SDHI I guess)
- The kernel crashes if SD card is removed (both patch sets)
- The risk of something nasty happening we don't know of

> Finally, I understand that you would be happy to scrap this series,

> but instead let Adrian's series, when re-posted, to go first. Could

> you perhaps re-consider that, because I wonder if it may not be

> smother and less risky, to actually apply everything up to patch 11 in

> this series?


This is possible.

But I think it is preferred to proceed with Adrian's patches.
I really like the looks of the code. He says he's coming back with
a set that also kills off the old block layer, and I am pretty
positive I will just ACK the whole thing.

I optimistically think we can jointly fix the card removal issue
and possible also mitigate or root-cause the performance
degradation observed by Adrian

In the best of worlds, Ming Lei's patches will just fix this too
(we'll see, we can probably get a branch from the block people
to try it) else we can use tracing and perf to drill into it I guess.

> I noticed that you reported issues with card removal during I/O (for

> both yours and Adrian's mq patch), but does those problems exists at

> patch 11 - or is those explicitly introduced with the mk patch (patch

> 12)?


I tested it and it is present earlier in the series. I would have to
revisit and hash it out.

> Of course, I realize that if we apply everything up to patch11, that

> would require a massive re-base of Adrian's mq/CQE series, but on the

> other hand, then matter which mq patch we decide to go with, it should

> be a rather small diff, thus easy to review and less risky.


At this point I would prefer to use Adrian's series. He has explained
pretty well his reasoning and when I tested the code it was performing
well. I have some outstanding thingies, but this I can just as well do
on top of his patches.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Nov. 14, 2017, 9:19 p.m. | #6
On Tue, Nov 14, 2017 at 2:30 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> On Tuesday, November 14, 2017 01:17:34 PM Bartlomiej Zolnierkiewicz wrote:

>

>> This works much better than initial version and a simple dd read

>> test shows more consistent results than with vanilla kernel.

>>

>> However there are still some issues:

>>

>> 1. 30 seconds delay on "Waiting for /dev to be fully populated..."

>>    during boot

>>

>> 2. reboot command no longer works (there is a livelock after

>>    "The system is going down for reboot NOW!" message)

>>

>> Full log (together with SysRq-l & SysRq-t outputs) attached.

>

> BTW: these problems are not present in Adrian's V13 patchset

> (with mmc-mq enabled by default).


Yes, as I even say in the cover letter I think his patches are
better so we should use those.

Bart can you provide a Tested-by for Adrian' patch set?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Nov. 15, 2017, 10:24 a.m. | #7
[...]

>> Moreover, for reasons brought up while reviewing Adrian's series,

>> regarding if mq is "ready", and because I see that the diff for patch

>> 12 is small, I suggest that we just skip the step adding a Kconfig

>> option to allow an opt-in of the mq path. In other words, *the* patch

>> that makes the switch to mq, should also remove the entire left over

>> of rubbish code, from the legacy request path. That's also what you do

>> in patch12, nice!

>

> Partly true.

>

> Adrian also pointed out the rubbishness of the error handling code

> in the old stack, and my patch set does *not* fix that. It is also a part

> of his patch set I like very much and a reason why I would prefer to

> use Adrian's patches if possible.

>

> We have the following risk factors:

>

> - Observed performance degradation of 1% (on x86 SDHI I guess)


I don't think that small degradation is a reason for not enabling mq,
although for sure we should continue to investigate why it is.

> - The kernel crashes if SD card is removed (both patch sets)


Yep, this needs to be fixed.

> - The risk of something nasty happening we don't know of


:-)

>

>> Finally, I understand that you would be happy to scrap this series,

>> but instead let Adrian's series, when re-posted, to go first. Could

>> you perhaps re-consider that, because I wonder if it may not be

>> smother and less risky, to actually apply everything up to patch 11 in

>> this series?

>

> This is possible.

>

> But I think it is preferred to proceed with Adrian's patches.

> I really like the looks of the code. He says he's coming back with

> a set that also kills off the old block layer, and I am pretty

> positive I will just ACK the whole thing.


Alright, let's go with this option!

>

> I optimistically think we can jointly fix the card removal issue

> and possible also mitigate or root-cause the performance

> degradation observed by Adrian

>

> In the best of worlds, Ming Lei's patches will just fix this too

> (we'll see, we can probably get a branch from the block people

> to try it) else we can use tracing and perf to drill into it I guess.


I think most of Ming's patches addressing performance issues should be
in Linus' master already, so once I have a my next branch based on
4.15rc1, we should be able to run a new round of test.

Anyway, if anything else is needed from the generic block layer, sure
I am open to pull in a branch if needed.

>

>> I noticed that you reported issues with card removal during I/O (for

>> both yours and Adrian's mq patch), but does those problems exists at

>> patch 11 - or is those explicitly introduced with the mk patch (patch

>> 12)?

>

> I tested it and it is present earlier in the series. I would have to

> revisit and hash it out.


Right. So, let's then forget about my suggested approach and spend
time more wisely on Adrian's series.

>

>> Of course, I realize that if we apply everything up to patch11, that

>> would require a massive re-base of Adrian's mq/CQE series, but on the

>> other hand, then matter which mq patch we decide to go with, it should

>> be a rather small diff, thus easy to review and less risky.

>

> At this point I would prefer to use Adrian's series. He has explained

> pretty well his reasoning and when I tested the code it was performing

> well. I have some outstanding thingies, but this I can just as well do

> on top of his patches.


Yep. As stated above, let's go for that solution.

I will then be awaiting a new version from Adrian, hopefully I can
apply his series already when rc1 is out, to make sure we get enough
time to smoke out any remaining problems.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter Nov. 15, 2017, 1:50 p.m. | #8
On 14/11/17 23:17, Linus Walleij wrote:
> We have the following risk factors:

> 

> - Observed performance degradation of 1% (on x86 SDHI I guess)

> - The kernel crashes if SD card is removed (both patch sets)


I haven't been able to reproduce that.  Do you have more information?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Nov. 29, 2017, 1:13 p.m. | #9
On Wed, Nov 15, 2017 at 2:50 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 14/11/17 23:17, Linus Walleij wrote:

>> We have the following risk factors:

>>

>> - Observed performance degradation of 1% (on x86 SDHI I guess)

>> - The kernel crashes if SD card is removed (both patch sets)

>

> I haven't been able to reproduce that.  Do you have more information?


I saw it in an earlier version of the patch set, but it might be due to
some confusion on my side.

I will try to get this series going and stress it a bit and see what happens.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html