diff for duplicates of <1492022286.2764.15.camel@sandisk.com> diff --git a/a/1.txt b/N1/1.txt index 6669244..900d670 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -2,66 +2,87 @@ On Wed, 2017-04-12 at 11:42 +0800, Ming Lei wrote: > On Tue, Apr 11, 2017 at 06:18:36PM +0000, Bart Van Assche wrote: > > On Tue, 2017-04-11 at 14:03 -0400, Mike Snitzer wrote: > > > Rather than working so hard to use DM code against me, your argument -> > > should be: "blk-mq drivers X, Y and Z rerun the hw queue; this is a well +> > > should be: "blk-mq drivers X, Y and Z rerun the hw queue; this is a w= +ell > > > established pattern" -> > > -> > > I see drivers/nvme/host/fc.c:nvme_fc_start_fcp_op() does. But that is +> > >=20 +> > > I see drivers/nvme/host/fc.c:nvme_fc_start_fcp_op() does. But that i= +s > > > only one other driver out of ~20 BLK_MQ_RQ_QUEUE_BUSY returns > > > tree-wide. -> > > -> > > Could be there are some others, but hardly a well-established pattern. -> > +> > >=20 +> > > Could be there are some others, but hardly a well-established pattern= +. +> >=20 > > Hello Mike, -> > +> >=20 > > Several blk-mq drivers that can return BLK_MQ_RQ_QUEUE_BUSY from their -> > .queue_rq() implementation stop the request queue (blk_mq_stop_hw_queue()) -> > before returning "busy" and restart the queue after the busy condition has -> > been cleared (blk_mq_start_stopped_hw_queues()). Examples are virtio_blk and -> > xen-blkfront. However, this approach is not appropriate for the dm-mq core -> > nor for the scsi core since both drivers already use the "stopped" state for -> > another purpose than tracking whether or not a hardware queue is busy. Hence -> > the blk_mq_delay_run_hw_queue() and blk_mq_run_hw_queue() calls in these last -> > two drivers to rerun a hardware queue after the busy state has been cleared. -> +> > .queue_rq() implementation stop the request queue=A0(blk_mq_stop_hw_que= +ue()) +> > before returning "busy" and restart the queue after the busy condition = +has +> > been cleared (blk_mq_start_stopped_hw_queues()). Examples are virtio_bl= +k and +> > xen-blkfront. However, this approach is not appropriate for the dm-mq c= +ore +> > nor for the scsi core since both drivers already use the "stopped" stat= +e for +> > another purpose than tracking whether or not a hardware queue is busy. = +Hence +> > the blk_mq_delay_run_hw_queue() and blk_mq_run_hw_queue() calls in thes= +e last +> > two drivers to rerun a hardware queue after the busy state has been cle= +ared. +>=20 > But looks this patch just reruns the hw queue after 100ms, which isn't > that after the busy state has been cleared, right? Hello Ming, -That patch can be considered as a first step that can be refined further, namely -by modifying the dm-rq code further such that dm-rq queues are only rerun after -the busy condition has been cleared. The patch at the start of this thread is -easier to review and easier to test than any patch that would only rerun dm-rq +That patch can be considered as a first step that can be refined further, n= +amely +by modifying the dm-rq code further such that dm-rq queues are only rerun a= +fter +the busy condition has been cleared. The patch at the start of this thread = +is +easier to review and easier to test than any patch that would only rerun dm= +-rq queues after the busy condition has been cleared. > Actually if BLK_MQ_RQ_QUEUE_BUSY is returned from .queue_rq(), blk-mq > will buffer this request into hctx->dispatch and run the hw queue again, -> so looks blk_mq_delay_run_hw_queue() in this situation shouldn't have been +> so looks blk_mq_delay_run_hw_queue() in this situation shouldn't have bee= +n > needed at my 1st impression. If the blk-mq core would always rerun a hardware queue if a block driver -returns BLK_MQ_RQ_QUEUE_BUSY then that would cause 100% of a single CPU core -to be busy with polling a hardware queue until the "busy" condition has been +returns BLK_MQ_RQ_QUEUE_BUSY then that would cause 100% of a single CPU cor= +e +to be busy with polling a hardware queue until the "busy" condition has bee= +n cleared. One can see easily that that's not what the blk-mq core does. From blk_mq_sched_dispatch_requests(): if (!list_empty(&rq_list)) { blk_mq_sched_mark_restart_hctx(hctx); - did_work = blk_mq_dispatch_rq_list(q, &rq_list); + did_work =3D blk_mq_dispatch_rq_list(q, &rq_list); } -From the end of blk_mq_dispatch_rq_list(): +>From the end of blk_mq_dispatch_rq_list(): if (!list_empty(list)) { [ ... ] if (!blk_mq_sched_needs_restart(hctx) && - !test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state)) + =A0=A0=A0=A0!test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state)) blk_mq_run_hw_queue(hctx, true); } -In other words, the BLK_MQ_S_SCHED_RESTART flag is set before the dispatch list -is examined and only if that flag gets cleared while blk_mq_dispatch_rq_list() +In other words, the BLK_MQ_S_SCHED_RESTART flag is set before the dispatch = +list +is examined and only if that flag gets cleared while blk_mq_dispatch_rq_lis= +t() is in progress by a concurrent blk_mq_sched_restart_hctx() call then the -dispatch list will be rerun after a block driver returned BLK_MQ_RQ_QUEUE_BUSY. +dispatch list will be rerun after a block driver returned=A0BLK_MQ_RQ_QUEUE= +_BUSY. -Bart. +Bart.= diff --git a/a/content_digest b/N1/content_digest index 104830e..3c9a5ea 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -22,68 +22,89 @@ "> On Tue, Apr 11, 2017 at 06:18:36PM +0000, Bart Van Assche wrote:\n" "> > On Tue, 2017-04-11 at 14:03 -0400, Mike Snitzer wrote:\n" "> > > Rather than working so hard to use DM code against me, your argument\n" - "> > > should be: \"blk-mq drivers X, Y and Z rerun the hw queue; this is a well\n" + "> > > should be: \"blk-mq drivers X, Y and Z rerun the hw queue; this is a w=\n" + "ell\n" "> > > established pattern\"\n" - "> > > \n" - "> > > I see drivers/nvme/host/fc.c:nvme_fc_start_fcp_op() does. But that is\n" + "> > >=20\n" + "> > > I see drivers/nvme/host/fc.c:nvme_fc_start_fcp_op() does. But that i=\n" + "s\n" "> > > only one other driver out of ~20 BLK_MQ_RQ_QUEUE_BUSY returns\n" "> > > tree-wide.\n" - "> > > \n" - "> > > Could be there are some others, but hardly a well-established pattern.\n" - "> > \n" + "> > >=20\n" + "> > > Could be there are some others, but hardly a well-established pattern=\n" + ".\n" + "> >=20\n" "> > Hello Mike,\n" - "> > \n" + "> >=20\n" "> > Several blk-mq drivers that can return BLK_MQ_RQ_QUEUE_BUSY from their\n" - "> > .queue_rq() implementation stop the request queue\302\240(blk_mq_stop_hw_queue())\n" - "> > before returning \"busy\" and restart the queue after the busy condition has\n" - "> > been cleared (blk_mq_start_stopped_hw_queues()). Examples are virtio_blk and\n" - "> > xen-blkfront. However, this approach is not appropriate for the dm-mq core\n" - "> > nor for the scsi core since both drivers already use the \"stopped\" state for\n" - "> > another purpose than tracking whether or not a hardware queue is busy. Hence\n" - "> > the blk_mq_delay_run_hw_queue() and blk_mq_run_hw_queue() calls in these last\n" - "> > two drivers to rerun a hardware queue after the busy state has been cleared.\n" - "> \n" + "> > .queue_rq() implementation stop the request queue=A0(blk_mq_stop_hw_que=\n" + "ue())\n" + "> > before returning \"busy\" and restart the queue after the busy condition =\n" + "has\n" + "> > been cleared (blk_mq_start_stopped_hw_queues()). Examples are virtio_bl=\n" + "k and\n" + "> > xen-blkfront. However, this approach is not appropriate for the dm-mq c=\n" + "ore\n" + "> > nor for the scsi core since both drivers already use the \"stopped\" stat=\n" + "e for\n" + "> > another purpose than tracking whether or not a hardware queue is busy. =\n" + "Hence\n" + "> > the blk_mq_delay_run_hw_queue() and blk_mq_run_hw_queue() calls in thes=\n" + "e last\n" + "> > two drivers to rerun a hardware queue after the busy state has been cle=\n" + "ared.\n" + ">=20\n" "> But looks this patch just reruns the hw queue after 100ms, which isn't\n" "> that after the busy state has been cleared, right?\n" "\n" "Hello Ming,\n" "\n" - "That patch can be considered as a first step that can be refined further, namely\n" - "by modifying the dm-rq code further such that dm-rq queues are only rerun after\n" - "the busy condition has been cleared. The patch at the start of this thread is\n" - "easier to review and easier to test than any patch that would only rerun dm-rq\n" + "That patch can be considered as a first step that can be refined further, n=\n" + "amely\n" + "by modifying the dm-rq code further such that dm-rq queues are only rerun a=\n" + "fter\n" + "the busy condition has been cleared. The patch at the start of this thread =\n" + "is\n" + "easier to review and easier to test than any patch that would only rerun dm=\n" + "-rq\n" "queues after the busy condition has been cleared.\n" "\n" "> Actually if BLK_MQ_RQ_QUEUE_BUSY is returned from .queue_rq(), blk-mq\n" "> will buffer this request into hctx->dispatch and run the hw queue again,\n" - "> so looks blk_mq_delay_run_hw_queue() in this situation shouldn't have been\n" + "> so looks blk_mq_delay_run_hw_queue() in this situation shouldn't have bee=\n" + "n\n" "> needed at my 1st impression.\n" "\n" "If the blk-mq core would always rerun a hardware queue if a block driver\n" - "returns BLK_MQ_RQ_QUEUE_BUSY then that would cause 100% of a single CPU core\n" - "to be busy with polling a hardware queue until the \"busy\" condition has been\n" + "returns BLK_MQ_RQ_QUEUE_BUSY then that would cause 100% of a single CPU cor=\n" + "e\n" + "to be busy with polling a hardware queue until the \"busy\" condition has bee=\n" + "n\n" "cleared. One can see easily that that's not what the blk-mq core does. From\n" "blk_mq_sched_dispatch_requests():\n" "\n" "\tif (!list_empty(&rq_list)) {\n" "\t\tblk_mq_sched_mark_restart_hctx(hctx);\n" - "\t\tdid_work = blk_mq_dispatch_rq_list(q, &rq_list);\n" + "\t\tdid_work =3D blk_mq_dispatch_rq_list(q, &rq_list);\n" "\t}\n" "\n" - "From the end of blk_mq_dispatch_rq_list():\n" + ">From the end of blk_mq_dispatch_rq_list():\n" "\n" "\tif (!list_empty(list)) {\n" "\t\t[ ... ]\n" "\t\tif (!blk_mq_sched_needs_restart(hctx) &&\n" - "\t\t\302\240\302\240\302\240\302\240!test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state))\n" + "\t\t=A0=A0=A0=A0!test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state))\n" "\t\t\tblk_mq_run_hw_queue(hctx, true);\n" "\t}\n" "\n" - "In other words, the BLK_MQ_S_SCHED_RESTART flag is set before the dispatch list\n" - "is examined and only if that flag gets cleared while blk_mq_dispatch_rq_list()\n" + "In other words, the BLK_MQ_S_SCHED_RESTART flag is set before the dispatch =\n" + "list\n" + "is examined and only if that flag gets cleared while blk_mq_dispatch_rq_lis=\n" + "t()\n" "is in progress by a concurrent blk_mq_sched_restart_hctx() call then the\n" - "dispatch list will be rerun after a block driver returned\302\240BLK_MQ_RQ_QUEUE_BUSY.\n" + "dispatch list will be rerun after a block driver returned=A0BLK_MQ_RQ_QUEUE=\n" + "_BUSY.\n" "\n" - Bart. + Bart.= -1d16cbecf051ecd53471635a20cbf4591399bc4c1c747e92b15c0327a68ce241 +ec6b0a7e94adb88a0e242fea65ca574eba2ddbe6acd767020fd69362c9ac73bf
diff --git a/a/1.txt b/N2/1.txt index 6669244..db14a1d 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -50,7 +50,7 @@ blk_mq_sched_dispatch_requests(): did_work = blk_mq_dispatch_rq_list(q, &rq_list); } -From the end of blk_mq_dispatch_rq_list(): +>From the end of blk_mq_dispatch_rq_list(): if (!list_empty(list)) { [ ... ] diff --git a/a/content_digest b/N2/content_digest index 104830e..9a28c17 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -70,7 +70,7 @@ "\t\tdid_work = blk_mq_dispatch_rq_list(q, &rq_list);\n" "\t}\n" "\n" - "From the end of blk_mq_dispatch_rq_list():\n" + ">From the end of blk_mq_dispatch_rq_list():\n" "\n" "\tif (!list_empty(list)) {\n" "\t\t[ ... ]\n" @@ -86,4 +86,4 @@ "\n" Bart. -1d16cbecf051ecd53471635a20cbf4591399bc4c1c747e92b15c0327a68ce241 +e875a92d802d793a16833864b03d4743e5f4878379c8a109e66484139385de3a
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.