Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant
On 11/26/2012 11:27 AM, Russell King - ARM Linux wrote:
> On Mon, Nov 26, 2012 at 11:20:32AM +0100, Per Förlin wrote:
>> On 11/22/2012 06:37 PM, Russell King - ARM Linux wrote:
>>> On Thu, Nov 22, 2012 at 06:28:30PM +0100, Per Forlin wrote:
>>>> On Wed, Nov 21, 2012 at 5:50 PM, Russell King - ARM Linux
>>>> wrote:
>>>>> On Wed, Nov 21, 2012 at 05:13:55PM +0100, Per Forlin wrote:
>>>>>> On Wed, Nov 21, 2012 at 4:38 PM, Russell King - ARM Linux
>>>>>> wrote:
>>>>>>> On Fri, Oct 12, 2012 at 04:02:02PM +0200, Ulf Hansson wrote:
>>>>>>>> /*
>>>>>>>> + * Validate mmc prerequisites
>>>>>>>> + */
>>>>>>>> +static int mmci_validate_data(struct mmci_host *host,
>>>>>>>> + struct mmc_data *data)
>>>>>>>> +{
>>>>>>>> + if (!data)
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + if (!host->variant->non_power_of_2_blksize &&
>>>>>>>> + !is_power_of_2(data->blksz)) {
>>>>>>>> + dev_err(mmc_dev(host->mmc),
>>>>>>>> + "unsupported block size (%d bytes)\n",
>>>>>>>> data->blksz);
>>>>>>>> + return -EINVAL;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + if (data->sg->offset & 3) {
>>>>>>>> + dev_err(mmc_dev(host->mmc),
>>>>>>>> + "unsupported alginment (0x%x)\n",
>>>>>>>> data->sg->offset);
>>>>>>>> + return -EINVAL;
>>>>>>>> + }
>>>>>>>
>>>>>>> Why? What's the reasoning behind this suddenly introduced restriction?
>>>>>>> readsl()/writesl() copes just fine with non-aligned pointers. It may be
>>>>>>> that your DMA engine can not, but that's no business interfering with
>>>>>>> non-DMA transfers, and no reason to fail such transfers.
>>>>>>>
>>>>>>> If your DMA engine can't do that then its your DMA engine code which
>>>>>>> should refuse to prepare the transfer.
>>>>>>>
>>>>>>> Yes, that means problems with the way things are ordered - or it needs a
>>>>>>> proper API where DMA engine can export these kinds of properties.
>>>>>> The alignment constraint is related to PIO, sg_miter and that FIFO
>>>>>> access must be done with 4 bytes.
>>>>>
>>>>> Total claptrap. No it isn't.
>>>>>
>>>>> - sg_miter just deals with bytes, and number of bytes transferred; there
>>>>> is no word assumptions in that code. Indeed many ATA disks transfer
>>>>> by half-word accesses so such a restriction would be insane.
>>>>>
>>>>> - the FIFO access itself needs to be 32-bit words, so readsl or writesl
>>>>> (or their io* equivalents must be used).
>>>>>
>>>>> - but - and this is the killer item to your argument as I said above -
>>>>> readsl and writesl _can_ take misaligned pointers and cope with them
>>>>> fine.
>>>>>
>>>>> The actual alignment of the buffer address is totally irrelevant here.
>>>>>
>>>>> What isn't irrelevant is the _number_ of bytes to be transferred, but
>>>>> that's something totally different and completely unrelated from
>>>>> data->sg->offset.
>>>> Let's try again :)
>>>>
>>>> Keep in mind that the mmc -block layer is aligned so from that aspect
>>>> everything is fine.
>>>> SDIO may have any length or alignment but sg-len is always 1.
>>>>
>>>> There is just one sg-element and one continues buffer.
>>>>
>>>> sg-miter splits the continues buffer in chunks that may not be aligned
>>>> with 4 byte size. It depends on the start address alignment of the
>>>> buffer.
>>>>
>>>> Is it more clear now?
>>>
>>> Is this more clear: you may be passed a single buffer which is misaligned.
>>> We cop
Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant
On 11/22/2012 06:37 PM, Russell King - ARM Linux wrote:
> On Thu, Nov 22, 2012 at 06:28:30PM +0100, Per Forlin wrote:
>> On Wed, Nov 21, 2012 at 5:50 PM, Russell King - ARM Linux
>> wrote:
>>> On Wed, Nov 21, 2012 at 05:13:55PM +0100, Per Forlin wrote:
On Wed, Nov 21, 2012 at 4:38 PM, Russell King - ARM Linux
wrote:
> On Fri, Oct 12, 2012 at 04:02:02PM +0200, Ulf Hansson wrote:
>> /*
>> + * Validate mmc prerequisites
>> + */
>> +static int mmci_validate_data(struct mmci_host *host,
>> + struct mmc_data *data)
>> +{
>> + if (!data)
>> + return 0;
>> +
>> + if (!host->variant->non_power_of_2_blksize &&
>> + !is_power_of_2(data->blksz)) {
>> + dev_err(mmc_dev(host->mmc),
>> + "unsupported block size (%d bytes)\n",
>> data->blksz);
>> + return -EINVAL;
>> + }
>> +
>> + if (data->sg->offset & 3) {
>> + dev_err(mmc_dev(host->mmc),
>> + "unsupported alginment (0x%x)\n",
>> data->sg->offset);
>> + return -EINVAL;
>> + }
>
> Why? What's the reasoning behind this suddenly introduced restriction?
> readsl()/writesl() copes just fine with non-aligned pointers. It may be
> that your DMA engine can not, but that's no business interfering with
> non-DMA transfers, and no reason to fail such transfers.
>
> If your DMA engine can't do that then its your DMA engine code which
> should refuse to prepare the transfer.
>
> Yes, that means problems with the way things are ordered - or it needs a
> proper API where DMA engine can export these kinds of properties.
The alignment constraint is related to PIO, sg_miter and that FIFO
access must be done with 4 bytes.
>>>
>>> Total claptrap. No it isn't.
>>>
>>> - sg_miter just deals with bytes, and number of bytes transferred; there
>>> is no word assumptions in that code. Indeed many ATA disks transfer
>>> by half-word accesses so such a restriction would be insane.
>>>
>>> - the FIFO access itself needs to be 32-bit words, so readsl or writesl
>>> (or their io* equivalents must be used).
>>>
>>> - but - and this is the killer item to your argument as I said above -
>>> readsl and writesl _can_ take misaligned pointers and cope with them
>>> fine.
>>>
>>> The actual alignment of the buffer address is totally irrelevant here.
>>>
>>> What isn't irrelevant is the _number_ of bytes to be transferred, but
>>> that's something totally different and completely unrelated from
>>> data->sg->offset.
>> Let's try again :)
>>
>> Keep in mind that the mmc -block layer is aligned so from that aspect
>> everything is fine.
>> SDIO may have any length or alignment but sg-len is always 1.
>>
>> There is just one sg-element and one continues buffer.
>>
>> sg-miter splits the continues buffer in chunks that may not be aligned
>> with 4 byte size. It depends on the start address alignment of the
>> buffer.
>>
>> Is it more clear now?
>
> Is this more clear: you may be passed a single buffer which is misaligned.
> We cope with that just fine. There is *no* reason to reject that transfer
> because readsl/writesl cope just fine with it.
>
The MMCI driver doesn't support alignment smaller than 4 bytes (it may result
in data corruption).
There are two options
1. Either one should fix the driver to support it. Currently the driver only
supports miss-alignment of the last sg-miter buffer.
2. Or be kind to inform the user that the alignment is not supported.
BR
Per
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On 11/19/2012 03:32 PM, Per Förlin wrote:
> On 11/19/2012 10:48 AM, Konstantin Dorfman wrote:
>> Hello Per,
>>
>> Thank you for giving me an example, below I'm trying to explain some
>> logic issues of it and asking you some questions about your vision of
>> the patch.
>>
>> On 11/15/2012 06:38 PM, Per Förlin wrote:
>>> On 11/14/2012 04:15 PM, Konstantin Dorfman wrote:
>>>> Hello Per,
>>>>
>>>> On 11/13/2012 11:10 PM, Per Forlin wrote:
>>>>> On Tue, Oct 30, 2012 at 1:19 PM, Konstantin Dorfman
>>>>> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> On 10/29/2012 11:40 PM, Per Forlin wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I would like to move the focus of my concerns from root cause analysis
>>>>>>> to the actual patch,
>>>>>>> My first reflection is that the patch is relatively complex and some
>>>>>>> of the code looks duplicated.
>>>>>>> Would it be possible to simplify it and re-use the current execution
>>>>>>> flow.
>>>>>>>
>>>>>>> Is the following flow feasible?
>>>>>>>
>>>>>>> in mmc_start_req():
>>>>>>> --
>>>>>>> if (rqc == NULL && not_resend)
>>>>>>> wait_for_both_mmc_and_arrival_of_new_req
>>>>>>> else
>>>>>>> wait_only_for_mmc
>>>>>>>
>>>>>>> if (arrival_of_new_req) {
>>>>>>>set flag to indicate fetch-new_req
>>>>>>> return NULL;
>>>>>>> }
>>>>>>> -
>>>>>>>
>>>>>>> in queue.c:
>>>>>>> if (fetch-new_req)
>>>>>>> don't overwrite previous req.
>>>>>>>
>>>>>>> BR
>>>>>>> Per
>>>>>>
>>>>>> You are talking about using both waiting mechanisms, old (wait on
>>>>>> completion) and new - wait_event_interruptible()? But how done()
>>>>>> callback, called on host controller irq context, will differentiate
>>>>>> which one is relevant for the request?
>>>>>>
>>>>>> I think it is better to use single wait point for mmc thread.
>>>>>
>>>>> I have sketch a patch to better explain my point. It's not tested it
>>>>> barely compiles :)
>>>>> The patch tries to introduce your feature and still keep the same code
>>>>> path. And it exposes an API that could be used by SDIO as well.
>>>>> The intention of my sketch patch is only to explain what I tried to
>>>>> visualize in the pseudo code previously in this thread.
>>>>> The out come of your final patch should be documented here I think:
>>>>> Documentation/mmc/mmc-async-req.txt
>>>> This document is ready, attaching it to this mail and will be included
>>>> in next version of the patch (or RESEND).
>>>>>
>>>>> Here follows my patch sketch:
>>>>>
>>>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>>>>> index e360a97..08036a1 100644
>>>>> --- a/drivers/mmc/card/queue.c
>>>>> +++ b/drivers/mmc/card/queue.c
>>>>> @@ -66,6 +66,8 @@ static int mmc_queue_thread(void *d)
>>>>> spin_unlock_irq(q->queue_lock);
>>>>>
>>>>> if (req || mq->mqrq_prev->req) {
>>>>> + if (!req)
>>>>> +
>>>>> mmc_prefetch_start(&mq->mqrq_prev->mmc_active, true);
>>>>> set_current_state(TASK_RUNNING);
>>>>> mq->issue_fn(mq, req);
>>>>> } else {
>>>>> @@ -79,11 +81,13 @@ static int mmc_queue_thread(void *d)
>>>>> }
>>>>>
>>>>> /* Current request becomes previous request and vice versa.
>>>>> */
>>>>> - mq->mqrq_prev->brq.mrq.data = NULL;
>>>>> - mq->mqrq_prev->req = NULL;
>>>>> - tmp = mq->mqrq_prev;
>>
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On 11/19/2012 10:48 AM, Konstantin Dorfman wrote:
> Hello Per,
>
> Thank you for giving me an example, below I'm trying to explain some
> logic issues of it and asking you some questions about your vision of
> the patch.
>
> On 11/15/2012 06:38 PM, Per Förlin wrote:
>> On 11/14/2012 04:15 PM, Konstantin Dorfman wrote:
>>> Hello Per,
>>>
>>> On 11/13/2012 11:10 PM, Per Forlin wrote:
>>>> On Tue, Oct 30, 2012 at 1:19 PM, Konstantin Dorfman
>>>> wrote:
>>>>> Hello,
>>>>>
>>>>> On 10/29/2012 11:40 PM, Per Forlin wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I would like to move the focus of my concerns from root cause analysis
>>>>>> to the actual patch,
>>>>>> My first reflection is that the patch is relatively complex and some
>>>>>> of the code looks duplicated.
>>>>>> Would it be possible to simplify it and re-use the current execution
>>>>>> flow.
>>>>>>
>>>>>> Is the following flow feasible?
>>>>>>
>>>>>> in mmc_start_req():
>>>>>> --
>>>>>> if (rqc == NULL && not_resend)
>>>>>> wait_for_both_mmc_and_arrival_of_new_req
>>>>>> else
>>>>>> wait_only_for_mmc
>>>>>>
>>>>>> if (arrival_of_new_req) {
>>>>>>set flag to indicate fetch-new_req
>>>>>> return NULL;
>>>>>> }
>>>>>> -
>>>>>>
>>>>>> in queue.c:
>>>>>> if (fetch-new_req)
>>>>>> don't overwrite previous req.
>>>>>>
>>>>>> BR
>>>>>> Per
>>>>>
>>>>> You are talking about using both waiting mechanisms, old (wait on
>>>>> completion) and new - wait_event_interruptible()? But how done()
>>>>> callback, called on host controller irq context, will differentiate
>>>>> which one is relevant for the request?
>>>>>
>>>>> I think it is better to use single wait point for mmc thread.
>>>>
>>>> I have sketch a patch to better explain my point. It's not tested it
>>>> barely compiles :)
>>>> The patch tries to introduce your feature and still keep the same code
>>>> path. And it exposes an API that could be used by SDIO as well.
>>>> The intention of my sketch patch is only to explain what I tried to
>>>> visualize in the pseudo code previously in this thread.
>>>> The out come of your final patch should be documented here I think:
>>>> Documentation/mmc/mmc-async-req.txt
>>> This document is ready, attaching it to this mail and will be included
>>> in next version of the patch (or RESEND).
>>>>
>>>> Here follows my patch sketch:
>>>>
>>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>>>> index e360a97..08036a1 100644
>>>> --- a/drivers/mmc/card/queue.c
>>>> +++ b/drivers/mmc/card/queue.c
>>>> @@ -66,6 +66,8 @@ static int mmc_queue_thread(void *d)
>>>> spin_unlock_irq(q->queue_lock);
>>>>
>>>> if (req || mq->mqrq_prev->req) {
>>>> + if (!req)
>>>> +
>>>> mmc_prefetch_start(&mq->mqrq_prev->mmc_active, true);
>>>> set_current_state(TASK_RUNNING);
>>>> mq->issue_fn(mq, req);
>>>> } else {
>>>> @@ -79,11 +81,13 @@ static int mmc_queue_thread(void *d)
>>>> }
>>>>
>>>> /* Current request becomes previous request and vice versa.
>>>> */
>>>> - mq->mqrq_prev->brq.mrq.data = NULL;
>>>> - mq->mqrq_prev->req = NULL;
>>>> - tmp = mq->mqrq_prev;
>>>> - mq->mqrq_prev = mq->mqrq_cur;
>>>> - mq->mqrq_cur = tmp;
>>>> + if (!mmc_prefetch_pending(&mq->mqrq_prev->mmc_active)) {
>>>> + mq->mqrq_prev->brq.mrq.data = NULL;
>>>> + mq->mqrq_prev->req = NULL;
>>>> +
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On 11/14/2012 04:15 PM, Konstantin Dorfman wrote:
> Hello Per,
>
> On 11/13/2012 11:10 PM, Per Forlin wrote:
>> On Tue, Oct 30, 2012 at 1:19 PM, Konstantin Dorfman
>> wrote:
>>> Hello,
>>>
>>> On 10/29/2012 11:40 PM, Per Forlin wrote:
Hi,
I would like to move the focus of my concerns from root cause analysis
to the actual patch,
My first reflection is that the patch is relatively complex and some
of the code looks duplicated.
Would it be possible to simplify it and re-use the current execution flow.
Is the following flow feasible?
in mmc_start_req():
--
if (rqc == NULL && not_resend)
wait_for_both_mmc_and_arrival_of_new_req
else
wait_only_for_mmc
if (arrival_of_new_req) {
set flag to indicate fetch-new_req
return NULL;
}
-
in queue.c:
if (fetch-new_req)
don't overwrite previous req.
BR
Per
>>>
>>> You are talking about using both waiting mechanisms, old (wait on
>>> completion) and new - wait_event_interruptible()? But how done()
>>> callback, called on host controller irq context, will differentiate
>>> which one is relevant for the request?
>>>
>>> I think it is better to use single wait point for mmc thread.
>>
>> I have sketch a patch to better explain my point. It's not tested it
>> barely compiles :)
>> The patch tries to introduce your feature and still keep the same code
>> path. And it exposes an API that could be used by SDIO as well.
>> The intention of my sketch patch is only to explain what I tried to
>> visualize in the pseudo code previously in this thread.
>> The out come of your final patch should be documented here I think:
>> Documentation/mmc/mmc-async-req.txt
> This document is ready, attaching it to this mail and will be included
> in next version of the patch (or RESEND).
>>
>> Here follows my patch sketch:
>>
>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>> index e360a97..08036a1 100644
>> --- a/drivers/mmc/card/queue.c
>> +++ b/drivers/mmc/card/queue.c
>> @@ -66,6 +66,8 @@ static int mmc_queue_thread(void *d)
>> spin_unlock_irq(q->queue_lock);
>>
>> if (req || mq->mqrq_prev->req) {
>> + if (!req)
>> + mmc_prefetch_start(&mq->mqrq_prev->mmc_active,
>> true);
>> set_current_state(TASK_RUNNING);
>> mq->issue_fn(mq, req);
>> } else {
>> @@ -79,11 +81,13 @@ static int mmc_queue_thread(void *d)
>> }
>>
>> /* Current request becomes previous request and vice versa. */
>> - mq->mqrq_prev->brq.mrq.data = NULL;
>> - mq->mqrq_prev->req = NULL;
>> - tmp = mq->mqrq_prev;
>> - mq->mqrq_prev = mq->mqrq_cur;
>> - mq->mqrq_cur = tmp;
>> + if (!mmc_prefetch_pending(&mq->mqrq_prev->mmc_active)) {
>> + mq->mqrq_prev->brq.mrq.data = NULL;
>> + mq->mqrq_prev->req = NULL;
>> + tmp = mq->mqrq_prev;
>> + mq->mqrq_prev = mq->mqrq_cur;
>> + mq->mqrq_cur = tmp;
>> + }
>> } while (1);
>> up(&mq->thread_sem);
>>
>> @@ -109,10 +113,44 @@ static void mmc_request_fn(struct request_queue *q)
>> return;
>> }
>>
>> + if (mq->prefetch_enable) {
>> + spin_lock(&mq->prefetch_lock);
>> + if (mq->prefetch_completion)
>> + complete(mq->prefetch_completion);
>> + mq->prefetch_pending = true;
>> + spin_unlock(&mq->prefetch_lock);
>> + }
>> +
>> if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)
>> wake_up_process(mq->thread);
>> }
>>
>> +static void mmc_req_init(struct mmc_async_req *areq, struct completion
>> *compl)
>> +{
>> + struct mmc_queue *mq =
>> + container_of(areq->prefetch, struct mmc_queue, prefetch);
>> +
>> + spin_lock(&mq->prefetch_lock);
>> + mq->prefetch_completion = compl;
>> + if (mq->prefetch_pending)
>> + complete(mq->prefetch_completion);
>> + spin_unlock(&mq->prefetch_lock);
>> +}
>> +
>> +static void mmc_req_start(struct mmc_async_req *areq, bool enable)
>> +{
>> + struct mmc_queue *mq =
>> + container_of(areq->prefetch, struct mmc_queue, prefetch);
>> + mq->prefetch_enable = enable;
>> +}
>> +
>> +static bool mmc_req_pending(struct mmc_async_req *areq)
>> +{
>> + struct mmc_queue *mq =
>> + container_of(areq->prefetch, struct mmc_queue, prefetch);
>> + return mq->prefetch_pending;
>> +}
>> +
>> static struct scatterlist *mmc_alloc_sg(int sg_len, int *err)
>> {
>> struct scatterlist *sg;
>> @@ -166,6 +204,12 @@ int mmc_init_queue(struct mmc_queue *mq, struct
>> mmc_card *card,
>>
Re: [PATCH v2] mmc: fix async request mechanism for sequential read scenarios
Hi Konstantin,
On 11/01/2012 03:40 PM, Konstantin Dorfman wrote:
> When current request is running on the bus and if next request fetched
> by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the
> current request completes. This means if new request comes in while
> the mmcqd thread is blocked, this new request can not be prepared in
> parallel to current ongoing request. This may result in latency to
> start new request.
>
> This change allows to wake up the MMC thread (which
> is waiting for the current running request to complete). Now once the
> MMC thread is woken up, new request can be fetched and prepared in
> parallel to current running request which means this new request can
> be started immediately after the current running request completes.
>
> With this change read throughput is improved by 16%.
What HW and what test cases have you been running?
>
> Signed-off-by: Konstantin Dorfman
> ---
Please add a change log here with a brief description of the changes since last
version
> drivers/mmc/card/block.c | 26 +---
> drivers/mmc/card/queue.c | 26 ++-
> drivers/mmc/card/queue.h |3 +
> drivers/mmc/core/core.c | 102 -
> include/linux/mmc/card.h | 12 +
> include/linux/mmc/core.h | 15 +++
> 6 files changed, 163 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 172a768..0e9bedb 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -112,17 +112,6 @@ struct mmc_blk_data {
>
> static DEFINE_MUTEX(open_lock);
>
> -enum mmc_blk_status {
> - MMC_BLK_SUCCESS = 0,
> - MMC_BLK_PARTIAL,
> - MMC_BLK_CMD_ERR,
> - MMC_BLK_RETRY,
> - MMC_BLK_ABORT,
> - MMC_BLK_DATA_ERR,
> - MMC_BLK_ECC_ERR,
> - MMC_BLK_NOMEDIUM,
> -};
> -
> module_param(perdev_minors, int, 0444);
> MODULE_PARM_DESC(perdev_minors, "Minors numbers to allocate per device");
>
> @@ -1225,6 +1214,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req
> *mqrq,
>
> mqrq->mmc_active.mrq = &brq->mrq;
> mqrq->mmc_active.err_check = mmc_blk_err_check;
> + mqrq->mmc_active.mrq->context_info = &mq->context_info;
>
> mmc_queue_bounce_pre(mqrq);
> }
> @@ -1284,9 +1274,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
> struct request *rqc)
> areq = &mq->mqrq_cur->mmc_active;
> } else
> areq = NULL;
> - areq = mmc_start_req(card->host, areq, (int *) &status);
> - if (!areq)
> + areq = mmc_start_req(card->host, areq, (int *)&status);
> + if (!areq) {
> + if (status == MMC_BLK_NEW_REQUEST)
> + mq->flags |= MMC_QUEUE_NEW_REQUEST;
> return 0;
> + }
>
> mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
> brq = &mq_rq->brq;
> @@ -1295,6 +1288,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
> struct request *rqc)
> mmc_queue_bounce_post(mq_rq);
>
> switch (status) {
> + case MMC_BLK_NEW_REQUEST:
> + BUG_ON(1); /* should never get here */
> case MMC_BLK_SUCCESS:
> case MMC_BLK_PARTIAL:
> /*
> @@ -1367,7 +1362,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
> struct request *rqc)
> * prepare it again and resend.
> */
> mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
> - mmc_start_req(card->host, &mq_rq->mmc_active, NULL);
> + mmc_start_req(card->host, &mq_rq->mmc_active, (int
> *)&status);
> }
> } while (ret);
>
> @@ -1406,6 +1401,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
> ret = 0;
> goto out;
> }
> + mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
>
> if (req && req->cmd_flags & REQ_DISCARD) {
> /* complete ongoing async transfer before issuing discard */
> @@ -1426,7 +1422,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
> }
>
> out:
> - if (!req)
> + if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
> /* release host only when there are no more requests */
> mmc_release_host(card->host);
> return ret;
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index fadf52e..7375476 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -22,7 +22,6 @@
>
> #define MMC_QUEUE_BOUNCESZ 65536
>
> -#define MMC_QUEUE_SUSPENDED(1 << 0)
>
> /*
> * Prepare a MMC request. This just filters out odd stuff.
> @@ -63,11 +62,17
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On 10/25/2012 03:28 PM, Konstantin Dorfman wrote: > On 10/24/2012 07:07 PM, Per Förlin wrote: >> On 10/24/2012 11:41 AM, Konstantin Dorfman wrote: >>> Hello Per, >>> >>> On Mon, October 22, 2012 1:02 am, Per Forlin wrote: >>>>> When mmcqt reports on completion of a request there should be >>>>> a context switch to allow the insertion of the next read ahead BIOs >>>>> to the block layer. Since the mmcqd tries to fetch another request >>>>> immediately after the completion of the previous request it gets NULL >>>>> and starts waiting for the completion of the previous request. >>>>> This wait on completion gives the FS the opportunity to insert the next >>>>> request but the MMC layer is already blocked on the previous request >>>>> completion and is not aware of the new request waiting to be fetched. >>>> I thought that I could trigger a context switch in order to give >>>> execution time for FS to add the new request to the MMC queue. >>>> I made a simple hack to call yield() in case the request gets NULL. I >>>> thought it may give the FS layer enough time to add a new request to >>>> the MMC queue. This would not delay the MMC transfer since the yield() >>>> is done in parallel with an ongoing transfer. Anyway it was just meant >>>> to be a simple test. >>>> >>>> One yield was not enough. Just for sanity check I added a msleep as >>>> well and that was enough to let FS add a new request, >>>> Would it be possible to gain throughput by delaying the fetch of new >>>> request? Too avoid unnecessary NULL requests >>>> >>>> If (ongoing request is read AND size is max read ahead AND new request >>>> is NULL) yield(); >>>> >>>> BR >>>> Per >>> We did the same experiment and it will not give maximum possible >>> performance. There is no guarantee that the context switch which was >>> manually caused by the MMC layer comes just in time: when it was early >>> then next fetch still results in NULL, when it was later, then we miss >>> possibility to fetch/prepare new request. >>> >>> Any delay in fetch of the new request that comes after the new request has >>> arrived hits throughput and latency. >>> >>> The solution we are talking about here will fix not only situation with FS >>> read ahead mechanism, but also it will remove penalty of the MMC context >>> waiting on completion while any new request arrives. >>> >>> Thanks, >>> >> It seems strange that the block layer cannot keep up with relatively slow >> flash media devices. There must be a limitation on number of outstanding >> request towards MMC. >> I need to make up my mind if it's the best way to address this issue in the >> MMC framework or block layer. I have started to look into the block layer >> code but it will take some time to dig out the relevant parts. >> >> BR >> Per >> > The root cause of the issue in incompletion of the current design with > well known producer-consumer problem solution (producer is block layer, > consumer is mmc layer). > Classic definitions states that the buffer is fix size, in our case we > have queue, so Producer always capable to put new request into the queue. > Consumer context blocked when both buffers (curr and prev) are busy > (first started its execution on the bus, second is fetched and waiting > for the first). This happens but I thought that the block layer would continue to add request to the MMC queue while the consumer is busy. When consumer fetches request from the queue again there should be several requests available in the queue, but there is only one. > Producer context considered to be blocked when FS (or others bio > sources) has no requests to put into queue. Does the block layer ever wait for outstanding request to finish? Could this be another reason why the producer doesn't add new requests on the MMC queue? > To maximize performance there are 2 notifications should be used: > 1. Producer notifies Consumer about new item to proceed. > 2. Consumer notifies Producer about free place. > > In our case 2nd notification not need since as I said before - it is > always free space in the queue. > There is no such notification as 1st, i.e. block layer has no way to > notify mmc layer about new request arrived. > > What you suggesting is to resolve specific case, when FS READ_AHEAD > mechanism behavior causes delays in producing new requests. > Probably you can res
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On 10/24/2012 11:41 AM, Konstantin Dorfman wrote: > Hello Per, > > On Mon, October 22, 2012 1:02 am, Per Forlin wrote: >>> When mmcqt reports on completion of a request there should be >>> a context switch to allow the insertion of the next read ahead BIOs >>> to the block layer. Since the mmcqd tries to fetch another request >>> immediately after the completion of the previous request it gets NULL >>> and starts waiting for the completion of the previous request. >>> This wait on completion gives the FS the opportunity to insert the next >>> request but the MMC layer is already blocked on the previous request >>> completion and is not aware of the new request waiting to be fetched. >> I thought that I could trigger a context switch in order to give >> execution time for FS to add the new request to the MMC queue. >> I made a simple hack to call yield() in case the request gets NULL. I >> thought it may give the FS layer enough time to add a new request to >> the MMC queue. This would not delay the MMC transfer since the yield() >> is done in parallel with an ongoing transfer. Anyway it was just meant >> to be a simple test. >> >> One yield was not enough. Just for sanity check I added a msleep as >> well and that was enough to let FS add a new request, >> Would it be possible to gain throughput by delaying the fetch of new >> request? Too avoid unnecessary NULL requests >> >> If (ongoing request is read AND size is max read ahead AND new request >> is NULL) yield(); >> >> BR >> Per > We did the same experiment and it will not give maximum possible > performance. There is no guarantee that the context switch which was > manually caused by the MMC layer comes just in time: when it was early > then next fetch still results in NULL, when it was later, then we miss > possibility to fetch/prepare new request. > > Any delay in fetch of the new request that comes after the new request has > arrived hits throughput and latency. > > The solution we are talking about here will fix not only situation with FS > read ahead mechanism, but also it will remove penalty of the MMC context > waiting on completion while any new request arrives. > > Thanks, > It seems strange that the block layer cannot keep up with relatively slow flash media devices. There must be a limitation on number of outstanding request towards MMC. I need to make up my mind if it's the best way to address this issue in the MMC framework or block layer. I have started to look into the block layer code but it will take some time to dig out the relevant parts. BR Per -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: mmci: assume maintainership
On 03/15/2012 03:30 PM, Linus WALLEIJ wrote: > From: Linus Walleij > > So since this driver is crucial for us (as in ST-Ericsson) > to have actively maintained, and since it is marked orphaned > I will assume maintainership for it starting with the 3.5 > integration cycle. The idea is to collect patches and send > either patch sets of pull requests to Chris. > > Cc: Ulf Hansson > Cc: Per Forlin > Cc: Pawel Moll > Cc: Russell King > Signed-off-by: Linus Walleij > --- > MAINTAINERS |3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3321d75..3f280f6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -630,7 +630,8 @@ F:drivers/input/serio/ambakmi.* > F: include/linux/amba/kmi.h > > ARM PRIMECELL MMCI PL180/1 DRIVER > -S: Orphan > +M: Linus Walleij > +L: linux-mmc@vger.kernel.org > F: drivers/mmc/host/mmci.* > > ARM PRIMECELL BUS SUPPORT Acked-by: Per Forlin Thanks, Per -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Clean up after mmc_pre_req if card was removed
On 03/05/2012 07:08 AM, Jaehoon Chung wrote:
> On 03/05/2012 02:08 PM, Jaehoon Chung wrote:
>
>> On 03/03/2012 12:29 AM, Per Förlin wrote:
>>
>>> On 03/02/2012 09:51 AM, Ulf HANSSON wrote:
>>>> Hi Jaehoon,
>>>>
>>>> I did not know this. Which host driver are you using? I would very much
>>>> appreciate of you could debug and share some result.
>>>>
>>>> Thanks!
>>>>
>>>> BR
>>>> Ulf Hansson
>>>>
>>>> On 03/02/2012 09:28 AM, Jaehoon Chung wrote:
>>>>> Hi Ulf.
>>>>>
>>>>> I tested with this patch.
>>>>> But in my environment, this patch didn't work fine before.
>>>>> 1) When remove/insert, didn't entered the suspend.
>>>>> 2) When removed during something write,
>>>>> [ 50.755067] FAT-fs (mmcblk1p1): Directory bread(block 8254) failed
>>>>> [ 50.761235] FAT-fs (mmcblk1p1): Directory bread(block 8255) failed
>>>>> then at next-time, didn't detect sd-card.
>>>>>
>>>>> Did you know this?
>>>>> If you want more information, i will debug, and share the result.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>> On 03/02/2012 12:44 AM, Ulf Hansson wrote:
>>>>>
>>>>>> Make sure mmc_start_req cancel the prepared job, if the request
>>>>>> was prevented to be started due to the card has been removed.
>>>>>>
>>>>>> This bug was introduced in commit:
>>>>>> mmc: allow upper layers to know immediately if card has been removed
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson
>>>>>> ---
>>>>>> drivers/mmc/core/core.c | 35 +++
>>>>>> 1 files changed, 15 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>> index 0b317f0..9e562ab 100644
>>>>>> --- a/drivers/mmc/core/core.c
>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>> @@ -249,16 +249,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
>>>>>> complete(&mrq->completion);
>>>>>> }
>>>>>>
>>>>>> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request
>>>>>> *mrq)
>>>>>> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request
>>>>>> *mrq)
>>>>>> {
>>>>>> init_completion(&mrq->completion);
>>>>>> mrq->done = mmc_wait_done;
>>>>>> if (mmc_card_removed(host->card)) {
>>>>>> mrq->cmd->error = -ENOMEDIUM;
>>>>>> complete(&mrq->completion);
>>>>>> -return;
>>>>>> +return -ENOMEDIUM;
>>>>>> }
>>>>>> mmc_start_request(host, mrq);
>>>>>> +return 0;
>>>>>> }
>>>>>>
>>>>>> static void mmc_wait_for_req_done(struct mmc_host *host,
>>>>>> @@ -342,6 +343,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host
>>>>>> *host,
>>>>>> struct mmc_async_req *areq, int
>>>>>> *error)
>>>>>> {
>>>>>> int err = 0;
>>>>>> +int start_err = 0;
>>>>>> struct mmc_async_req *data = host->areq;
>>>>>>
>>>>>> /* Prepare a new request */
>>>>>> @@ -351,30 +353,23 @@ struct mmc_async_req *mmc_start_req(struct
>>>>>> mmc_host *host,
>>>>>> if (host->areq) {
>>>>>> mmc_wait_for_req_done(host, host->areq->mrq);
>>>>>> err = host->areq->err_check(host->card, host->areq);
>>>>>> -if (err) {
>>>>>> -/* post process the completed failed request */
>>>>>> -mmc_post_req(host, host->areq->mrq, 0);
>>>>>> -if (areq)
>>>>>> -
Re: [PATCH] mmc: core: Clean up after mmc_pre_req if card was removed
On 03/02/2012 09:51 AM, Ulf HANSSON wrote:
> Hi Jaehoon,
>
> I did not know this. Which host driver are you using? I would very much
> appreciate of you could debug and share some result.
>
> Thanks!
>
> BR
> Ulf Hansson
>
> On 03/02/2012 09:28 AM, Jaehoon Chung wrote:
>> Hi Ulf.
>>
>> I tested with this patch.
>> But in my environment, this patch didn't work fine before.
>> 1) When remove/insert, didn't entered the suspend.
>> 2) When removed during something write,
>> [ 50.755067] FAT-fs (mmcblk1p1): Directory bread(block 8254) failed
>> [ 50.761235] FAT-fs (mmcblk1p1): Directory bread(block 8255) failed
>> then at next-time, didn't detect sd-card.
>>
>> Did you know this?
>> If you want more information, i will debug, and share the result.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>> On 03/02/2012 12:44 AM, Ulf Hansson wrote:
>>
>>> Make sure mmc_start_req cancel the prepared job, if the request
>>> was prevented to be started due to the card has been removed.
>>>
>>> This bug was introduced in commit:
>>> mmc: allow upper layers to know immediately if card has been removed
>>>
>>> Signed-off-by: Ulf Hansson
>>> ---
>>> drivers/mmc/core/core.c | 35 +++
>>> 1 files changed, 15 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 0b317f0..9e562ab 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -249,16 +249,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
>>> complete(&mrq->completion);
>>> }
>>>
>>> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
>>> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
>>> {
>>> init_completion(&mrq->completion);
>>> mrq->done = mmc_wait_done;
>>> if (mmc_card_removed(host->card)) {
>>> mrq->cmd->error = -ENOMEDIUM;
>>> complete(&mrq->completion);
>>> - return;
>>> + return -ENOMEDIUM;
>>> }
>>> mmc_start_request(host, mrq);
>>> + return 0;
>>> }
>>>
>>> static void mmc_wait_for_req_done(struct mmc_host *host,
>>> @@ -342,6 +343,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host
>>> *host,
>>> struct mmc_async_req *areq, int *error)
>>> {
>>> int err = 0;
>>> + int start_err = 0;
>>> struct mmc_async_req *data = host->areq;
>>>
>>> /* Prepare a new request */
>>> @@ -351,30 +353,23 @@ struct mmc_async_req *mmc_start_req(struct mmc_host
>>> *host,
>>> if (host->areq) {
>>> mmc_wait_for_req_done(host, host->areq->mrq);
>>> err = host->areq->err_check(host->card, host->areq);
>>> - if (err) {
>>> - /* post process the completed failed request */
>>> - mmc_post_req(host, host->areq->mrq, 0);
>>> - if (areq)
>>> - /*
>>> -* Cancel the new prepared request, because
>>> -* it can't run until the failed
>>> -* request has been properly handled.
>>> -*/
>>> - mmc_post_req(host, areq->mrq, -EINVAL);
>>> -
>>> - host->areq = NULL;
>>> - goto out;
>>> - }
>>> }
>>>
>>> - if (areq)
>>> - __mmc_start_req(host, areq->mrq);
>>> + if (!err&& areq)
>>> + start_err = __mmc_start_req(host, areq->mrq);
>>>
>>> if (host->areq)
>>> mmc_post_req(host, host->areq->mrq, 0);
>>>
>>> - host->areq = areq;
>>> - out:
>>> + if (err || start_err) {
>>> + if (areq)
>>> + /* The prepared request was not started, cancel it. */
>>> + mmc_post_req(host, areq->mrq, -EINVAL);
>>> + host->areq = NULL;
There seems to be an issue when setting host->areq=NULL when __mmc_start_req
fails. host->areq == NULL indicates there are no ongoing transfers.
host->areq is used in block.c to check if there are pending requests.
This seem to work:
...
if (err || start_err) {
if (areq)
/* The prepared request was not started, cancel it. */
mmc_post_req(host, areq->mrq, -EINVAL);
}
if (err)
host->areq = NULL;
else
host->areq = areq;
...
This issue will be addressed in version 2. How to resolve it is not decided
yet.
Feel free to comment,
Per
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Clean up after mmc_pre_req if card was removed
On 03/01/2012 04:44 PM, Ulf HANSSON wrote:
> Make sure mmc_start_req cancel the prepared job, if the request
> was prevented to be started due to the card has been removed.
>
> This bug was introduced in commit:
> mmc: allow upper layers to know immediately if card has been removed
>
> Signed-off-by: Ulf Hansson
> ---
> drivers/mmc/core/core.c | 35 +++
> 1 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b317f0..9e562ab 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -249,16 +249,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
> complete(&mrq->completion);
> }
>
> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
> {
> init_completion(&mrq->completion);
> mrq->done = mmc_wait_done;
> if (mmc_card_removed(host->card)) {
> mrq->cmd->error = -ENOMEDIUM;
> complete(&mrq->completion);
> - return;
> + return -ENOMEDIUM;
> }
> mmc_start_request(host, mrq);
> + return 0;
> }
>
> static void mmc_wait_for_req_done(struct mmc_host *host,
> @@ -342,6 +343,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
> struct mmc_async_req *areq, int *error)
> {
> int err = 0;
> + int start_err = 0;
> struct mmc_async_req *data = host->areq;
>
> /* Prepare a new request */
> @@ -351,30 +353,23 @@ struct mmc_async_req *mmc_start_req(struct mmc_host
> *host,
> if (host->areq) {
> mmc_wait_for_req_done(host, host->areq->mrq);
> err = host->areq->err_check(host->card, host->areq);
> - if (err) {
> - /* post process the completed failed request */
> - mmc_post_req(host, host->areq->mrq, 0);
> - if (areq)
> - /*
> - * Cancel the new prepared request, because
> - * it can't run until the failed
> - * request has been properly handled.
> - */
> - mmc_post_req(host, areq->mrq, -EINVAL);
> -
> - host->areq = NULL;
> - goto out;
> - }
> }
>
> - if (areq)
> - __mmc_start_req(host, areq->mrq);
> + if (!err && areq)
> + start_err = __mmc_start_req(host, areq->mrq);
>
> if (host->areq)
> mmc_post_req(host, host->areq->mrq, 0);
>
> - host->areq = areq;
> - out:
> + if (err || start_err) {
> + if (areq)
> + /* The prepared request was not started, cancel it. */
> + mmc_post_req(host, areq->mrq, -EINVAL);
> + host->areq = NULL;
> + } else {
> + host->areq = areq;
> + }
> +
> if (error)
> *error = err;
> return data;
I have reviewed this patch offline. Patch looks good to me.
Reviewed-by: Per Forlin
Br
Per
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: block: release host in case of error
On 11/17/2011 10:18 AM, Adrian Hunter wrote:
> On 14/11/11 13:12, Per Forlin wrote:
>> Host is claimed as long as there are requests in the block queue
>> and all request are completed successfully. If an error occur release
>> the host in case someone else needs to claim it, for instance if the card
>> is removed during a transfer.
>>
>> Signed-off-by: Per Forlin
>> ---
>> drivers/mmc/card/block.c | 37 +
>> 1 files changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index c80bb6d..c21fd2c 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1158,6 +1158,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md,
>> struct mmc_card *card,
>> return ret;
>> }
>>
>> +/*
>> + * This function should be called to resend a request after failure.
>> + * Prepares and starts the request.
>> + */
>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>> + struct mmc_queue *mq,
>> + struct mmc_queue_req *mqrq,
>> + int disable_multi,
>> + struct mmc_async_req *areq)
>> +{
>> +/*
>> + * Release host after failure in case the host is needed
>> + * by someone else. For instance, if the card is removed the
>> + * worker thread needs to claim the host in order to do mmc_rescan.
>> + */
>> +mmc_release_host(card->host);
>> +mmc_claim_host(card->host);
>
> Does this work? Won't the current thread win the race
> to claim the host again?
>
Good question. I've tested it and I haven't seen any cases where current has
claimed the host again. Sujit has tested the patch as well.
But I can't say that your scenario can't happen. I will study the wake_up and
wait_queue code to see if I can find the answer.
Thanks,
Per
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: mmc_test: align max_seg_size
On 11/17/2011 04:56 AM, Sujit Reddy Thumma wrote: > On 11/14/2011 4:34 PM, Per Forlin wrote: >> If max_seg_size is unaligned, mmc_test_map_sg() may create sg element >> sizes that are not aligned with 512 byte. Fix, align max_seg_size at >> mmc_test_area_init(). >> >> Signed-off-by: Per Forlin >> --- >> drivers/mmc/card/mmc_test.c |1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c >> index b038c4a..5848998 100644 >> --- a/drivers/mmc/card/mmc_test.c >> +++ b/drivers/mmc/card/mmc_test.c >> @@ -1581,6 +1581,7 @@ static int mmc_test_area_init(struct mmc_test_card >> *test, int erase, int fill) >> >> t->max_segs = test->card->host->max_segs; >> t->max_seg_sz = test->card->host->max_seg_size; >> +t->max_seg_sz -= t->max_seg_sz % 512; > > Shouldn't we align this to host->max_blk_size supported by controller? It's up to each test case to set the test transfer size. It's not necessary to align max_size but it wouldn't make any harm either. All tests use a multiple of 512 blocks, but this may be changed of course in the future to support large blocks. The max_seg_sz must be aligned otherwise the host driver may get sg-elements that are hard to handle, aligned to 1 byte in the worst case. The test case don't control how the transfer is sg-mapped. /Per -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: slow eMMC write speed
On 09/29/2011 09:24 AM, Linus Walleij wrote: > On Wed, Sep 28, 2011 at 11:34 PM, J Freyensee > wrote: > >> Now in the 3.0 kernel I know mmc_wait_for_req() has changed and the goal was >> to try and make that function a bit more non-blocking, > > What has been done by Per Förlin is to add pre_req/post_req hooks > for the datapath. This will improve data transfers in general if and > only if the driver can do some meaningful work in these hooks, so > your driver needs to be patched to use these. > > Per patched a few select drivers to prepare the DMA buffers > at this time. In our case (mmci.c) dma_map_sg() can be done in > parallel with an ongoing transfer. > > In our case (ux500, mmci, dma40) we don't have bounce buffers > so the only thing that will happen in parallel with ongoing transfers > is L2 and L1 cache flush. *still* we see a noticeable improvement in > throughput, most in L2, but even on the U300 which only does L1 > cache I see some small improvements. > > I *guess* if you're using bounce buffers, the gain will be even > more pronounced. > > (Per, correct me if I'm wrong on any of this...) > Summary: * The mmc block driver runs mmc_blk_rw_rq_prep(), mmc_queue_bounce_post() and __blk_end_request() in parallel with an ongoing mmc transfer. * The driver may use the hooks to schedule low level work such as preparing dma and caches in parallel with ongoing mmc transfer. * The big benefit of this is when using DMA and running the CPU at a lower speed. Here's an example of that: https://wiki.linaro.org/WorkingGroups/Kernel/Specs/StoragePerfMMC-async-req#Block_device_tests_with_governor >> with it too much because my current focus is on existing products and no >> handheld product uses a 3.0 kernel yet (that I am aware of at least). >> However, I still see the fundamental problem is that the MMC stack, which >> was probably written with the intended purpose to be independent of the OS >> block subsystem (struct request and other stuff), really isn't independent >> of the OS block subsystem and will cause holdups between one another, >> thereby dragging down read/write performance of the MMC. > > There are two issues IIRC: > > - The block layer does not provide enough buffers at a time for > the out-of-order buffer pre/post preps to make effect, I think this > was during writes only (Per, can you elaborate?) > Writes are buffered and pushed down many in one go. This mean they can easily be scheduled to be prepared while another is being transferred. Large continues reads are pushed down to MMC synchronously one request per read ahead size. The next large continues read will wait in the block layer and not start until the current one is complete. Read more about the details here: https://wiki.linaro.org/WorkingGroups/Kernel/Specs/StoragePerfMMC-async-req#Analysis_of_how_block_layer_adds_read_request_to_the_mmc_block_queue > - Anything related to card geometries and special sectors and > sector sizes etc, i.e. the stuff that Arnd has analyzed in detail, > also Tixy looked into that for some cards IIRC. > > Each needs to be adressed and is currently "to be done". > >> The other fundamental problem is the writes themselves. Way, WAY more >> writes occur on a handheld system in an end-user's hands than reads. >> Fundamental computer principle states "you make the common case fast". So >> focus should be on how to complete a write operation the fastest way >> possible. > > First case above I think, yep it needs looking into... > The mmc non-blocking patches only tries to move any overhead in parallel with transfer. The actual transfer speed of MMC reads and writes are unaffected. I am hoping that the eMMC v4.5 packed commands support (the ability to group a series of commands in a single data transaction) will help to boost the performance in the future. Regards, Per -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
