Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Ulf Hansson]

On 6 September 2017 at 09:20, Adrian Hunter  wrote:
> On 05/09/17 20:54, Ulf Hansson wrote:
>> On 5 September 2017 at 10:10, Adrian Hunter  wrote:
>>> On 05/09/17 10:24, Ulf Hansson wrote:
 [...]

>>>
>>> I can send blk-mq support for legacy requests in a few days if you 
>>> like, but
>>> I want to hear a better explanation of why you are delaying CQE support.
>>
>> That would be very nice, however be aware of that we are in the merge
>> window, so I am not picking new material for 4.14 from this point. I
>> assume you understand why.
>
> Nope.  This is new functionality - doesn't affect anyone who doesn't have 
> a
> command queue engine.  Next to no chance of regressions.  Tested by 
> several
> in the community.  Substantially unchanged since February.  It is not even
> very much code in the block driver.

 Let me make it clear, once more - I don't want to maintain more hacks
 in mmc block layer code.

 This series add blkmq support, using a method (which may be considered
 as intermediate) via a new change in patch1 - but only for the new CQE
 path. That means the old legacy mmc block path is still there. So, for
 the reason stated above - no thanks!
>>>
>>> And where is your alternative.  When I pointed out you need a way to
>>> arbitrate between internal partitions, you went silent again.
>>>
>>> Can't have CQE without blk-mq but can't have blk-mq because you don't
>>> understand it, is hardly acceptable.
>>
>> Adrian, this discussion seems to lead nowhere. Can we please stop and
>> be constructive instead!
>
> If you want to be constructive you will queue CQE support for v4.15 now!
>
>>
>> Regarding the arbitration issue. We have been moving forward,
>> re-factoring the mmc block driver code, soon also solving the problem
>> for the rpmb internal partition [1]. Maybe the background to why Linus
>> is working on mmc block re-factoring, hasn't been entirely clear.
>> Anyway, it's exactly because of moving closer to address these issues.
>
> Nope, wrt blk-mq you are moving sideways with no clear idea where you're 
> going.
>
>> Even if the problems certainly becomes a step harder to resolve for
>> the boot and the general purpose partitions, it's still a path we
>> should try to find a solution for. Yeah, that may mean we need to
>> suggest changes for the generic block layer, to teach it to better
>> deal with these kind of devices.
>
> You mean you have no idea how to do it but we are still expected to wait.
> How is that acceptable!
>
>> Finally, I have never said the arbitration issue *must* be solved
>> before converting to blkmq. Only that we should avoid performance
>> regressions, but that of course applies to whatever changes we do.
>
> Then there is no problem in queuing up the CQE patches now!

Either you are not paying attention or just being grumpy.

So for the last time, I don't want to pick CQE, prior the existing mmc
block code has converted to blkmq.

End of discussion!

Kind regards
Uffe

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Adrian Hunter]

On 05/09/17 20:54, Ulf Hansson wrote:
> On 5 September 2017 at 10:10, Adrian Hunter  wrote:
>> On 05/09/17 10:24, Ulf Hansson wrote:
>>> [...]
>>>
>>
>> I can send blk-mq support for legacy requests in a few days if you like, 
>> but
>> I want to hear a better explanation of why you are delaying CQE support.
>
> That would be very nice, however be aware of that we are in the merge
> window, so I am not picking new material for 4.14 from this point. I
> assume you understand why.

 Nope.  This is new functionality - doesn't affect anyone who doesn't have a
 command queue engine.  Next to no chance of regressions.  Tested by several
 in the community.  Substantially unchanged since February.  It is not even
 very much code in the block driver.
>>>
>>> Let me make it clear, once more - I don't want to maintain more hacks
>>> in mmc block layer code.
>>>
>>> This series add blkmq support, using a method (which may be considered
>>> as intermediate) via a new change in patch1 - but only for the new CQE
>>> path. That means the old legacy mmc block path is still there. So, for
>>> the reason stated above - no thanks!
>>
>> And where is your alternative.  When I pointed out you need a way to
>> arbitrate between internal partitions, you went silent again.
>>
>> Can't have CQE without blk-mq but can't have blk-mq because you don't
>> understand it, is hardly acceptable.
> 
> Adrian, this discussion seems to lead nowhere. Can we please stop and
> be constructive instead!

If you want to be constructive you will queue CQE support for v4.15 now!

> 
> Regarding the arbitration issue. We have been moving forward,
> re-factoring the mmc block driver code, soon also solving the problem
> for the rpmb internal partition [1]. Maybe the background to why Linus
> is working on mmc block re-factoring, hasn't been entirely clear.
> Anyway, it's exactly because of moving closer to address these issues.

Nope, wrt blk-mq you are moving sideways with no clear idea where you're going.

> Even if the problems certainly becomes a step harder to resolve for
> the boot and the general purpose partitions, it's still a path we
> should try to find a solution for. Yeah, that may mean we need to
> suggest changes for the generic block layer, to teach it to better
> deal with these kind of devices.

You mean you have no idea how to do it but we are still expected to wait.
How is that acceptable!

> Finally, I have never said the arbitration issue *must* be solved
> before converting to blkmq. Only that we should avoid performance
> regressions, but that of course applies to whatever changes we do.

Then there is no problem in queuing up the CQE patches now!

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Ulf Hansson]

On 5 September 2017 at 10:10, Adrian Hunter  wrote:
> On 05/09/17 10:24, Ulf Hansson wrote:
>> [...]
>>
>
> I can send blk-mq support for legacy requests in a few days if you like, 
> but
> I want to hear a better explanation of why you are delaying CQE support.

 That would be very nice, however be aware of that we are in the merge
 window, so I am not picking new material for 4.14 from this point. I
 assume you understand why.
>>>
>>> Nope.  This is new functionality - doesn't affect anyone who doesn't have a
>>> command queue engine.  Next to no chance of regressions.  Tested by several
>>> in the community.  Substantially unchanged since February.  It is not even
>>> very much code in the block driver.
>>
>> Let me make it clear, once more - I don't want to maintain more hacks
>> in mmc block layer code.
>>
>> This series add blkmq support, using a method (which may be considered
>> as intermediate) via a new change in patch1 - but only for the new CQE
>> path. That means the old legacy mmc block path is still there. So, for
>> the reason stated above - no thanks!
>
> And where is your alternative.  When I pointed out you need a way to
> arbitrate between internal partitions, you went silent again.
>
> Can't have CQE without blk-mq but can't have blk-mq because you don't
> understand it, is hardly acceptable.

Adrian, this discussion seems to lead nowhere. Can we please stop and
be constructive instead!

Regarding the arbitration issue. We have been moving forward,
re-factoring the mmc block driver code, soon also solving the problem
for the rpmb internal partition [1]. Maybe the background to why Linus
is working on mmc block re-factoring, hasn't been entirely clear.
Anyway, it's exactly because of moving closer to address these issues.

Even if the problems certainly becomes a step harder to resolve for
the boot and the general purpose partitions, it's still a path we
should try to find a solution for. Yeah, that may mean we need to
suggest changes for the generic block layer, to teach it to better
deal with these kind of devices.

Finally, I have never said the arbitration issue *must* be solved
before converting to blkmq. Only that we should avoid performance
regressions, but that of course applies to whatever changes we do.

[...]

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/9911463/

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Adrian Hunter]

On 05/09/17 10:24, Ulf Hansson wrote:
> [...]
> 

 I can send blk-mq support for legacy requests in a few days if you like, 
 but
 I want to hear a better explanation of why you are delaying CQE support.
>>>
>>> That would be very nice, however be aware of that we are in the merge
>>> window, so I am not picking new material for 4.14 from this point. I
>>> assume you understand why.
>>
>> Nope.  This is new functionality - doesn't affect anyone who doesn't have a
>> command queue engine.  Next to no chance of regressions.  Tested by several
>> in the community.  Substantially unchanged since February.  It is not even
>> very much code in the block driver.
> 
> Let me make it clear, once more - I don't want to maintain more hacks
> in mmc block layer code.
> 
> This series add blkmq support, using a method (which may be considered
> as intermediate) via a new change in patch1 - but only for the new CQE
> path. That means the old legacy mmc block path is still there. So, for
> the reason stated above - no thanks!

And where is your alternative.  When I pointed out you need a way to
arbitrate between internal partitions, you went silent again.

Can't have CQE without blk-mq but can't have blk-mq because you don't
understand it, is hardly acceptable.

> 
>>
>>>
>>> Still, big changes is always nice to queue up early for a release
>>> cycle. Let's aim for that!
>>
>> You said that in February.  Never happened.  You said you wanted blk-mq, so
>> I waited to re-base on top, but it never appeared.
> 
> Yes, I want blkmq - and I believe I have explained why several times by now.
> 
> Unfortunate, blkmq just doesn't appear, we have to work on it - together.

If we are working on it together, how come you have never taken the time to
find out how blk-mq works?

> 
>>
>>> Moreover, I am not delaying CQE, but really want it to be merged asap!
>>> However, I am also having the role as a maintainer and the things that
>>> comes with it. For example, I would like the community to reach
>>> consensus around how to move forward with CQE, before I decide to pick
>>> it up.
>>
>> It has been more than 6 months.  That is enough time to wait for "consensus".
> 
> Normally it should be more than enough, on the other hand, it has
> turned out this was more complex than we first thought.

Nonsense.  I have raised the issues time and again but there have never been
any replies.

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Ulf Hansson]

[...]

>>>
>>> I can send blk-mq support for legacy requests in a few days if you like, but
>>> I want to hear a better explanation of why you are delaying CQE support.
>>
>> That would be very nice, however be aware of that we are in the merge
>> window, so I am not picking new material for 4.14 from this point. I
>> assume you understand why.
>
> Nope.  This is new functionality - doesn't affect anyone who doesn't have a
> command queue engine.  Next to no chance of regressions.  Tested by several
> in the community.  Substantially unchanged since February.  It is not even
> very much code in the block driver.

Let me make it clear, once more - I don't want to maintain more hacks
in mmc block layer code.

This series add blkmq support, using a method (which may be considered
as intermediate) via a new change in patch1 - but only for the new CQE
path. That means the old legacy mmc block path is still there. So, for
the reason stated above - no thanks!

>
>>
>> Still, big changes is always nice to queue up early for a release
>> cycle. Let's aim for that!
>
> You said that in February.  Never happened.  You said you wanted blk-mq, so
> I waited to re-base on top, but it never appeared.

Yes, I want blkmq - and I believe I have explained why several times by now.

Unfortunate, blkmq just doesn't appear, we have to work on it - together.

>
>> Moreover, I am not delaying CQE, but really want it to be merged asap!
>> However, I am also having the role as a maintainer and the things that
>> comes with it. For example, I would like the community to reach
>> consensus around how to move forward with CQE, before I decide to pick
>> it up.
>
> It has been more than 6 months.  That is enough time to wait for "consensus".

Normally it should be more than enough, on the other hand, it has
turned out this was more complex than we first thought.

Kind regards
Uffe

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Ulf Hansson]

On 4 September 2017 at 09:06, Adrian Hunter  wrote:
> On 01/09/17 16:28, Adrian Hunter wrote:
>> On 01/09/17 15:58, Ulf Hansson wrote:
>>> + Christoph
>>>
>>> On 1 September 2017 at 13:42, Adrian Hunter  wrote:
 On 31/08/17 14:56, Adrian Hunter wrote:
> Here is V7 of the hardware command queue patches without the software
> command queue patches, now using blk-mq.
>
> HW CMDQ offers 25% - 50% better random multi-threaded I/O.  I see a slight
> 2% drop in sequential read speed but no change to sequential write.

 Any comments?
>>>
>>> A couple of overall comments, for now.
>>>
>>> To make sure we don't overlook something when converting to mq, I
>>> would prefer that we first convert the existing mmc block code to mq,
>>> then we add CMDQ on top.
>>
>> That doesn't make sense.  This patch set is not converting the legacy driver
>> to mq therefore it cannot overlook anything for converting to mq.
>
> And then you go silent again.

We have weekends in Sweden - and I also work on other things than mmc :-).

I do however admit, that I could have been reviewing a bit faster
throughout the re-spins. Apologize for that, but I am only doing my
best.

>
> I can send blk-mq support for legacy requests in a few days if you like, but
> I want to hear a better explanation of why you are delaying CQE support.

That would be very nice, however be aware of that we are in the merge
window, so I am not picking new material for 4.14 from this point. I
assume you understand why.

Still, big changes is always nice to queue up early for a release
cycle. Let's aim for that!

Moreover, I am not delaying CQE, but really want it to be merged asap!
However, I am also having the role as a maintainer and the things that
comes with it. For example, I would like the community to reach
consensus around how to move forward with CQE, before I decide to pick
it up.

Kind regards
Uffe

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Adrian Hunter]

On 01/09/17 16:28, Adrian Hunter wrote:
> On 01/09/17 15:58, Ulf Hansson wrote:
>> + Christoph
>>
>> On 1 September 2017 at 13:42, Adrian Hunter  wrote:
>>> On 31/08/17 14:56, Adrian Hunter wrote:
 Here is V7 of the hardware command queue patches without the software
 command queue patches, now using blk-mq.

 HW CMDQ offers 25% - 50% better random multi-threaded I/O.  I see a slight
 2% drop in sequential read speed but no change to sequential write.
>>>
>>> Any comments?
>>
>> A couple of overall comments, for now.
>>
>> To make sure we don't overlook something when converting to mq, I
>> would prefer that we first convert the existing mmc block code to mq,
>> then we add CMDQ on top.
> 
> That doesn't make sense.  This patch set is not converting the legacy driver
> to mq therefore it cannot overlook anything for converting to mq.

And then you go silent again.

I can send blk-mq support for legacy requests in a few days if you like, but
I want to hear a better explanation of why you are delaying CQE support.

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Adrian Hunter]

On 01/09/17 15:58, Ulf Hansson wrote:
> + Christoph
> 
> On 1 September 2017 at 13:42, Adrian Hunter  wrote:
>> On 31/08/17 14:56, Adrian Hunter wrote:
>>> Here is V7 of the hardware command queue patches without the software
>>> command queue patches, now using blk-mq.
>>>
>>> HW CMDQ offers 25% - 50% better random multi-threaded I/O.  I see a slight
>>> 2% drop in sequential read speed but no change to sequential write.
>>
>> Any comments?
> 
> A couple of overall comments, for now.
> 
> To make sure we don't overlook something when converting to mq, I
> would prefer that we first convert the existing mmc block code to mq,
> then we add CMDQ on top.

That doesn't make sense.  This patch set is not converting the legacy driver
to mq therefore it cannot overlook anything for converting to mq.

> 
> Regarding patch1. in the long-term solution, we should really strive
> towards getting rid of the big mmc host lock, as it causes us to lose
> some of the benefits on which principles mq is designed upon.

Lose what benefits?

> However, it may be feasible to use patch1 as an intermediate step, to
> be able to convert to mq short term. Then we can continue doing the
> re-factoring, to get rid of the big mmc host lock and solve other
> relates issues/optimize things. What remains to be seen in this
> approach, is how well mq performs, as we would still be using the big
> mmc host lock. We may tolerate some minor regressions, but if too
> much, there is no other way that first removing the big mmc lock.

Unlike HDDs, eMMC must switch between internal partitions (LUNs) so there
must a way to arbitrate between them.

> Of course, if Linus/Christoph don't think this intermediate approach
> is good enough, we will have to respect that as well.

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Ulf Hansson]

+ Christoph

On 1 September 2017 at 13:42, Adrian Hunter  wrote:
> On 31/08/17 14:56, Adrian Hunter wrote:
>> Here is V7 of the hardware command queue patches without the software
>> command queue patches, now using blk-mq.
>>
>> HW CMDQ offers 25% - 50% better random multi-threaded I/O.  I see a slight
>> 2% drop in sequential read speed but no change to sequential write.
>
> Any comments?

A couple of overall comments, for now.

To make sure we don't overlook something when converting to mq, I
would prefer that we first convert the existing mmc block code to mq,
then we add CMDQ on top.

Regarding patch1. in the long-term solution, we should really strive
towards getting rid of the big mmc host lock, as it causes us to lose
some of the benefits on which principles mq is designed upon.

However, it may be feasible to use patch1 as an intermediate step, to
be able to convert to mq short term. Then we can continue doing the
re-factoring, to get rid of the big mmc host lock and solve other
relates issues/optimize things. What remains to be seen in this
approach, is how well mq performs, as we would still be using the big
mmc host lock. We may tolerate some minor regressions, but if too
much, there is no other way that first removing the big mmc lock.

Of course, if Linus/Christoph don't think this intermediate approach
is good enough, we will have to respect that as well.

Kind regards
Uffe

Re: [PATCH V7 00/10] mmc: Add Command Queue support

[Adrian Hunter]

On 31/08/17 14:56, Adrian Hunter wrote:
> Here is V7 of the hardware command queue patches without the software
> command queue patches, now using blk-mq.
> 
> HW CMDQ offers 25% - 50% better random multi-threaded I/O.  I see a slight
> 2% drop in sequential read speed but no change to sequential write.

Any comments?

[PATCH V7 00/10] mmc: Add Command Queue support

[Adrian Hunter]

Hi

Here is V7 of the hardware command queue patches without the software
command queue patches, now using blk-mq.

HW CMDQ offers 25% - 50% better random multi-threaded I/O.  I see a slight
2% drop in sequential read speed but no change to sequential write.


Changes since V6:
  mmc: core: Introduce host claiming by context
New patch.
  mmc: core: Move mmc_start_areq() declaration
Dropped because it has been applied
  mmc: block: Fix block status codes
Dropped because it has been applied
  mmc: host: Add CQE interface
Dropped because it has been applied
  mmc: core: Turn off CQE before sending commands
Dropped because it has been applied
  mmc: block: Factor out mmc_setup_queue()
New patch.
  mmc: block: Add CQE support
Drop legacy support and add blk-mq support

Changes since V5:
Re-based
  mmc: core: Add mmc_retune_hold_now()
Dropped because it has been applied
  mmc: core: Add members to mmc_request and mmc_data for CQE's
Dropped because it has been applied
  mmc: core: Move mmc_start_areq() declaration
New patch at Ulf's request
  mmc: block: Fix block status codes
Another un-related patch
  mmc: host: Add CQE interface
Move recovery_notifier() callback to struct mmc_request
  mmc: core: Add support for handling CQE requests
Roll __mmc_cqe_request_done() into mmc_cqe_request_done()
Move function declarations requested by Ulf
  mmc: core: Remove unused MMC_CAP2_PACKED_CMD
Dropped because it has been applied
  mmc: block: Add CQE support
Add explanation to commit message
Adjustment for changed recovery_notifier() callback
  mmc: cqhci: support for command queue enabled host
Adjustment for changed recovery_notifier() callback
  mmc: sdhci-pci: Add CQHCI support for Intel GLK
Add DCMD capability for Intel controllers except GLK

Changes since V4:
  mmc: core: Add mmc_retune_hold_now()
Add explanation to commit message.
  mmc: host: Add CQE interface
Add comments to callback declarations.
  mmc: core: Turn off CQE before sending commands
Add explanation to commit message.
  mmc: core: Add support for handling CQE requests
Add comments as requested by Ulf.
  mmc: core: Remove unused MMC_CAP2_PACKED_CMD
New patch.
  mmc: mmc: Enable Command Queuing
Adjust for removal of MMC_CAP2_PACKED_CMD.
Add a comment about Packed Commands.
  mmc: mmc: Enable CQE's
Remove un-necessary check for MMC_CAP2_CQE
  mmc: block: Use local variables in mmc_blk_data_prep()
New patch.
  mmc: block: Prepare CQE data
Adjust due to "mmc: block: Use local variables in mmc_blk_data_prep()"
Remove priority setting.
Add explanation to commit message.
  mmc: cqhci: support for command queue enabled host
Fix transfer descriptor setting in cqhci_set_tran_desc() for 32-bit DMA

Changes since V3:
Adjusted ...blk_end_request...() for new block status codes
Fixed CQHCI transaction descriptor for "no DCMD" case

Changes since V2:
Dropped patches that have been applied.
Re-based
Added "mmc: sdhci-pci: Add CQHCI support for Intel GLK"

Changes since V1:

"Share mmc request array between partitions" is dependent
on changes in "Introduce queue semantics", so added that
and block fixes:

Added "Fix is_waiting_last_req set incorrectly"
Added "Fix cmd error reset failure path"
Added "Use local var for mqrq_cur"
Added "Introduce queue semantics"

Changes since RFC:

Re-based on next.
Added comment about command queue priority.
Added some acks and reviews.


Adrian Hunter (9):
  mmc: core: Introduce host claiming by context
  mmc: core: Add support for handling CQE requests
  mmc: mmc: Enable Command Queuing
  mmc: mmc: Enable CQE's
  mmc: block: Use local variables in mmc_blk_data_prep()
  mmc: block: Prepare CQE data
  mmc: block: Factor out mmc_setup_queue()
  mmc: block: Add CQE support
  mmc: sdhci-pci: Add CQHCI support for Intel GLK

Venkat Gopalakrishnan (1):
  mmc: cqhci: support for command queue enabled host

 drivers/mmc/core/block.c  |  260 -
 drivers/mmc/core/block.h  |7 +
 drivers/mmc/core/bus.c|7 +
 drivers/mmc/core/core.c   |  272 -
 drivers/mmc/core/core.h   |   12 +
 drivers/mmc/core/mmc.c|   29 +
 drivers/mmc/core/queue.c  |  438 --
 drivers/mmc/core/queue.h  |   46 +-
 drivers/mmc/host/Kconfig  |   14 +
 drivers/mmc/host/Makefile |1 +
 drivers/mmc/host/cqhci.c  | 1154 +
 drivers/mmc/host/cqhci.h  |  240