Re: [PATCH V14 14/24] mmc: block: Add CQE support

From: Ulf Hansson
Date: Tue Nov 28 2017 - 06:21:07 EST


On 21 November 2017 at 14:42, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> Add CQE support to the block driver, including:
> - optionally using DCMD for flush requests
> - "manually" issuing discard requests
> - issuing read / write requests to the CQE
> - supporting block-layer timeouts
> - handling recovery
> - supporting re-tuning
>
> CQE offers 25% - 50% better random multi-threaded I/O. There is a slight
> (e.g. 2%) drop in sequential read speed but no observable change to sequential
> write.
>
> CQE automatically sends the commands to complete requests. However it only
> supports reads / writes and so-called "direct commands" (DCMD). Furthermore
> DCMD is limited to one command at a time, but discards require 3 commands.
> That makes issuing discards through CQE very awkward, but some CQE's don't
> support DCMD anyway. So for discards, the existing non-CQE approach is
> taken, where the mmc core code issues the 3 commands one at a time i.e.
> mmc_erase(). Where DCMD is used, is for issuing flushes.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>

This looks good to me!

I only have one, very minor comment.

[...]

> @@ -370,10 +514,14 @@ static int mmc_mq_init_queue(struct mmc_queue *mq, int q_depth,
> static int mmc_mq_init(struct mmc_queue *mq, struct mmc_card *card,
> spinlock_t *lock)
> {
> + struct mmc_host *host = card->host;
> int q_depth;
> int ret;
>
> - q_depth = MMC_QUEUE_DEPTH;
> + if (mq->use_cqe)
> + q_depth = min_t(int, card->ext_csd.cmdq_depth, host->cqe_qdepth);

To make it clear why this is needed, could you please add some comment
in the code?

As I was trying to point out in the other reply about queue depth, for
patch 13, this is weird to me.
This may mean that we end up using queue_depth being less than
MMC_QUEUE_DEPTH (64) for the CQE case. While in fact, in the CQE case
the HW actually supports a bigger queue depth, comparing when not
using CQE.

Anyway, it seems like that will have to be a separate topic to discuss
with the blkmq experts.

> + else
> + q_depth = MMC_QUEUE_DEPTH;
>

[...]

Kind regards
Uffe