From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 1 of 3] Introduce __memcpy_toio32
Date: Sat, 31 Dec 2005 00:27:34 UTC
Message-ID: <fa.fv9t53d.g0g7in@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.64.0512301559160.3249@g5.osdl.org>

On Fri, 30 Dec 2005, Matt Mackall wrote:
> >
> > Where's the problem with the __HAVE_ARCH_* mechanism?
>
> The head penguin peed on it last week.

Actually "sprinkling with penguin pee" means that something is blessed
(it's like a kernel baptism). Maybe that's not very civilized, but hey,
penguins don't have thumbs, and are thus kind of limited in their actions.
Don't be speciesist.

So the head penguin didn't pee on it, it turned its back in disgust, and
hoped that it would freeze to death in the arctic winter.

And no, I don't like the __HAVE_ARCH_xxx mechanisms at all. They are
pointless, and hard to follow. If an architecture wants to use a generic
mechanism, it should do one of the following (or a combination):

 - use the config file mechanism, and use

	obj-$(CONFIG_GENERIC_FOO) += generic-foo.c

   in a Makefile to link in the generic version.

   Examples: CONFIG_RWSEM_GENERIC_SPINLOCK.

 - just include the generic header from its own header, eg just do a

	#include <asm-generic/div64.h>

   or similar.

Now, the latter in particular is very easy to follow: if you look into the
<asm/div64.h> file and see that it just includes <asm-generic/div64.h>,
it's very obvious what is going on and where to find the real
implementation. You never have to wonder what the indirection means.

Similarly, anybody that fixes the generic header file can _trivially_ grep
for its use. So the code stays clean, and there are absolutely zero
compile-time conditionals, and the linkages both ways are obvious. And
architectures that do _not_ use the generic routines are totally
unaffected by them, and they don't need to specify any flags like "I have
my own routines" to disable things.

Now, the CONFIG_GENERIC_FOO thing is a bit less obvious, and you may have
to know about that config option in order to realize that a particular
architecture is using a generic library routine, but at least with those
Kconfig options, the language to describe them is clean these days, and
it's _the_ standard way to express configuration information. So it may be
a bit subtler and more indirect, but once you get used to it, it too is
very clean.

In contrast, the __HAVE_ARCH_xxx thing has zero upsides. It just causes
#ifdef mess in C source files, and unnecessary noise in standard header
files. I know it's been there for a long time, but just grep for
__HAVE_ARCH_MEMCPY and cry. Why the hell should all the architectures that
have their own optimized memcpy() have to tell the rest of the world about
it?

[ Yeah, I know why: bad implementation choice. It could easily have been
  done with the asm-generic approach or CONFIG_GENERIC_MEMCPY, but it
  wasn't. Note that the __HAVE_ARCH_xxx thing isn't even a standard form:
  sometimes it's the negation: __ARCH_WANT_xxx, and sometimes it's called
  something else entirely, like USE_ELF_CORE_DUMP or HAVE_PCI_MMAP, or
  ARCH_HAS_PREFETCH or HAVE_CSUM_COPY_USER. My point being that it's
  totally ad-hoc and random. ]

So we do have tons of ugly stuff, I just am trying to argue for not making
more of it (I don't think it's a big enough deal that it would be worth it
trying to clean up old uses).

If you look at the mutex patches, for example, I think everybody will
agree that they look much _better_ after they moved to just using the
trivial "#include <asm-generic/mutex-xyzzy.h>" format. At least I don't
_think_ this is just a personal weird hang-up of mine. It's literally a
cleanliness issue.

			Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: (OT) [PATCH 1 of 3] Introduce __memcpy_toio32
Date: Sat, 31 Dec 2005 00:47:51 UTC
Message-ID: <fa.fvpr4jc.hgi62m@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.64.0512301638240.3249@g5.osdl.org>

On Sat, 31 Dec 2005, Jan Engelhardt wrote:
> >>
> >> The head penguin peed on it last week.
> >
> >Actually "sprinkling with penguin pee" means that something is blessed
> >(it's like a kernel baptism). Maybe that's not very civilized, but hey,
> >penguins don't have thumbs, and are thus kind of limited in their actions.
> >Don't be speciest.
>
> At least they could have used water instead of pee.

Hey, when you live at -40 deg C for long times, I challenge you to find
some liquid water to sprinkle around.

Antarctica is one of the driest places on earth - never mind that there's
tons of ice around.

You use what you have.

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: __weak vs ifdef
Date: Sat, 26 Jul 2008 19:41:28 UTC
Message-ID: <fa.Xkogu6P4g8awF9J5S9m89VIiy+I@ifi.uio.no>

On Fri, 25 Jul 2008, Matthew Wilcox wrote:

> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
>
> I strongly disagree.  I find it makes it harder to follow code flow
> when __weak functions are involved.  Ifdefs are ugly, no question, but
> they're easier to grep for

Hell no, they're not.

Our use of random HAVE_ARCH_xyz or ARCH_SUPPORTS_xyz etc stuff makes
things _totally_ impossible to grep for.

In contrast, it we did this code as

	#ifndef arch_pick_mmap_layout
	void __weak arch_pick_mmap_layout(struct mm_struct *mm)
	{
		mm->mmap_base = TASK_UNMAPPED_BASE;
		mm->get_unmapped_area = arch_get_unmapped_area;
		mm->unmap_area = arch_unmap_area;
	}
	#endif

then trying to grep for arch_pick_mmap_layout() would show EVERY SINGLE
RELEVANT CASE! And it would show the "__weak" there too, so that once
people get used to this convention, they'd have a really easy time
figuring out the rules from just the output of the 'grep'.

I really think that whoever started that 'HAVE_ARCH_x'/'ARCH_HAS_x' mess
with totally random symbols that have NOTHING WHAT-SO-EVER to do with the
actual symbols in question (so they do _not_ show up in grep'ing for some
use) should be shot.

We should never _ever_ use that model. And we use it way too much.

We should generally strive for the simpler and much more obvious

	/* Generic definition */
	#ifndef symbol
	int symbol(..)
	...
	#endif

and then architecture code can do

	#define symbol(x) ...

or if they want to do a function, and you _really_ don't like the '__weak'
part (or you want to make it an inline function and don't want the clash
with the real declaration), then you can just do

	static inline int symbol(x)
	{
		...
	}
	#define symbol symbol

and again it all works fine WITHOUT having to introduce some idiotic new
and unrelated element called ARCH_HAS_SYMBOL.

And now when you do 'git grep symbol' you really will see the rules. ALL
the rules. Not some random collection of uses that don't actually explain
why there are five different definitions of the same thing and then you
have to figure out which one gets used.

> My basic point here is that __weak makes the code easier to write but
> harder to read, and we're supposed to be optimising for easier to read.

But your basic point is flawed. The thing you advocate is actually harder
to read.

Yes, if you don't follow the coding style, and you write

	int __weak
	symbol(x)
	{

you are (a) a moronic rebel you never understood why the declaration
should be on one line and (b) as a result your 'grep' won't see the __weak
and you'll be confused about the rules.

But if we _consistently_ used

 - '#ifndef symbol' to avoid redeclaring something that the architecture
   overrides

 - and '__weak' to allow architectures to just override functions without
   extra work and rules

then after a while people would simply _know_ that very simple set of
rules, and a 'grep' would work so much better than it does now.

Really. Try it. Try it with 'arch_pick_mmap_layout' (with Andrews patch in
place). And then imagine that you'd be used to '__weak', and seeing that
additional

	mm/util.c:#ifndef arch_pick_mmap_layout
	mm/util.c:void __weak arch_pick_mmap_layout(struct mm_struct *mm)

in the output. Be honest now - wouldn't that actually _tell_ you something
relevant about that particular declaration? And make the fact that some
architectures override it _less_ confusing?

IOW, you could tell directly from the grep output that it's a "default
fallback". Which you definitely cannot tell right now, because we have
insane models for doing it.

		Linus