| Message ID | CA+55aFy34p7YAtts5e0Uxt5drKws8QHw5nPDoBi64YX2xXnLrw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 17, 2017 at 11:29 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > "F*ck no, that code is too ugly, you need to write it so that it can > be maintained". Dammit, the rest of the pull looks ok, so I'll take it anyway. But I really do expect you to (a) clean up that mess - maybe with my patch as an example, but maybe some entirely different way. The patch I sent is already deleted from my tree, it was purely meant as an example. (b) really push back on people when they send you ugly code It shouldn't have to always be me being upset and pushing back on the block pulls. Linus
> Il giorno 17 nov 2017, alle ore 20:29, Linus Torvalds <torvalds@linux-foundation.org> ha scritto: > > On Fri, Nov 17, 2017 at 8:51 AM, Jens Axboe <axboe@kernel.dk> 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. > Sorry for causing this problem. Yours was our first version, but then we feared that leaving useless instructions was worse than adding a burst of ifdefs. I'll try not to repeat this mistake. Thanks, Paolo > 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 > <patch.diff>
On Fri, Nov 17, 2017 at 12:18 PM, Paolo Valente <paolo.valente@linaro.org> wrote: > > Sorry for causing this problem. Yours was our first version, but then > we feared that leaving useless instructions was worse than adding a > burst of ifdefs. I'll try not to repeat this mistake. So I generally do not want people to depend on the compiler to do non-obvious conversions (so please do not write bad code that you just hope the compiler can sort out and make reasonable), but we do heavily rely on the compiler doing the obvious dead code removal particularly for when things end up not being used. A lot of the time that's very much something you can and should depend on, because we have abstraction layers that involves a lot of code just becoming no-ops on many architectures, but other architectures need some particular sequence. Another example is a lot of helper functions to do verification of various kernel rules that then are enabled by some debug option (eg CONFIG_DEBUG_ATOMIC_SLEEP etc), where we know that (and depend on) the compiler will do a reasonable job. That's not so different from having "statistics gathering function that compiles to nothing". And yes, sometimes the code might need to be written so that the compiler has an easier time _seeing_ that the core really goes away, so it's not necessarily a blind trust in the compiler. So for example, the functions that can go away should obviously be inline functions so that you don't end up having the compiler generate the arguments and the call to an empty function body, and so that the compiler can see that "oh, those arguments aren't even used at all, so now I can remove all the code that generates them". If you are comfy with assembler, it can actually be a great thing to occasionally just verify the actual generated code if it's some piece of code you care deeply about. Obviously for performance work, profiling happens, and then you see it that way. Because sometimes you find some silly "Duh!" moments, where you didn't realize that the compiler wasn't able to really get rid of something for some reason that is usually very obvious in hindsight, but wasn't actually obvious at all until you looked at what the compiler generated. Linus
On 11/17/2017 12:35 PM, Linus Torvalds wrote: > On Fri, Nov 17, 2017 at 11:29 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> "F*ck no, that code is too ugly, you need to write it so that it can >> be maintained". > > Dammit, the rest of the pull looks ok, so I'll take it anyway. > > But I really do expect you to > > (a) clean up that mess - maybe with my patch as an example, but maybe > some entirely different way. The patch I sent is already deleted from > my tree, it was purely meant as an example. > > (b) really push back on people when they send you ugly code > > It shouldn't have to always be me being upset and pushing back on the > block pulls. My bad, I have probably given Paolo and Luca a bit more leeway than usual, since they are new to upstream development and the BFQ code does need some cleanups here and there. A huge part of that was completed as part of getting it merged, but it still needs a bit of love. Now that they have received the Linus hazing, I guess the honey moon is over. In all seriousness, I do push back on crap quite often. Sometimes I miss some too...
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; }