Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751925AbdLNGKh (ORCPT ); Thu, 14 Dec 2017 01:10:37 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:41160 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751879AbdLNGKe (ORCPT ); Thu, 14 Dec 2017 01:10:34 -0500 X-Google-Smtp-Source: ACJfBouKr0Exy15DKFPp9jIHJRFNO540NW9e/UwggRk7j9mLn90FRY+ejeZBahpkgqcWgGq5pmJLOg== Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: Re: [PATCH V2] block, bfq: remove batches of confusing ifdefs From: Paolo Valente In-Reply-To: <20171204104205.3290-1-paolo.valente@linaro.org> Date: Thu, 14 Dec 2017 07:10:30 +0100 Cc: linux-block , Linux Kernel Mailing List , Ulf Hansson , broonie@kernel.org, linus.walleij@linaro.org, lucmiccio@gmail.com Message-Id: <2C37D091-F207-4E9E-B8A8-06BDCDF47A28@linaro.org> References: <20171204104205.3290-1-paolo.valente@linaro.org> To: Jens Axboe X-Mailer: Apple Mail (2.3445.5.20) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id vBE6ArAh032651 Content-Length: 7735 Lines: 228 Hi Jens, do you think this version could be ok? Thanks, Paolo > Il giorno 04 dic 2017, alle ore 11:42, Paolo Valente ha scritto: > > Commit a33801e8b473 ("block, bfq: move debug blkio stats behind > CONFIG_DEBUG_BLK_CGROUP") introduced two batches of confusing ifdefs: > one reported in [1], plus a similar one in another function. This > commit removes both batches, in the way suggested in [1]. > > [1] https://www.spinics.net/lists/linux-block/msg20043.html > > Fixes: a33801e8b473 ("block, bfq: move debug blkio stats behind CONFIG_DEBUG_BLK_CGROUP") > Reported-by: Linus Torvalds > Tested-by: Luca Miccio > Signed-off-by: Paolo Valente > --- > block/bfq-iosched.c | 127 +++++++++++++++++++++++++++++----------------------- > 1 file changed, 72 insertions(+), 55 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index bcb6d21..3feed64 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -3689,35 +3689,16 @@ static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) > return rq; > } > > -static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) > -{ > - struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; > - struct request *rq; > #if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > - struct bfq_queue *in_serv_queue, *bfqq; > - bool waiting_rq, idle_timer_disabled; > -#endif > - > - spin_lock_irq(&bfqd->lock); > - > -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > - in_serv_queue = bfqd->in_service_queue; > - waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue); > - > - rq = __bfq_dispatch_request(hctx); > - > - idle_timer_disabled = > - waiting_rq && !bfq_bfqq_wait_request(in_serv_queue); > - > -#else > - rq = __bfq_dispatch_request(hctx); > -#endif > - spin_unlock_irq(&bfqd->lock); > +static void bfq_update_dispatch_stats(struct request_queue *q, > + struct request *rq, > + struct bfq_queue *in_serv_queue, > + bool idle_timer_disabled) > +{ > + struct bfq_queue *bfqq = rq ? RQ_BFQQ(rq) : NULL; > > -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > - bfqq = rq ? RQ_BFQQ(rq) : NULL; > if (!idle_timer_disabled && !bfqq) > - return rq; > + return; > > /* > * rq and bfqq are guaranteed to exist until this function > @@ -3732,7 +3713,7 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) > * In addition, the following queue lock guarantees that > * bfqq_group(bfqq) exists as well. > */ > - spin_lock_irq(hctx->queue->queue_lock); > + spin_lock_irq(q->queue_lock); > if (idle_timer_disabled) > /* > * Since the idle timer has been disabled, > @@ -3751,9 +3732,37 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) > bfqg_stats_set_start_empty_time(bfqg); > bfqg_stats_update_io_remove(bfqg, rq->cmd_flags); > } > - spin_unlock_irq(hctx->queue->queue_lock); > + spin_unlock_irq(q->queue_lock); > +} > +#else > +static inline void bfq_update_dispatch_stats(struct request_queue *q, > + struct request *rq, > + struct bfq_queue *in_serv_queue, > + bool idle_timer_disabled) {} > #endif > > +static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) > +{ > + struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; > + struct request *rq; > + struct bfq_queue *in_serv_queue; > + bool waiting_rq, idle_timer_disabled; > + > + spin_lock_irq(&bfqd->lock); > + > + in_serv_queue = bfqd->in_service_queue; > + waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue); > + > + rq = __bfq_dispatch_request(hctx); > + > + idle_timer_disabled = > + waiting_rq && !bfq_bfqq_wait_request(in_serv_queue); > + > + spin_unlock_irq(&bfqd->lock); > + > + bfq_update_dispatch_stats(hctx->queue, rq, in_serv_queue, > + idle_timer_disabled); > + > return rq; > } > > @@ -4276,16 +4285,46 @@ static bool __bfq_insert_request(struct bfq_data *bfqd, struct request *rq) > return idle_timer_disabled; > } > > +#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > +static void bfq_update_insert_stats(struct request_queue *q, > + struct bfq_queue *bfqq, > + bool idle_timer_disabled, > + unsigned int cmd_flags) > +{ > + if (!bfqq) > + return; > + > + /* > + * bfqq still exists, because it can disappear only after > + * either it is merged with another queue, or the process it > + * is associated with exits. But both actions must be taken by > + * the same process currently executing this flow of > + * instructions. > + * > + * In addition, the following queue lock guarantees that > + * bfqq_group(bfqq) exists as well. > + */ > + spin_lock_irq(q->queue_lock); > + bfqg_stats_update_io_add(bfqq_group(bfqq), bfqq, cmd_flags); > + if (idle_timer_disabled) > + bfqg_stats_update_idle_time(bfqq_group(bfqq)); > + spin_unlock_irq(q->queue_lock); > +} > +#else > +static inline void bfq_update_insert_stats(struct request_queue *q, > + struct bfq_queue *bfqq, > + bool idle_timer_disabled, > + unsigned int cmd_flags) {} > +#endif > + > static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, > bool at_head) > { > struct request_queue *q = hctx->queue; > struct bfq_data *bfqd = q->elevator->elevator_data; > -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > struct bfq_queue *bfqq = RQ_BFQQ(rq); > bool idle_timer_disabled = false; > unsigned int cmd_flags; > -#endif > > spin_lock_irq(&bfqd->lock); > if (blk_mq_sched_try_insert_merge(q, rq)) { > @@ -4304,7 +4343,6 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, > else > list_add_tail(&rq->queuelist, &bfqd->dispatch); > } else { > -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > idle_timer_disabled = __bfq_insert_request(bfqd, rq); > /* > * Update bfqq, because, if a queue merge has occurred > @@ -4312,9 +4350,6 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, > * redirected into a new queue. > */ > bfqq = RQ_BFQQ(rq); > -#else > - __bfq_insert_request(bfqd, rq); > -#endif > > if (rq_mergeable(rq)) { > elv_rqhash_add(q, rq); > @@ -4323,35 +4358,17 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, > } > } > > -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > /* > * Cache cmd_flags before releasing scheduler lock, because rq > * may disappear afterwards (for example, because of a request > * merge). > */ > cmd_flags = rq->cmd_flags; > -#endif > + > spin_unlock_irq(&bfqd->lock); > > -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP) > - if (!bfqq) > - return; > - /* > - * bfqq still exists, because it can disappear only after > - * either it is merged with another queue, or the process it > - * is associated with exits. But both actions must be taken by > - * the same process currently executing this flow of > - * instruction. > - * > - * In addition, the following queue lock guarantees that > - * bfqq_group(bfqq) exists as well. > - */ > - spin_lock_irq(q->queue_lock); > - bfqg_stats_update_io_add(bfqq_group(bfqq), bfqq, cmd_flags); > - if (idle_timer_disabled) > - bfqg_stats_update_idle_time(bfqq_group(bfqq)); > - spin_unlock_irq(q->queue_lock); > -#endif > + bfq_update_insert_stats(q, bfqq, idle_timer_disabled, > + cmd_flags); > } > > static void bfq_insert_requests(struct blk_mq_hw_ctx *hctx, > -- > 2.10.0 >