Re: [PATCH 06/16] mmc: core: replace waitqueue with worker

Linus Walleij
2017-03-09T22:49:19Z

On Wed, Feb 22, 2017 at 2:29 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 09/02/17 17:33, Linus Walleij wrote:
>> The waitqueue in the host context is there to signal back from
>> mmc_request_done() through mmc_wait_data_done() that the hardware
>> is done with a command, and when the wait is over, the core
>> will typically submit the next asynchronous request that is pending
>> just waiting for the hardware to be available.
>>
>> This is in the way for letting the mmc_request_done() trigger the
>> report up to the block layer that a block request is finished.
>>
>> Re-jig this as a first step, remvoving the waitqueue and introducing
>> a work that will run after a completed asynchronous request,
>> finalizing that request, including retransmissions, and eventually
>> reporting back with a completion and a status code to the
>> asynchronous issue method.
>>
>> This had the upside that we can remove the MMC_BLK_NEW_REQUEST
>> status code and the "new_request" state in the request queue
>> that is only there to make the state machine spin out
>> the first time we send a request.
>>
>> Introduce a workqueue in the host for handling just this, and
>> then a work and completion in the asynchronous request to deal
>> with this mechanism.
>>
>> This is a central change that let us do many other changes since
>> we have broken the submit and complete code paths in two, and we
>> can potentially remove the NULL flushing of the asynchronous
>> pipeline and report block requests as finished directly from
>> the worker.
>
> This needs more thought. The completion should go straight to the mmc block
> driver from the ->done() callback. And from there straight back to the
> block layer if recovery is not needed. We want to stop using
> mmc_start_areq() altogether because we never want to wait - we always want
> to issue (if possible) and return.

I don't quite follow this. Isn't what you request exactly what
patch 15/16 "mmc: queue: issue requests in massive parallel"
is doing?

The whole patch series leads up to that.

> The core API to use is __mmc_start_req() but the block driver should
> populate mrq->done with its own handler. i.e. change __mmc_start_req()
>
> - mrq->done = mmc_wait_done;
> + if (!mrq->done)
> + mrq->done = mmc_wait_done;
>
> mrq->done() would complete the request (e.g. via blk_complete_request()) if
> it has no errors (and doesn't need polling), and wake up the queue thread to
> finish up everything else and start the next request.

I think this is what it does at the end of the patch series, patch 15/16.
I have to split it somehow...

> For the blk-mq port, the queue thread should also be retained, partly
> because it solves some synchronization problems, but mostly because, at this
> stage, we anyway don't have solutions for all the different ways the driver
> can block.
> (as listed here https://marc.info/?l=linux-mmc&m=148336571720463&w=2 )

Essentially I take out that thread and replace it with this one worker
introduced in this very patch. I agree the driver can block in many ways
and that is why I need to have it running in process context, and this
is what the worker introduced here provides.

Maybe I'm getting something backwards, sorry then :/

Yours,
Linus Walleij