On Tue, Nov 12, 2019 at 12:59 AM Arnd Bergmann <arnd@xxxxxxxx> wrote: > > On Mon, Nov 11, 2019 at 1:58 PM Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > > On Mon, 11 Nov 2019 at 17:28, Arnd Bergmann <arnd@xxxxxxxx> wrote: > > > On Mon, Nov 11, 2019 at 8:35 AM Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > > > - Removing all the context switches and workqueues from the data submission > > > path is also the right idea. As you found, there is still a workqueue inside > > > of blk_mq that is used because it may get called from atomic context but > > > the submission may get blocked in __mmc_claim_host(). This really > > > needs to be changed as well, but not in the way I originally suggested: > > > As Hannes suggested, the host interrrupt handler should always use > > > request_threaded_irq() to have its own process context, and then pass a > > > flag to blk_mq to say that we never need another workqueue there. > > > > So you mean we should complete the request in the host driver irq > > thread context, then issue another request in this context by calling > > blk_mq_run_hw_queues()? > > Yes. I assumed there was already code that would always run > blk_mq_run_hw_queue() at I/O completion, but I can't find where > that happens today. OK. Now we will complete a request in block softirq, which means the irq thread of host driver should call blk_mq_complete_request() to complete this request (triggering the block softirq) and call blk_mq_run_hw_queues() to dispatch another request in this context. > > As I understand, the main difference to today is that > __blk_mq_delay_run_hw_queue() can call into __blk_mq_run_hw_queue > directly rather than using the delayed work queue once we > can skip the BLK_MQ_F_BLOCKING check. Right. Need to improve this as you suggested. > > > > - With that change in place calling a blocking __mmc_claim_host() is > > > still a problem, so there should still be a nonblocking mmc_try_claim_host() > > > for the submission path, leading to a BLK_STS_DEV_RESOURCE (?) > > > return code from mmc_mq_queue_rq(). Basically mmc_mq_queue_rq() > > > should always return right away, either after having queued the next I/O > > > or with an error, but not waiting for the device in any way. > > > > Actually not only the mmc_claim_host() will block the MMC request > > processing, in this routine, the mmc_blk_part_switch() and > > mmc_retune() can also block the request processing. Moreover the part > > switching and tuning should be sync operations, and we can not move > > them to a work or a thread. > > Ok, I see. > > Those would also cause requests to be sent to the device or the host > controller, right? Maybe we can treat them as "a non-IO request Right. > has successfully been queued to the device" events, returning > busy from the mmc_mq_queue_rq() function and then running > the queue again when they complete? Yes, seems reasonable to me. > > > > - For the packed requests, there is apparently a very simple way to implement > > > that without a software queue: mmc_mq_queue_rq() is allowed to look at > > > and dequeue all requests that are currently part of the request_queue, > > > so it should take out as many as it wants to submit at once and send > > > them all down to the driver together, avoiding the need for any further > > > round-trips to blk_mq or maintaining a queue in mmc. > > > > You mean we can dispatch a request directly from > > elevator->type->ops.dispatch_request()? but we still need some helper > > functions to check if these requests can be packed (the package > > condition), and need to invent new APIs to start a packed request (or > > using cqe interfaces, which means we still need to implement some cqe > > callbacks). > > I don't know how the dispatch_request() function fits in there, > what Hannes told me is that in ->queue_rq() you can always > look at the following requests that are already queued up > and take the next ones off the list. Looking at bd->last > tells you if there are additional requests. If there are, you can > look at the next one from blk_mq_hw_ctx (not sure how, but > should not be hard to find) > > I also see that there is a commit_rqs() callback that may > go along with queue_rq(), implementing that one could make > this easier as well. Yes, we can use queue_rq()/commit_rqs() and bd->last (now bd->last may can not work well, see [1]), but like we talked before, for packed request, we still need some new interfaces (for example, a interface used to start a packed request, and a interface used to complete a packed request), but at last we got a consensus that we should re-use the CQE interfaces instead of new invention. [1] https://lore.kernel.org/patchwork/patch/1102897/ > > > > - The DMA management (bounce buffer, map, unmap) that is currently > > > done in mmc_blk_mq_issue_rq() should ideally be done in the > > > init_request()/exit_request() (?) callbacks from mmc_mq_ops so this > > > can be done asynchronously, out of the critical timing path for the > > > submission. With this, there won't be any need for a software queue. > > > > This is not true, now the blk-mq will allocate some static request > > objects (usually the static requests number should be the same with > > the hardware queue depth) saved in struct blk_mq_tags. So the > > init_request() is used to initialize the static requests when > > allocating them, and call exit_request to free the static requests > > when freeing the 'struct blk_mq_tags', such as the queue is dead. So > > we can not move the DMA management into the init_request/exit_request. > > Ok, I must have misremembered which callback that is then, but I guess > there is some other place to do it. I checked the 'struct blk_mq_ops', and I did not find a ops can be used to do DMA management. And I also checked UFS driver, it also did the DMA mapping in the queue_rq() (scsi_queue_rq() ---> ufshcd_queuecommand() ---> ufshcd_map_sg()). Maybe I missed something? Moreover like I said above, for the packed request, we still need implement something (like the software queue) based on the CQE interfaces to help to handle packed requests.
