> Il giorno 17 nov 2017, alle ore 20:29, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> ha scritto: > > 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. > 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>
