RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon <tgih.jun@samsung.com>]

Maya Erez wrote:
> On Thu, Nov 10, 2011 Maya Erez wrote:
> > S, Venkatraman <svenkatr@ti.com> wrote:
> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
> wrote:
> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
> >> request *req)
> 
> The function prepares the checkable list and not only checks if packing is
> possible, therefore I think its name should change to reflect its real
> action
I labored at naming. Isn't it proper? :)
Do you have any recommendation?
group_pack_req?

> 
> >> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
> >> >> > +                       !card->ext_csd.packed_event_en)
> >> >> > +               goto no_packed;
> 
> Having the condition with a && can lead to cases where CMD23 is not
> supported and we send packed commands. Therfore the condition should be
> changed to || or be splitted to 2 separate checks.
> Also, according to the standard the generic error flag in
> PACKED_COMMAND_STATUS is set in case of any error and having
> PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
> the packed_event_en should be a mandatory condition for using packed
> coammnds.
... cases where CMD23 is not supported and we send packed commands?
Packed command must not be allowed in such a case.
It works only with predefined mode which is essential fator.
And spec doesn't mentioned PACKED_EVENT_EN must be set.
So Packed command can be sent regardless PACKED_EVENT_EN,
but it's not complete without reporting of error.
Then host driver may suffer error recovery.
Why packed command is used without error reporting?

> 
> >> >> > +       if (mmc_req_rel_wr(cur) &&
> >> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
> >> >> > +                       !en_rel_wr) {
> >> >> > +               goto no_packed;
> >> >> > +       }
> 
> Can you please explain this condition and its purpose?
> 
In the case where reliable write is request but enhanced reliable write
is not supported, write access must be partial according to
reliable write sector count. Because even a single request can be split,
packed command is not allowed in this case.

> >> >> > +               phys_segments +=  next->nr_phys_segments;
> >> >> > +               if (phys_segments > max_phys_segs) {
> >> >> > +                       blk_requeue_request(q, next);
> >> >> > +                       break;
> >> >> > +               }
> >> >> I mentioned this before - if the next request is not packable and
> >> requeued,
> >> >> blk_fetch_request will retrieve it again and this while loop will
> never terminate.
> >> >>
> >> > If next request is not packable, it is requeued and 'break'
> terminates
> >> this loop.
> >> > So it not infinite.
> >> Right !! But that doesn't help finding the commands that are packable.
> Ideally, you'd need to pack all neighbouring requests into one packed
> command.
> >> The way CFQ works, it is not necessary that the fetch would return all
> outstanding
> >> requests that are packable (unless you invoke a forced dispatch) It
> would be good to see some numbers on the number of pack hits /
> misses
> >> that
> >> you would encounter with this logic, on a typical usecase.
> > Is it considered only for CFQ scheduler? How about other I/O scheduler?
> If all requests are drained from scheduler queue forcedly,
> > the number of candidate to be packed can be increased.
> > However we may lose the unique CFQ's strength and MMC D/D may take the
> CFQ's duty.
> > Basically, this patch accommodates the origin order requests from I/O
> scheduler.
> >
> 
> In order to better utilize the packed commands feature and achieve the
> best performance improvements I think that the command packing should be
> done in the block layer, according to the scheduler policy.
> That is, the scheduler should be aware of the capability of the device to
> receive a request list and its constrains (such as maximum number of
> requests, max number of sectors etc) and use this information as a  factor
> to its algorithm.
> This way you keep the decision making in the hands of the scheduler while
> the MMC layer will only have to send this list as a packed command.
> 
Yes, it would be another interesting approach.
Command packing you mentioned means gathering request among same direction(read/write)?
Currently I/O scheduler may know device constrains which MMC driver informs
with the exception of order information for packed command.
But I think the dependency of I/O scheduler may be able to come up.
How can MMC layer treat packed command with I/O scheduler which doesn't support this?

> >> >> > +       if (rqc)
> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> 
> It would be best to keep all the calls to blk_fetch_request in the same
> location. Therefore, I suggest to move the call to mmc_blk_chk_packable to
> mmc/card/queue.c after the first request is fetched.

At the first time, I considered that way.
I'll do more, if possible.
> 
> >> >> >  cmd_abort:
> >> >> > -       spin_lock_irq(&md->lock);
> >> >> > -       while (ret)
> >> >> > -               ret = __blk_end_request(req, -EIO,
> >> blk_rq_cur_bytes(req));
> >> >> > -       spin_unlock_irq(&md->lock);
> >> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> 
> This should be the case for MMC_PACKED_NONE.
Right, I missed it.
> 
> >> >> > +               spin_lock_irq(&md->lock);
> >> >> > +               while (ret)
> >> >> > +                       ret = __blk_end_request(req, -EIO,
> >> blk_rq_cur_bytes(req));
> 
> Do we need the while or should it be an if? In other cases where
> __blk_end_request is called there is no such while.
This part is not only the new but also origin code which is moved in this patch.
Maybe...,'If' case is used  for a whole of remained bytes and
'while' case is used for partial report of remained bytes.

Thank you for review.

Best regards,
Seugwon Jeon.
> 
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html