On Thursday, November 15, 2012, Seungwon Jeon <tgih.jun@xxxxxxxxxxx>
> I checked this commit.
> v3 doesn't work in dw_mmc host driver.
> It makes some driver warning while v1 is fine.
> I'm finding the reason. And could you check minor one below?
Please let me correct my test. There is something applied wrong.
Anyway I saw the working fine. Sorry for the confusion.
Thanks,
Seungwon Jeon
>
> On Tuesday, November 13, 2012, Seungwon Jeon <tgih.jun@xxxxxxxxxxx>
> > Hi Konstantin,
> >
> > I'm looking into this patch.
> > Idea looks good as a whole.
> >
> > On Tuesday, November 13, 2012, Konstantin Dorfman <kdorfman@xxxxxxxxxxxxxx> 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%.
> > >
> > > Signed-off-by: Konstantin Dorfman <kdorfman@xxxxxxxxxxxxxx>
> > > ---
> > > v3:
> > > - new MMC_QUEUE_NEW_REQUEST flag to mark new request case
> > > - lock added to update is_new_req flag
> > > - condition for sending new req notification changed
> > > - Moved start waiting for new req notification after
> > > fetching NULL
> > > v2:
> > > - Removed synchronization flags
> > > - removed lock from done()
> > > - flags names changed
> > > v1:
> > > - Initial submit
> > >
> > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > > index 172a768..34d8bd9 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(); /* should never get here */
> > > case MMC_BLK_SUCCESS:
> > > case MMC_BLK_PARTIAL:
> > > /*
> > > @@ -1367,7 +1362,8 @@ 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);
> > Here, we'll try to send previous request which is not completed successfully.
> > And then at the top of do~while, mmc_start_req will be called for current request and get the status.
> > Above change looks like unnecessary. Is there any reason?
> >
> > Thanks,
> > Seungwon Jeon
> >
> > > }
> > > } while (ret);
> > >
> > > @@ -1407,6 +1403,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
> > > goto out;
> > > }
> > >
> > > + mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
> > > +
> > > if (req && req->cmd_flags & REQ_DISCARD) {
> > > /* complete ongoing async transfer before issuing discard */
> > > if (card->host->areq)
> > > @@ -1426,9 +1424,10 @@ 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..ef575ac 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,20 @@ static int mmc_queue_thread(void *d)
> > > set_current_state(TASK_INTERRUPTIBLE);
> > > req = blk_fetch_request(q);
> > > mq->mqrq_cur->req = req;
> > > + if (!req && mq->mqrq_prev->req &&
> > > + !(mq->mqrq_prev->req->cmd_flags & REQ_FLUSH) &&
> > > + !(mq->mqrq_prev->req->cmd_flags & REQ_DISCARD)) {
> > > + mq->context_info.is_waiting_last_req = true;
> > > + }
> > > spin_unlock_irq(q->queue_lock);
> > >
> > > if (req || mq->mqrq_prev->req) {
> > > set_current_state(TASK_RUNNING);
> > > mq->issue_fn(mq, req);
> > > + if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
> > > + mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
> > > + continue; /* fetch again */
> > > + }
> > >
> > > /*
> > > * Current request becomes previous request
> > > @@ -103,6 +111,7 @@ static void mmc_request_fn(struct request_queue *q)
> > > {
> > > struct mmc_queue *mq = q->queuedata;
> > > struct request *req;
> > > + unsigned long flags;
> > >
> > > if (!mq) {
> > > while ((req = blk_fetch_request(q)) != NULL) {
> > > @@ -111,8 +120,19 @@ static void mmc_request_fn(struct request_queue *q)
> > > }
> > > return;
> > > }
> > > -
> > > - if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)
> > > + if (!mq->mqrq_cur->req && mq->mqrq_prev->req) {
> > > + /*
> > > + * New MMC request arrived when MMC thread may be
> > > + * blocked on the previous request to be complete
> > > + * with no current request fetched
> > > + */
> > > + spin_lock_irqsave(&mq->context_info.lock, flags);
> > > + if (mq->context_info.is_waiting_last_req) {
> > > + mq->context_info.is_new_req = true;
> > > + wake_up_interruptible(&mq->context_info.wait);
> > > + }
> > > + spin_unlock_irqrestore(&mq->context_info.lock, flags);
> > > + } else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)
> > > wake_up_process(mq->thread);
> > > }
> > >
> > > @@ -262,6 +282,11 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
> > > }
> > >
> > > sema_init(&mq->thread_sem, 1);
> > > + spin_lock_init(&mq->context_info.lock);
> > > + mq->context_info.is_new_req = false;
> > > + mq->context_info.is_done_rcv = false;
> > > + mq->context_info.is_waiting_last_req = false;
> > > + init_waitqueue_head(&mq->context_info.wait);
> > >
> > > mq->thread = kthread_run(mmc_queue_thread, mq, "mmcqd/%d%s",
> > > host->index, subname ? subname : "");
> > > diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> > > index d2a1eb4..f5885e9 100644
> > > --- a/drivers/mmc/card/queue.h
> > > +++ b/drivers/mmc/card/queue.h
> > > @@ -26,6 +26,8 @@ struct mmc_queue {
> > > struct mmc_card *card;
> > > struct task_struct *thread;
> > > struct semaphore thread_sem;
> > > +#define MMC_QUEUE_SUSPENDED (1 << 0)
> > > +#define MMC_QUEUE_NEW_REQUEST (1 << 1)
> > > unsigned int flags;
> > > int (*issue_fn)(struct mmc_queue *, struct request *);
> > > void *data;
> > > @@ -33,6 +35,7 @@ struct mmc_queue {
> > > struct mmc_queue_req mqrq[2];
> > > struct mmc_queue_req *mqrq_cur;
> > > struct mmc_queue_req *mqrq_prev;
> > > + struct mmc_context_info context_info;
> > > };
> > >
> > > extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *, spinlock_t *,
> > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > index 06c42cf..3374195 100644
> > > --- a/drivers/mmc/core/core.c
> > > +++ b/drivers/mmc/core/core.c
> > > @@ -316,11 +316,43 @@ out:
> > > }
> > > EXPORT_SYMBOL(mmc_start_bkops);
> > >
> > > +/*
> > > + * mmc_wait_data_done() - done callback for data request
> > > + * @mrq: done data request
> > > + *
> > > + * Wakes up mmc context, passed as callback to host controller driver
> > > + */
> > > +static void mmc_wait_data_done(struct mmc_request *mrq)
> > > +{
> > > + mrq->context_info->is_done_rcv = true;
> > > + wake_up_interruptible(&mrq->context_info->wait);
> > > +}
> > > +
> > > static void mmc_wait_done(struct mmc_request *mrq)
> > > {
> > > complete(&mrq->completion);
> > > }
> > >
> > > +/*
> > > + *__mmc_start_data_req() - starts data request
> > > + * @host: MMC host to start the request
> > > + * @mrq: data request to start
> > > + *
> > > + * Fills done callback that will be used when request are done by card.
> > > + * Starts data mmc request execution
> > > + */
> > > +static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq)
> > > +{
> > > + mrq->done = mmc_wait_data_done;
> > > + if (mmc_card_removed(host->card)) {
> > > + mrq->cmd->error = -ENOMEDIUM;
> > > + return -ENOMEDIUM;
> > > + }
> > > + mmc_start_request(host, mrq);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
> > > {
> > > init_completion(&mrq->completion);
> > > @@ -334,6 +366,60 @@ static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * mmc_wait_for_data_req_done() - wait for request completed or new
> > > + * request notification arrives
> > > + * @host: MMC host to prepare the command.
> > > + * @mrq: MMC request to wait for
> > > + *
> > > + * Blocks MMC context till host controller will ack end of data request
> > > + * execution or new request arrives from block layer. Handles
> > > + * command retries.
> > > + *
> > > + * Returns enum mmc_blk_status after checking errors.
> > > + */
> > > +static int mmc_wait_for_data_req_done(struct mmc_host *host,
> > > + struct mmc_request *mrq)
> > > +{
> > > + struct mmc_command *cmd;
> > > + struct mmc_context_info *context_info = mrq->context_info;
> > > + int err;
> > > + unsigned long flags;
> > > +
> > > + while (1) {
> > > + wait_event_interruptible(context_info->wait,
> > > + (context_info->is_done_rcv ||
> > > + context_info->is_new_req));
> > > + spin_lock_irqsave(&context_info->lock, flags);
> > > + context_info->is_waiting_last_req = false;
> > > + spin_unlock_irqrestore(&context_info->lock, flags);
> > > + if (context_info->is_done_rcv) {
> > > + context_info->is_done_rcv = false;
> > > + context_info->is_new_req = false;
> > > + cmd = mrq->cmd;
> > > + if (!cmd->error || !cmd->retries ||
> > > + mmc_card_removed(host->card)) {
> > > + err = host->areq->err_check(host->card,
> > > + host->areq);
> > > + break; /* return err */
> > > + } else {
> > > + pr_info("%s: req failed (CMD%u): %d, retrying...\n",
> > > + mmc_hostname(host),
> > > + cmd->opcode, cmd->error);
> > > + cmd->retries--;
> > > + cmd->error = 0;
> > > + host->ops->request(host, mrq);
> > > + continue; /* wait for done/new event again */
> > > + }
> > > + } else if (context_info->is_new_req) {
> > > + context_info->is_new_req = false;
> > > + err = MMC_BLK_NEW_REQUEST;
> > > + break; /* return err */
> > > + }
> > > + } /* while */
> > > + return err;
> > > +}
> > > +
> > > static void mmc_wait_for_req_done(struct mmc_host *host,
> > > struct mmc_request *mrq)
> > > {
> > > @@ -396,23 +482,17 @@ static void mmc_post_req(struct mmc_host *host, struct mmc_request *mrq,
> > > }
> > >
> > > /**
> > > - * mmc_start_req - start a non-blocking request
> > > - * @host: MMC host to start command
> > > - * @areq: async request to start
> > > - * @error: out parameter returns 0 for success, otherwise non zero
> > > - *
> > > - * Start a new MMC custom command request for a host.
> > > - * If there is on ongoing async request wait for completion
> > > - * of that request and start the new one and return.
> > > - * Does not wait for the new request to complete.
> > > + * mmc_start_req - start a non-blocking data request
> > > + * @host: MMC host to start the command
> > > + * @areq: async request to start
> > > + * @error: out parameter; returns 0 for success, otherwise non zero
> > > *
> > > - * Returns the completed request, NULL in case of none completed.
> > > - * Wait for the an ongoing request (previoulsy started) to complete and
> > > - * return the completed request. If there is no ongoing request, NULL
> > > - * is returned without waiting. NULL is not an error condition.
> > > + * Wait for the ongoing request (previoulsy started) to complete and
> > > + * return the completed request. If there is no ongoing request, NULL
> > > + * is returned without waiting. NULL is not an error condition.
> > > */
> > > struct mmc_async_req *mmc_start_req(struct mmc_host *host,
> > > - struct mmc_async_req *areq, int *error)
> > > + struct mmc_async_req *areq, int *error)
> > > {
> > > int err = 0;
> > > int start_err = 0;
> > > @@ -423,8 +503,20 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
> > > mmc_pre_req(host, areq->mrq, !host->areq);
> > >
> > > if (host->areq) {
> > > - mmc_wait_for_req_done(host, host->areq->mrq);
> > > - err = host->areq->err_check(host->card, host->areq);
> > > + err = mmc_wait_for_data_req_done(host, host->areq->mrq);
> > > + if (err == MMC_BLK_NEW_REQUEST) {
> > > + if (areq) {
> > > + pr_err("%s: new request while areq = %p",
> > > + mmc_hostname(host), areq);
> > > + BUG();
> > > + }
> > > + *error = err;
> Need to check NULL of error.
> if (error)
>
> Thanks,
> Seungwon Jeon
> > > + /*
> > > + * The previous request was not completed,
> > > + * nothing to return
> > > + */
> > > + return NULL;
> > > + }
> > > /*
> > > * Check BKOPS urgency for each R1 response
> > > */
> > > @@ -436,7 +528,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
> > > }
> > >
> > > if (!err && areq)
> > > - start_err = __mmc_start_req(host, areq->mrq);
> > > + start_err = __mmc_start_data_req(host, areq->mrq);
> > >
> > > if (host->areq)
> > > mmc_post_req(host, host->areq->mrq, 0);
> > > @@ -452,6 +544,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
> > >
> > > if (error)
> > > *error = err;
> > > +
> > > return data;
> > > }
> > > EXPORT_SYMBOL(mmc_start_req);
> > > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > > index 943550d..9681d8f 100644
> > > --- a/include/linux/mmc/card.h
> > > +++ b/include/linux/mmc/card.h
> > > @@ -186,6 +186,18 @@ struct sdio_func_tuple;
> > >
> > > #define SDIO_MAX_FUNCS 7
> > >
> > > +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,
> > > + MMC_BLK_NEW_REQUEST,
> > > +};
> > > +
> > > /* The number of MMC physical partitions. These consist of:
> > > * boot partitions (2), general purpose partitions (4) in MMC v4.4.
> > > */
> > > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> > > index 9b9cdaf..aa17096 100644
> > > --- a/include/linux/mmc/core.h
> > > +++ b/include/linux/mmc/core.h
> > > @@ -120,6 +120,21 @@ struct mmc_data {
> > > s32 host_cookie; /* host private data */
> > > };
> > >
> > > +/**
> > > + * mmc_context_info - synchronization details for mmc context
> > > + * @is_done_rcv wake up reason was done request
> > > + * @is_new_req wake up reason was new request
> > > + * @is_waiting_last_req mmc context waiting for single running request
> > > + * @wait wait queue
> > > + */
> > > +struct mmc_context_info {
> > > + bool is_done_rcv;
> > > + bool is_new_req;
> > > + bool is_waiting_last_req;
> > > + wait_queue_head_t wait;
> > > + spinlock_t lock;
> > > +};
> > > +
> > > struct mmc_request {
> > > struct mmc_command *sbc; /* SET_BLOCK_COUNT for multiblock */
> > > struct mmc_command *cmd;
> > > @@ -128,6 +143,7 @@ struct mmc_request {
> > >
> > > struct completion completion;
> > > void (*done)(struct mmc_request *);/* completion function */
> > > + struct mmc_context_info *context_info;
> > > };
> > >
> > > struct mmc_host;
> > > --
> > > 1.7.6
> > > --
> > > Konstantin Dorfman,
> > > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> > > of Code Aurora Forum, hosted by The Linux Foundation
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
