On Tue, Mar 31, 2020 at 04:51:00PM -0700, Doug Anderson wrote: > Hi, > > On Tue, Mar 31, 2020 at 11:26 AM Jens Axboe <axboe@xxxxxxxxx> wrote: > > > > On 3/31/20 12:07 PM, Paolo Valente wrote: > > >> Il giorno 31 mar 2020, alle ore 03:41, Ming Lei <ming.lei@xxxxxxxxxx> ha scritto: > > >> > > >> On Mon, Mar 30, 2020 at 07:49:06AM -0700, Douglas Anderson wrote: > > >>> It is possible for two threads to be running > > >>> blk_mq_do_dispatch_sched() at the same time with the same "hctx". > > >>> This is because there can be more than one caller to > > >>> __blk_mq_run_hw_queue() with the same "hctx" and hctx_lock() doesn't > > >>> prevent more than one thread from entering. > > >>> > > >>> If more than one thread is running blk_mq_do_dispatch_sched() at the > > >>> same time with the same "hctx", they may have contention acquiring > > >>> budget. The blk_mq_get_dispatch_budget() can eventually translate > > >>> into scsi_mq_get_budget(). If the device's "queue_depth" is 1 (not > > >>> uncommon) then only one of the two threads will be the one to > > >>> increment "device_busy" to 1 and get the budget. > > >>> > > >>> The losing thread will break out of blk_mq_do_dispatch_sched() and > > >>> will stop dispatching requests. The assumption is that when more > > >>> budget is available later (when existing transactions finish) the > > >>> queue will be kicked again, perhaps in scsi_end_request(). > > >>> > > >>> The winning thread now has budget and can go on to call > > >>> dispatch_request(). If dispatch_request() returns NULL here then we > > >>> have a potential problem. Specifically we'll now call > > >> > > >> I guess this problem should be BFQ specific. Now there is definitely > > >> requests in BFQ queue wrt. this hctx. However, looks this request is > > >> only available from another loser thread, and it won't be retrieved in > > >> the winning thread via e->type->ops.dispatch_request(). > > >> > > >> Just wondering why BFQ is implemented in this way? > > >> > > > > > > BFQ inherited this powerful non-working scheme from CFQ, some age ago. > > > > > > In more detail: if BFQ has at least one non-empty internal queue, then > > > is says of course that there is work to do. But if the currently > > > in-service queue is empty, and is expected to receive new I/O, then > > > BFQ plugs I/O dispatch to enforce service guarantees for the > > > in-service queue, i.e., BFQ responds NULL to a dispatch request. > > > > What BFQ is doing is fine, IFF it always ensures that the queue is run > > at some later time, if it returns "yep I have work" yet returns NULL > > when attempting to retrieve that work. Generally this should happen from > > subsequent IO completion, or whatever else condition will resolve the > > issue that is currently preventing dispatch of that request. Last resort > > would be a timer, but that can happen if you're slicing your scheduling > > somehow. > > I've been poking more at this today trying to understand why the idle > timer that Paolo says is in BFQ isn't doing what it should be doing. > I've been continuing to put most of my stream-of-consciousness at > <https://crbug.com/1061950> to avoid so much spamming of this thread. > In the trace I looked at most recently it looks like BFQ does try to > ensure that the queue is run at a later time, but at least in this > trace the later time is not late enough. Specifically the quick > summary of my recent trace: > > 28977309us - PID 2167 got the budget. > 28977518us - BFQ told PID 2167 that there was nothing to dispatch. > 28977702us - BFQ idle timer fires. > 28977725us - We start to try to dispatch as a result of BFQ's idle timer. > 28977732us - Dispatching that was a result of BFQ's idle timer can't get > budget and thus does nothing. Looks the BFQ idle timer may be re-tried given it knows there is work to do. > 28977780us - PID 2167 put the budget and exits since there was nothing > to dispatch. > > This is only one particular trace, but in this case BFQ did attempt to > rerun the queue after it returned NULL, but that ran almost > immediately after it returned NULL and thus ran into the race. :( > > > > > It would be very easy to change bfq_has_work so that it returns false > > > in case the in-service queue is empty, even if there is I/O > > > backlogged. My only concern is: since everything has worked with the > > > current scheme for probably 15 years, are we sure that everything is > > > still ok after we change this scheme? > > > > You're comparing apples to oranges, CFQ never worked within the blk-mq > > scheduling framework. > > > > That said, I don't think such a change is needed. If we currently have a > > hang due to this discrepancy between has_work and gets_work, then it > > sounds like we're not always re-running the queue as we should. From the > > original patch, the budget putting is not something the scheduler is > > involved with. Do we just need to ensure that if we put budget without > > having dispatched a request, we need to kick off dispatching again? > > By this you mean a change like this in blk_mq_do_dispatch_sched()? > > if (!rq) { > blk_mq_put_dispatch_budget(hctx); > + ret = true; > break; > } >From Jens's tree, blk_mq_do_dispatch_sched() returns nothing. Which tree are you talking against? Thanks, Ming
