On Fri, Nov 17, 2017 at 8:51 AM, Jens Axboe <axboe@xxxxxxxxx> 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;
}
