Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios

[Konstantin Dorfman <kdorfman@codeaurora.org>]

Hello,

On 11/19/2012 11:34 PM, Per Förlin wrote:

>>>>>>        if (host->areq) {
>>>>>> +             if (!areq)
>>>>>> +                     mmc_prefetch_init(host->areq,
>>>>>> +                                       &host->areq->mrq->completion);
>>>>>>                mmc_wait_for_req_done(host, host->areq->mrq);
>>>>>> +             if (!areq) {
>>>>>> +                     mmc_prefetch_start(host->areq, false);
>>>>>> +                     if (mmc_prefetch_pending(host->areq))
>>>>>> +                             return NULL;
>>> In this case, mmc thread may be unblocked because done() arrived for
>>> current request and not because new request notification. In such a case
>>> we would like the done request to be handled before fetching the new
>>> request.
>> I agree. We wake up and there is no pending new request execution should proceed normally by handling the completed request.
>>
>>> In my code is_done_rcv flag used along with is_new_req flag in
>>> order to differentiate the reason for mmc thread awake.
>> In my patch the return value of "mmc_prefetch_pending()" specifies if thee is a pending request.what should happen.
>> If both current request is done and mmc_prefetch_pending is done at the same time I see there is a problem with my patch. There needs to be a flag to indicate if current request is done.
There is race between done() and NEW_REQUEST events, also when we got 
both of them, the order is to complete current and then to fetch new.


>>>>> I understand your motivation and idea for re-structure the patch. It is
>>>>> still not clear for me how exactly mmc thread will be awaken on new
>>>>> request notification in your version, but I have another problem:
>>>>>
>>>> mmc_request_fn() is called and it calls complete(mq->prefetch_completion) which wakes up the current thread.
>>>> My patch is just an example. The intention is to make the patch cleaner. But I may have missed some of the HPI aspects.
>>>
>>> Is it the lack of functions wrappers that you are using in your example?
>> It's fine to skip the functions wrappers if it makes no sense.
>> What I want to avoid is to create new functions for data_req_start and data_req_wait.
>> I would rather add this to the existing functions in core.c and keep changes in block.c and the flow minimal.
>>
>>> As I understand your example, you mean to implement generic logic on
>>> core/core.c level by using wrapper functions and leave final
>>> implementation for MMC to card/queue.c and for SDIO layer to
>>> card/..sdio.. (I'm not too familiar with sdio protocol implementation).
>>> Well, it is make a lot of sense.
>>>
>> I still have "plans" to add pre/post/async support in the SDIO framework too but I have not got to it.
>> I would be nice to add your NEW_REQ feature along with the other mmc-async features, to make it usable from SDIO.
>>
>>
>>> But the devil is in details - there is a lot of work in
>>> mmc_wait_for_data_req_done(), done() callback and also error handling
>>> changes for card/block.c
>> Things in core.c could be re-used in the SDIO case. In block.c there are only FS specific implementation not relevant for SDIO.
>>
>>>
>>> Do you think, that wait_event() API used not suits the same semantic as
>>> completion API?
>> The waiting mechanims may be wait_event or completion.
>> I'm not in favor of using both. Better to replace completion with wait_event if you prefer.
>>
> I'm fine with wait_event() (block request transfers) combined with completion (for other transfer), as long as the core.c API is intact. I don't see a reason for a new start_data_req()
>
> mmc_start_req() is only used by the mmc block transfers.
The main idea is to change start_req() waiting point 
(mmc_wait_for_data_req_done() function) in such way that external events 
(in the MMC case it is coming from block layer) may awake MMC context 
from waiting for current request. This is done by change the original 
mmc_start_req() function and not changing its signature of mmc_start_req().

May I ask you to see [PATCH v2] patch? I think that is is no change in 
core.c API (by core.c API you mean all functions from core.h?).

Also to use new request feature of core.c one need to implement 
notification similar to mmc_request_fn() and I do not see difficulties 
to do it from SDIO.


> Making a test case in mmc_test.c is a good way to stress test the feature (e.g. random timing of incoming new requests) and it will show how the API works too.
>
> BR
> Per
>
>> My main concern is to avoid adding new API to core.c in order to add the NEW_REQ feature. My patch is an example of changes to be done in core.c without adding new functions.
>>
>>>
>>> We would like to have a generic capability to handle additional events,
>>> such as HPI/stop flow, in addition to the NEW_REQUEST notification.
>>> Therefore, an event mechanism seems to be a better design than completion.
>>>
>>> I've looked at SDIO code and from what I can understand, right now SDIO
>>> is not using async request mechanism and works from 'wait_for_cmd()`
>>> level. This means that such work as exposing MMC core API's is major
>>> change and definitely should be separate design and implementation
>>> effort, while my current patch right now will fix mmc thread behavior
>>> and give better performance for upper layers.
>> You are right. There is no support in the SDIO implementation for pre/post/async feature. Still I had it in mind design the "struct mmc_async". It's possible to re-use the mmc core parts in order to utilize this support in the SDIO case. I'm not saying we should design for SDIO but at least keep the API in a way that it's potential usable from SDIO too. The API of core.c (start/wait of requests) should make sense without the existence of MMC block driver (block/queue).
>>
>> One way to test the design is to add a test in mmc_test.c for penging NEW_REQ. In mmc_test.c there is not block.c or queue.c.
>> If the test case if simple to write in mmc_test.c it's means that the API is on a good level.
I have plans to implement new test in mmc_test.c, but current patch was 
tested using special test I/O scheduler, that is used instead of cfq I/O 
scheduler: "[PATCH v5 1/3] block: Add test-iosched scheduler"

This gives us great power to create any ut scenarios.

-- 
Konstantin Dorfman,
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center,
Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation