From patchwork Fri Nov 17 19:29:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 10063493 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 ADE376037E for ; Fri, 17 Nov 2017 19:29:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 995DE2A53E for ; Fri, 17 Nov 2017 19:29:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8B1D82A5B2; Fri, 17 Nov 2017 19:29:44 +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.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID 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 C02552A53E for ; Fri, 17 Nov 2017 19:29:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933838AbdKQT3n (ORCPT ); Fri, 17 Nov 2017 14:29:43 -0500 Received: from mail-it0-f49.google.com ([209.85.214.49]:38951 "EHLO mail-it0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750895AbdKQT3l (ORCPT ); Fri, 17 Nov 2017 14:29:41 -0500 Received: by mail-it0-f49.google.com with SMTP id y15so5310471ita.4 for ; Fri, 17 Nov 2017 11:29:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=R0bOyTzfkfDB5AjiEsFcGolSkpdgvd+/rp4PeeuwNdM=; b=krDvhF3c9RwMV3GaZYzGYKEVGAFIrZbtcUw2M8DO9QsrodDryvv+CQBLcqsGXz2rpA icyfpmlZyv0oQ0J3qj9HYumERWtuUIJO+elmwW/mQx/Tqxny6PpOStamyVYlyLFYvyYu sb5LzJ4j6l15Hoe+8+ez5n54KkOPAUHdbHSsn6Qj+dT2IccmzcTvE6y7fqGK5BGttH1o 1d+KJ7ILKXNljtd5H5ebjDXCKnqmeHzdHyAeiFUM4in40qGmdaq0yvefz7LdfJQRs2Ep o3KxH/xl2YdLD0pgyiVpHPNSnRCsQGCx1mg3ddDZCK2zZklTTvd8ZRqePoUU/xuEOpLz vmtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=R0bOyTzfkfDB5AjiEsFcGolSkpdgvd+/rp4PeeuwNdM=; b=m4lpZagVj6zXQKf9ws672EjQPoJ9sDRSh7R30iNRPGUjqd49m+hzvWhxZ591RLa3dJ hA49dg6D1w4e+PaiOzo5mCrVrfrNWe9SgPD4A8ZlKqmC6kkmhOFmbKQIgGcA4vo5C6So 0UM4T1wF4NC0uC/RisIGaoFDHP7QG5iZlU6ey/LFh5453XFIDXyqEVCK9Qb4t/PDkVBx A17luG1+NoSmSn/vz7gUEqOmvTwvbGYe/HE7I97HfGKYGbxnZoiasqCvfx/jiC63klZV 3fdZUQTCHKOusF8cKJ0qUhl2Qu8+wQcqK/q/t6QLgPjbPz98ivcLoGOeRA5IMptnjkbp trOw== X-Gm-Message-State: AJaThX4NavJvZx/2Dtsi11KNLg2KzNTpxDcpzKSKxAU+0Op/liW42xph 9h0AQ4olXNiP2FOjTy/eFJWDQh7IWS7aKw9fshU= X-Google-Smtp-Source: AGs4zMZiwqbvWXMVWQ4zEEJk0MiW8j1sVj/KoyNOQq+nAxJv9/uT4SF1p7lr4NbWt3zxwpwmW3eqRhP36/Vjq2AiomU= X-Received: by 10.36.93.84 with SMTP id w81mr8139275ita.139.1510946980949; Fri, 17 Nov 2017 11:29:40 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.88.8 with HTTP; Fri, 17 Nov 2017 11:29:40 -0800 (PST) In-Reply-To: <82ee3a59-1767-3073-7893-bf78b5933c5e@kernel.dk> References: <82ee3a59-1767-3073-7893-bf78b5933c5e@kernel.dk> From: Linus Torvalds Date: Fri, 17 Nov 2017 11:29:40 -0800 X-Google-Sender-Auth: EGNxA4qI3VlYuG7Wc8iCVoeiBoM Message-ID: Subject: Re: [GIT PULL] Followup merge window block fixes/changes To: Jens Axboe , Luca Miccio Cc: "linux-block@vger.kernel.org" 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 Fri, Nov 17, 2017 at 8:51 AM, Jens Axboe wrote: > > - Small BFQ updates series from Luca and Paolo. Honestly, this code is too ugly to live. Seriously. That update should have been rejected on the grounds of being completely unmaintainable crap. Why didn't you? Why are you allowing code like that patch to bfq_dispatch_request() into the kernel source tree? Yes, it improves performance. But it does so by adding random #iofdef's into the middle of some pretty crtitical code, with absolutely no attempt at having a sane maintainable code-base. That function is literally *three* lines of code without the #ifdef case: spin_lock_irq(&bfqd->lock); rq = __bfq_dispatch_request(hctx); spin_unlock_irq(&bfqd->lock); and you could actually see what it did. But instead of trying to abstract it in some legible manner, that three-line obvious function got *three* copies of the same #if mess all enclosing rancom crap, and the end result is really hard to read. For example, I just spent a couple of minutes trying to make sense of the code, and stop that unreadable mess. I came up with the appended patch. It may not work for some reason I can't see, but that's not the point. The attached patch is not meant as a "apply this as-is". It's meant as a "look, you can write legible code where the statistics just go away when they are compiled out". Now that "obvious three-liner function" is not three lines any more, but it's at least *clear* straight-line code: 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_dispatch_statistics(hctx, rq, in_serv_queue, waiting_rq, idle_timer_disabled); and I am hopeful that when bfq_dispatch_statistics() is disabled, the compiler will be smart enough to not generate extra code, certainly not noticeably so. See? One is a mess of horrible ugly #ifdef's in the middle of the code that makes the end result completely illegible. The other is actually somewhat legible, and has a clear separation of the statistics gathering. And that *statistics* gathering is clearly optionally enabled or not. Not the mess of random code put together in random ways that are completely illegble. Your job as maintainer is _literally_ to tell people "f*ck no, that code is too ugly, you need to write it so that it can be maintained". And I'm doing my job. "F*ck no, that code is too ugly, you need to write it so that it can be maintained". Show some taste. Linus block/bfq-iosched.c | 55 ++++++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index bcb6d21baf12..47d88d03dadd 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -3689,35 +3689,14 @@ 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_dispatch_statistics(struct blk_mq_hw_ctx *hctx, struct request *rq, + struct bfq_queue *in_serv_queue, bool waiting_rq, 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 @@ -3752,8 +3731,32 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) bfqg_stats_update_io_remove(bfqg, rq->cmd_flags); } spin_unlock_irq(hctx->queue->queue_lock); +} +#else +static inline void bfq_dispatch_statistics(struct blk_mq_hw_ctx *hctx, struct request *rq, struct bfq_queue *in_serv_queue, bool waiting_rq, 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_dispatch_statistics(hctx, rq, in_serv_queue, waiting_rq, idle_timer_disabled); + return rq; }