On Fri, Aug 23, 2024 at 7:28 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> On Sun, Aug 11, 2024 at 06:19:21 PM +0800, Muchun Song wrote:
> > Supposing the following scenario.
> >
> > CPU0 CPU1
> >
> > blk_mq_request_issue_directly() blk_mq_unquiesce_queue()
> > if (blk_queue_quiesced()) blk_queue_flag_clear(QUEUE_FLAG_QUIESCED) 3) store
> > blk_mq_insert_request() blk_mq_run_hw_queues()
> > /* blk_mq_run_hw_queue()
> > * Add request to dispatch list or set bitmap of if (!blk_mq_hctx_has_pending()) 4) load
> > * software queue. 1) store return
> > */
> > blk_mq_run_hw_queue()
> > if (blk_queue_quiesced()) 2) load
> > return
> > blk_mq_sched_dispatch_requests()
> >
> > The full memory barrier should be inserted between 1) and 2), as well as
> > between 3) and 4) to make sure that either CPU0 sees QUEUE_FLAG_QUIESCED is
> > cleared or CPU1 sees dispatch list or setting of bitmap of software queue.
> > Otherwise, either CPU will not re-run the hardware queue causing starvation.
>
> Memory barrier shouldn't serve as bug fix for two slow code paths.
>
> One simple fix is to add helper of blk_queue_quiesced_lock(), and
> call the following check on CPU0:
>
> if (blk_queue_quiesced_lock())
> blk_mq_run_hw_queue();
This only fixes blk_mq_request_issue_directly(), I think anywhere that
matching this
pattern (inserting a request to dispatch list and then running the
hardware queue)
should be fixed. And I think there are many places which match this
pattern (E.g.
blk_mq_submit_bio()). The above graph should be adjusted to the following.
CPU0 CPU1
blk_mq_insert_request() 1) store blk_mq_unquiesce_queue()
blk_mq_run_hw_queue()
blk_queue_flag_clear(QUEUE_FLAG_QUIESCED) 3) store
if (blk_queue_quiesced()) 2) load blk_mq_run_hw_queues()
return blk_mq_run_hw_queue()
blk_mq_sched_dispatch_requests() if
(!blk_mq_hctx_has_pending()) 4) load
return
So I think fixing blk_mq_run_hw_queue() could cover all of the situations.
Maybe I thought wrongly. Please correct me.
Muchun,
Thanks.
