From patchwork Mon Feb 6 19:53:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Omar Sandoval X-Patchwork-Id: 9558691 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D02F56021C for ; Mon, 6 Feb 2017 19:54:05 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C101C1FF12 for ; Mon, 6 Feb 2017 19:54:05 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B59EC205A8; Mon, 6 Feb 2017 19:54:05 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 221531FF12 for ; Mon, 6 Feb 2017 19:54:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751946AbdBFTyE (ORCPT ); Mon, 6 Feb 2017 14:54:04 -0500 Received: from mail-pf0-f174.google.com ([209.85.192.174]:34627 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750997AbdBFTyE (ORCPT ); Mon, 6 Feb 2017 14:54:04 -0500 Received: by mail-pf0-f174.google.com with SMTP id e4so26239832pfg.1 for ; Mon, 06 Feb 2017 11:54:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osandov-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Fycb/popRUqaLwnS80fJr2OH+60JPGy/pjG22W35prM=; b=HJxY+HV+nJFRuxf2adFqrvSZHjbl1+4Y2Wv4va53vEkyk2gw6cOia/g6ah9umeM25w 328b4qxT7+/jWn3FkHk1xfYshlvetPpZdfnPvN2i/ckSZHA8tP13GuXKHfSUZtaUwHt/ JV/mLRiC0KNNcv5AOoyd5VGN1zNUxPDkv7lzoXhYOinqC7B/3jb73/B2N+xL/Cmaaifu YxmPd/2b97B/Ph9DXCM0DUCiacmMiEZO9B/ZUSYeyvzPfq9SOHrB82W7nRIXvKjisdzV QSg1BshaBmzxJRG0F8tsa2m7WQ5ogRFNPJnaGHGZe9GS1aCNeKMXPGbtFzK8F9xcdI8k WYVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Fycb/popRUqaLwnS80fJr2OH+60JPGy/pjG22W35prM=; b=hANWobnpX65sKpo0Rf8JzSG77ViybtM9RUpuZPZ8cdHtvcyD4jD5/wqitXD54NHLJ+ doe2TNAtMB81gJ+FiAhSYz/jXEuAR2AiiMtpUoreYdl/8nrw4j7fFaS3/GZzD5nsrubz hbjGFOq/GVIGmyJas9Xdd9xmqGn2CjxcsJ2pM39C7R9Qp3B9Dqri6Y+dKId99ggahM9z zabXRpFcLWpyb3aZLtsJGHN4W0LHuo4IDZeRufITQNK/exnF+ZogqLRwODx/G6UQo6P5 QXHmAwpcPdn4bBGev+AOk+gcv1HigYPAx34G0PK7KDziLKd0N5sWOMr0Re8BjaqfVHgv AmQA== X-Gm-Message-State: AIkVDXJWgFq6k6YfApcKhXWMEK0F3r+TCZvc9FCBsbOdeO8XJraaVVKMuq3cNYnqIp3HrGkK X-Received: by 10.84.231.143 with SMTP id g15mr20017122plk.1.1486410843364; Mon, 06 Feb 2017 11:54:03 -0800 (PST) Received: from vader ([2620:10d:c090:180::96e7]) by smtp.gmail.com with ESMTPSA id o24sm4636753pfj.78.2017.02.06.11.54.02 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Feb 2017 11:54:02 -0800 (PST) Date: Mon, 6 Feb 2017 11:53:30 -0800 From: Omar Sandoval To: Jens Axboe Cc: linux-block@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH v2] blk-mq-sched: separate mark hctx and queue restart operations Message-ID: <20170206195330.GB20714@vader> References: <0ec9b1167d22966cdf77a1d5140499493ea888a2.1486408944.git.osandov@fb.com> <182de41f-33d3-bbf8-c591-9e577b0f1762@fb.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <182de41f-33d3-bbf8-c591-9e577b0f1762@fb.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Feb 06, 2017 at 12:39:57PM -0700, Jens Axboe wrote: > On 02/06/2017 12:24 PM, Omar Sandoval wrote: > > From: Omar Sandoval > > > > In blk_mq_sched_dispatch_requests(), we call blk_mq_sched_mark_restart() > > after we dispatch requests left over on our hardware queue dispatch > > list. This is so we'll go back and dispatch requests from the scheduler. > > In this case, it's only necessary to restart the hardware queue that we > > are running; there's no reason to run other hardware queues just because > > we are using shared tags. > > > > So, split out blk_mq_sched_mark_restart() into two operations, one for > > just the hardware queue and one for the whole request queue. The core > > code uses both, and I/O schedulers may also want to use them. > > > > Signed-off-by: Omar Sandoval > > --- > > Patch based on block/for-next. > > > > block/blk-mq-sched.c | 2 +- > > block/blk-mq-sched.h | 25 ++++++++++++++++++------- > > block/blk-mq.c | 5 ++++- > > 3 files changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c > > index ee455e7cf9d8..7538565359ea 100644 > > --- a/block/blk-mq-sched.c > > +++ b/block/blk-mq-sched.c > > @@ -201,7 +201,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) > > * needing a restart in that case. > > */ > > if (!list_empty(&rq_list)) { > > - blk_mq_sched_mark_restart(hctx); > > + blk_mq_sched_mark_restart_hctx(hctx); > > blk_mq_dispatch_rq_list(hctx, &rq_list); > > What if we dispatched nothing on this hardware queue, and it currently > doesn't have any IO pending? Hm, so there are two ways that could happen. If it's because ->queue_rq() returned BLK_MQ_RQ_QUEUE_BUSY, then the driver is supposed to kick I/O off again, right? If it's because we failed to get a driver tag, then we'll call blk_mq_sched_mark_restart_queue() in the shared case. I just realized that there's a bug there, though. Since we already set the hctx restart bit, we won't set the queue restart bit. The below should work, and makes more sense in general. Or were you thinking of something else? From c6fb0a75bd3169d09d0da0dd2da82f20f20b8574 Mon Sep 17 00:00:00 2001 Message-Id: From: Omar Sandoval Date: Fri, 3 Feb 2017 11:06:37 -0800 Subject: [PATCH v3] blk-mq-sched: separate mark hctx and queue restart operations In blk_mq_sched_dispatch_requests(), we call blk_mq_sched_mark_restart() after we dispatch requests left over on our hardware queue dispatch list. This is so we'll go back and dispatch requests from the scheduler. In this case, it's only necessary to restart the hardware queue that we are running; there's no reason to run other hardware queues just because we are using shared tags. So, split out blk_mq_sched_mark_restart() into two operations, one for just the hardware queue and one for the whole request queue. The core code uses both, and I/O schedulers may also want to use them. Signed-off-by: Omar Sandoval --- block/blk-mq-sched.c | 2 +- block/blk-mq-sched.h | 26 ++++++++++++++++++-------- block/blk-mq.c | 5 ++++- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index ee455e7cf9d8..7538565359ea 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -201,7 +201,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) * needing a restart in that case. */ if (!list_empty(&rq_list)) { - blk_mq_sched_mark_restart(hctx); + blk_mq_sched_mark_restart_hctx(hctx); blk_mq_dispatch_rq_list(hctx, &rq_list); } else if (!e || !e->type->ops.mq.dispatch_request) { blk_mq_flush_busy_ctxs(hctx, &rq_list); diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h index 5954859c8670..36cc68481b0c 100644 --- a/block/blk-mq-sched.h +++ b/block/blk-mq-sched.h @@ -121,17 +121,27 @@ static inline bool blk_mq_sched_has_work(struct blk_mq_hw_ctx *hctx) return false; } -static inline void blk_mq_sched_mark_restart(struct blk_mq_hw_ctx *hctx) +/* + * Mark a hardware queue as needing a restart. + */ +static inline void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx) { - if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) { + if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state); - if (hctx->flags & BLK_MQ_F_TAG_SHARED) { - struct request_queue *q = hctx->queue; +} + +/* + * Mark a hardware queue and the request queue it belongs to as needing a + * restart. + */ +static inline void blk_mq_sched_mark_restart_queue(struct blk_mq_hw_ctx *hctx) +{ + struct request_queue *q = hctx->queue; - if (!test_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) - set_bit(QUEUE_FLAG_RESTART, &q->queue_flags); - } - } + if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) + set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state); + if (!test_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) + set_bit(QUEUE_FLAG_RESTART, &q->queue_flags); } static inline bool blk_mq_sched_needs_restart(struct blk_mq_hw_ctx *hctx) diff --git a/block/blk-mq.c b/block/blk-mq.c index be183e6115a1..a494c0b589d5 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -937,7 +937,10 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list) * in case the needed IO completed right before we * marked the queue as needing a restart. */ - blk_mq_sched_mark_restart(hctx); + if (hctx->flags & BLK_MQ_F_TAG_SHARED) + blk_mq_sched_mark_restart_queue(hctx); + else + blk_mq_sched_mark_restart_hctx(hctx); if (!blk_mq_get_driver_tag(rq, &hctx, false)) break; }