Date:	Sun, 7 Jul 2013 15:33:31 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	David Miller <davem@...emloft.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Network Development <netdev@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT] Networking

On Sun, Jul 7, 2013 at 2:27 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> Because quite frankly, the fs/select.c changes make me go: "No way in
> hell". Partly because of the idiotic and completely undescriptive
> naming, partly because of the disgisting calling convetions with
> random flags variables passed in to be changed be helper functions.

Actually, I take that second one back. I mis-read things.

So I would suggest fixing this by:

 - get rid of all those idiotic "ll" things. They describe nothing at
all, and code like this

        if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))

   should have made anybody sane go "WTF?" and wonder about bad drugs.
Seriously, I'm disappointed code like this reaches me. In some random
driver I can see bad naming conventions like this, but in the core
kernel? We should have better taste.

 - talk about what it is, not short-hand. The "ll" stands for "low
latency", but that makes it sound all good. Make it describe what it
actually does: "busy loop", and write it out. So that people
understand what the actual downsides are. We're not a marketing group.
People shouldn't go "Oh, I like low-latency select(), I'll set that
latency value to 100usec". It should damn well be clear that this
enables BUSY LOOPING.

   So no POLL_LL crap. Call it POLL_BUSY_LOOP. Make everybody aware of
what it actually does.

 - Stop the marketing crap #2:

    "Recommended value is 50. May increase power usage"

   WTF? The default value is 0. Not 50. And I think "May increase
power usage" is the lie of the century. I don't see that there is any
"may" about it. Since when did we sugar-coat things?

 - I was confused by that whole ll_flag vs can_ll thing. "can_ll" is
not about whether we "can", it's actually "should_busy_loop", and
about whether we *should* busy loop. And to mention that nasty "lots
of nonsensical ll" line again:

                if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))

   that doesn't make much sense. If ll_flag isn't set, then can_ll
shouldn't have been set either, so why are we testing both values? And
because the code is mixing booleans with that mask, it's all
unnecessarily complicated in general. We'd be much better off having
"can_ll" be a mask value, and (along with renaming) we'd have

        unsigned int should_busy_loop = 0;
        ...
        should_busy_loop != mask & busy_loop_flag;

which does it all without conditionals, just by making the types
simpler. And then remove that "ll_flag && can_ll &&" which should be
just pointless, replacing it with just testing "should_busy_loop".

(That "can_poll_ll" and ll_start/ll_time should obviously *also* get
fixed naming-wise)

Finally, there is NO WAY IN HELL that busy-looping is ok if
need_resched is set. So this code also needs to make it very very
clear that it tests "current->need_resched" before busy-looping
anything at all.

End result: I think the code is salvageable and people who want this
kind of busy-looping can have it. But I really don't want to merge it
as-is. I think it was badly done, I think it was badly documented, and
I think somebody over-sold the feature by emphasizing the upsides and
not the problems.

                       Linus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists