x86 merge - a little feedback

x86 merge - a little feedback

[Sam Ravnborg]

Hi Thomas et al.

After spending several hours fiddeling with and improving
the current Makefile for x86_64 I decided to take a closer look
at the x86 merge og i386 and x86_64.

I took a closer look at x86/pci. There are 16 C files.

>From the mails and discussions I expected it be be
obvious what was i386 only, what was shared and what was x86_64 only.

But see following table

Filename		i386	x86_64
acpi.c			X	X
common.c		X	X
direct.c		X	X
early.c			X	X
fixup.c			X	X
i386.c			X	X
init.c			X	X
irq.c			X
k8-bus.c			X
legacy.c		X	X
mmconfig_32.c		X
mmconfig_64.c			X
mmconfig-shared.c	X	X
numa.c			X
pcbios.c		X
visws.c			X

In the filename there is NOTHING for most of
the non-shared code that tell that this file is
used by only i386 or x86_64.

The exception is mmconfig that is prefixed with _32 versus _64.
But as I have understood the mails floating around using _32,_64
is a way to say here are a potential candidate for futher merging.

In a meged x86 tree it would be very beneficial to either include
in the filename that a specific file is i386 or x86_64 specific or
stuff them in a separate subdirectory.

If legacy.c numa.c, pcibios.c and visws.c placed in a directory named i386
then it would be obvious that this is i386 only.
Or they could be named filename_32 (or the uglier filename_i386).
As it stands out today the filename are kept but thier relationship are lost.

I dunno if this will address the concern of Andi about mixing i386 and x86_64
but to me at least things would be so much more obvious if the original
relationship are spelled out.

	Sam

Re: x86 merge - a little feedback

[Thomas Gleixner]

Sam,

On Tue, 2007-09-11 at 22:12 +0200, Sam Ravnborg wrote:
> Hi Thomas et al.
> 
> After spending several hours fiddeling with and improving
> the current Makefile for x86_64 I decided to take a closer look
> at the x86 merge og i386 and x86_64.
> 
> I took a closer look at x86/pci. There are 16 C files.
> 
> From the mails and discussions I expected it be be
> obvious what was i386 only, what was shared and what was x86_64 only.
> 
> But see following table
> 
> Filename		i386	x86_64
> acpi.c			X	X
> common.c		X	X
> direct.c		X	X
> early.c			X	X
> fixup.c			X	X
> i386.c			X	X
> init.c			X	X
> irq.c			X
> k8-bus.c			X
> legacy.c		X	X
> mmconfig_32.c		X
> mmconfig_64.c			X
> mmconfig-shared.c	X	X
> numa.c			X
> pcbios.c		X
> visws.c			X
> 
> In the filename there is NOTHING for most of
> the non-shared code that tell that this file is
> used by only i386 or x86_64.

True. I tried to unify the Makefile by using 

obj-$(CONFIG_X86_32) += ....
and
obj-$(CONFIG_X86_64) += ....

but I did fail due to link order problems in that code. I had not yet
time to go down and figure that out yet, but it is on my todo list.

> The exception is mmconfig that is prefixed with _32 versus _64.
> But as I have understood the mails floating around using _32,_64
> is a way to say here are a potential candidate for futher merging.

Yes, if it is possible and makes sense.

> In a meged x86 tree it would be very beneficial to either include
> in the filename that a specific file is i386 or x86_64 specific or
> stuff them in a separate subdirectory.
> 
> If legacy.c numa.c, pcibios.c and visws.c placed in a directory named i386
> then it would be obvious that this is i386 only.
> Or they could be named filename_32 (or the uglier filename_i386).
> As it stands out today the filename are kept but thier relationship are lost.

We concentrated first on the move to make it simple and binary
equivalent. The cleanup of code (merging, location, makefile updates)
are definitely on our todo list.

> I dunno if this will address the concern of Andi about mixing i386 and x86_64
> but to me at least things would be so much more obvious if the original
> relationship are spelled out.

Good point.

	tglx

Re: x86 merge - a little feedback

[Linus Torvalds]

On Tue, 11 Sep 2007, Thomas Gleixner wrote:
> 
> I tried to unify the Makefile by using 
> 
> obj-$(CONFIG_X86_32) += ....
> and
> obj-$(CONFIG_X86_64) += ....

Don't do that.

I think it would be much better to instead do something like

	obj-y += mmconfig_$(CONFIG_WORD_SIZE).o

to make it clear when we have a file that is conceptually the same, but 
has different implementations.

That also makes the unification (assuming/hoping it gets done) of such 
files much cleaner - you just merge them, and the obj-y line can just drop 
the $(CONFIG_WORD_SIZE) thing. Very logical.

> but I did fail due to link order problems in that code.

.. the above approach also gets rid of any link order problems.

			Linus

Re: x86 merge - a little feedback

[Adrian Bunk]

On Tue, Sep 11, 2007 at 10:12:19PM +0200, Sam Ravnborg wrote:
>...
> In a meged x86 tree it would be very beneficial to either include
> in the filename that a specific file is i386 or x86_64 specific or
> stuff them in a separate subdirectory.
> 
> If legacy.c numa.c, pcibios.c and visws.c placed in a directory named i386
> then it would be obvious that this is i386 only.
> Or they could be named filename_32 (or the uglier filename_i386).
> As it stands out today the filename are kept but thier relationship are lost.
>...

I'm not agreeing with you on this.

It seems artificial to think 32bit<->64bit was the only interesting 
distinction on x86 machines.

You might as well create different directories for i386<->i486+ or
pre-i686<->i686+.

As an example, visws.c is as much non-64bit as it is pre-i686.

> 	Sam

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: x86 merge - a little feedback

[Sam Ravnborg]

> 
> As an example, visws.c is as much non-64bit as it is pre-i686.
Bad example...
visws is a visws specific file that should naver have allowed
anywhere outside mach-visws.
Any link-order issues should have been dealt with in other ways.

	Sam

Re: x86 merge - a little feedback

[Adrian Bunk]

On Tue, Sep 11, 2007 at 11:05:16PM +0200, Sam Ravnborg wrote:
> > 
> > As an example, visws.c is as much non-64bit as it is pre-i686.
> Bad example...
> visws is a visws specific file that should naver have allowed
> anywhere outside mach-visws.

Exactly, it's not about 64bit at all.

Similar, e.g. legacy.c is not about 32bit<->64bit.

> Any link-order issues should have been dealt with in other ways.
> 
> 	Sam

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: x86 merge - a little feedback

[Christoph Hellwig]

On Tue, Sep 11, 2007 at 10:34:13PM +0200, Adrian Bunk wrote:
> As an example, visws.c is as much non-64bit as it is pre-i686.

Bullshit.  visws were shipped with P3s.

Re: x86 merge - a little feedback

[Lennart Sorensen]

On Wed, Sep 12, 2007 at 10:27:16AM +0100, Christoph Hellwig wrote:
> On Tue, Sep 11, 2007 at 10:34:13PM +0200, Adrian Bunk wrote:
> > As an example, visws.c is as much non-64bit as it is pre-i686.
> 
> Bullshit.  visws were shipped with P3s.

Certainly true, but still not 64bit and never will be.

--
Len Sorensen

Re: x86 merge - a little feedback

[Andi Kleen]

> In the filename there is NOTHING for most of
> the non-shared code that tell that this file is
> used by only i386 or x86_64.

Exactly my point from KS. The big mash-up will not really make much difference
in terms of Makefile clarity or whatever Thomas' point was. Apparently he 
wanted to eliminate a few lines of code from the Makefile and merge
the header files completely? 

Anyways, the end result will be roughly the same as it is now: i386 and x86-64
are shared in not completely obvious ways and if you change one you have to 
test compile the other too.

Same old, same old, as always.

> I dunno if this will address the concern of Andi about mixing i386 and
> x86_64 but to me at least things would be so much more obvious if the
> original relationship are spelled out.

In the end it won't make much difference where the files are located
(although I frankly don't see the advantage of this intrusive move).

You always have to at least compile test both if you change one and I doubt 
most  people will be able to avoid this no matter how the Makefile looks
or where the files are. 

Even if everything was merged together and only ifdefs remained that
fundamental fact would not change either.

A few more files could be also definitely shared, no argument. e.g. 
the time subsystem will likely to be shared soon anyways. And probably
a few others. That should be better all done carefully step by step and 
properly reviewed though, not in some kind of brutal "rewrite the world" 
event.

My concern is mostly that he seems to want to merge some things between 32bit 
and  64bit (like the APIC drivers or the crappy i386 maze-of-inlines 
subarchitecture design) which ought not be together. I think I managed to 
keep x86-64 a relatively clean port over-all, but I see this now going down 
the drain :/

-Andi

Re: x86 merge - a little feedback

[Adrian Bunk]

On Tue, Sep 11, 2007 at 09:38:10PM +0100, Andi Kleen wrote:
>...
> In the end it won't make much difference where the files are located
> (although I frankly don't see the advantage of this intrusive move).
> 
> You always have to at least compile test both if you change one and I doubt 
> most  people will be able to avoid this no matter how the Makefile looks
> or where the files are. 
>...

The important point is:

People do not expect code under arch/i386/ to be used by code under 
arch/x86_64/ and vice versa.

That regularly results in people sending patches that don't compile on 
the other architecture.

With one architecture it's much more obvious that the code is shared.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: x86 merge - a little feedback

[Andi Kleen]

>
> People do not expect code under arch/i386/ to be used by code under
> arch/x86_64/ and vice versa.
>
> That regularly results in people sending patches that don't compile on
> the other architecture.
>
> With one architecture it's much more obvious that the code is shared.

Will that cause people to compile test both? I have my doubts that 
will really work.

e.g. a similar example would be CONFIG_MMU=n. The code 
is mostly shared and in the same directories, but people still
break the MMUless architectures all the time. 

I don't expect this to be different with 32bit/64bit.

-Andi

Re: x86 merge - a little feedback

[Linus Torvalds]

On Tue, 11 Sep 2007, Andi Kleen wrote:
> 
> Will that cause people to compile test both? I have my doubts that 
> will really work.

If people don't compile-test both now, then why would they compile-test 
things when merged?

So no, that's not the point.

But at least things like "grep" will work sanely, and people will be 
*aware* that "Oh, this touches a file that may be used by the other 
word-size".

Right now, we have people changing "i386-only" files that turn out to be 
used by x86-64 too - through very subtle Makefile things that the person 
who only looks into the i386 Makefile will never even *see*.

THAT is the problem (well, at least part of it).

		Linus

Re: x86 merge - a little feedback

[Jan Engelhardt]

On Sep 11 2007 14:51, Linus Torvalds wrote:
>On Tue, 11 Sep 2007, Andi Kleen wrote:
>> 
>> Will that cause people to compile test both? I have my doubts that 
>> will really work.
>
>If people don't compile-test both now, then why would they compile-test 
>things when merged?
>
>So no, that's not the point.
>
>But at least things like "grep" will work sanely, and people will be 
>*aware* that "Oh, this touches a file that may be used by the other 
>word-size".

Speaking of which, how does sparc/sparc64 handle things?




	Jan
-- 

Re: x86 merge - a little feedback

[Adrian Bunk]

On Tue, Sep 11, 2007 at 10:34:23PM +0100, Andi Kleen wrote:
> 
> >
> > People do not expect code under arch/i386/ to be used by code under
> > arch/x86_64/ and vice versa.
> >
> > That regularly results in people sending patches that don't compile on
> > the other architecture.
> >
> > With one architecture it's much more obvious that the code is shared.
> 
> Will that cause people to compile test both? I have my doubts that 
> will really work.
>...

You will see that it could be shared, and it'll be much easier to see 
all configurations it's used in.

Currently, there are 6 or 7 different ways how a function under 
arch/i386/ could be used by a function under arch/x86_64/ (and vice 
versa) and it's non-trivial to figure out all usages when grep'ing for 
users.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: x86 merge - a little feedback

[Paul Mundt]

On Tue, Sep 11, 2007 at 10:34:23PM +0100, Andi Kleen wrote:
> > People do not expect code under arch/i386/ to be used by code under
> > arch/x86_64/ and vice versa.
> >
> > That regularly results in people sending patches that don't compile on
> > the other architecture.
> >
> > With one architecture it's much more obvious that the code is shared.
> 
> Will that cause people to compile test both? I have my doubts that 
> will really work.
> 
> e.g. a similar example would be CONFIG_MMU=n. The code 
> is mostly shared and in the same directories, but people still
> break the MMUless architectures all the time. 
> 
As I was the first one to do CONFIG_MMU=y/n in the same arch directory,
since 2.5, I can tell you that that's simply crap. The only reason
CONFIG_MMU=n gets broken all the time is because people don't think about
it in generic code, it's rarely broken in the architecture code, and even
with the most occasional of build tests most of that gets caught in a
hurry.

You do of course have to consider both cases when writing new code, but
those things tend to be pretty obvious. It's a bit more work for the arch
maintainer, but it's certainly far less confusing and problematic than
separating things out.

In fact, going the opposite route is what leads to endless trouble in the
long run, since you brought up the MMUless example, m68knommu is a good
example. Rather than beginning life in arch/m68k, it was forked off,
mostly to deal with the ColdFire CPUs that weren't planned to have MMUs.
Now that the product line has moved along, adding an MMU to it is in the
roadmap, which means that inevitably they're both going to have to be
combined anyways. Simply dealing with the initial trouble of having them
combined initially would have solved a lot of that mess.

You can ignore the added maintenance for as long as possible, but sooner
or later it's going to be a problem. Procrastination is not something
that bodes particularly well for divergent hardware support.

Re: x86 merge - a little feedback

[Andrew Morton]

On Wed, 12 Sep 2007 09:29:53 +0900 Paul Mundt <lethal@linux-sh.org> wrote:

> As I was the first one to do CONFIG_MMU=y/n in the same arch directory,
> since 2.5, I can tell you that that's simply crap. The only reason
> CONFIG_MMU=n gets broken all the time is because people don't think about
> it in generic code, it's rarely broken in the architecture code, and even
> with the most occasional of build tests most of that gets caught in a
> hurry.

oy.  I do sh allmodconfig test builds fairly regularly.

<does one>

It all went OK, up until moddep:

ERROR: "_ebss" [drivers/mtd/maps/uclinux.ko] undefined!

Re: x86 merge - a little feedback

[Andrew Morton]

On Tue, 11 Sep 2007 23:14:22 +0200 Adrian Bunk <bunk@kernel.org> wrote:

> People do not expect code under arch/i386/ to be used by code under 
> arch/x86_64/ and vice versa.

[OT: it drives me batshit that we ended up including stuff in both directions]

Re: x86 merge - a little feedback

[Andi Kleen]

On Sat, Sep 15, 2007 at 02:32:58AM -0700, Andrew Morton wrote:
> On Tue, 11 Sep 2007 23:14:22 +0200 Adrian Bunk <bunk@kernel.org> wrote:
> 
> > People do not expect code under arch/i386/ to be used by code under 
> > arch/x86_64/ and vice versa.
> 
> [OT: it drives me batshit that we ended up including stuff in both directions]

Why? 

Anyways, i wouldn't have a problem with putting the already shared
files into a different directory or move it over to one of the architectures, 
although I must admit I personally wouldn't see a big benefit from it. But if 
it gives people a warm fuzzy feeling I'm all for it.

-Andi

Re: x86 merge - a little feedback

[Andrew Morton]

On Sat, 15 Sep 2007 20:36:23 +0200 Andi Kleen <andi@firstfloor.org> wrote:

> On Sat, Sep 15, 2007 at 02:32:58AM -0700, Andrew Morton wrote:
> > On Tue, 11 Sep 2007 23:14:22 +0200 Adrian Bunk <bunk@kernel.org> wrote:
> > 
> > > People do not expect code under arch/i386/ to be used by code under 
> > > arch/x86_64/ and vice versa.
> > 
> > [OT: it drives me batshit that we ended up including stuff in both directions]
> 
> Why? 

It's more complex, obviously.  More surprising.  It used to be the case that
arch/x86^4 files were xx86_64 and arch/i386 files were i386 and possibly
x86_64.  Now it's the case that arch/x86_64 files are x86_64 and maybe i386
and arch/i386 files are i386 and maybe x86_64.  Additional and quite
unnecessary complexity.

I mean, how often do x86_64 changes in your tree break i386?  Once every
3ish weeks would be my guess.  Often this will be because the person making
(and reviewing) the x86_64 change didn't know (or forgot) that the file is
also used by x86_64.

> Anyways, i wouldn't have a problem with putting the already shared
> files into a different directory or move it over to one of the architectures, 
> although I must admit I personally wouldn't see a big benefit from it. But if 
> it gives people a warm fuzzy feeling I'm all for it.

Doing something like that would reduce complexity, reduce surprise and
increase maintainability.  That's more than warm-and-fuzzies.

Re: x86 merge - a little feedback

[Linus Torvalds]

On Tue, 11 Sep 2007, Sam Ravnborg wrote:
> 
> From the mails and discussions I expected it be be obvious what was i386 
> only, what was shared and what was x86_64 only.

The problem right now is the *reverse* - even though they are in different 
subdirectories (and thus *look* like they are all separate), they aren't.

So the merged end result is much better: at a first approximation 
everything is shared (largely true), and the ones that aren't shared can 
be made more obvious.

> But see following table
> 
> Filename		i386	x86_64
> acpi.c		X	X
> common.c		X	X
> direct.c		X	X
> early.c		X	X
> fixup.c		X	X
> i386.c		X	X
> init.c		X	X
> legacy.c		X	X
> mmconfig-shared.c	X	X

So sharing is the default, as it should be.

> irq.c			X
> k8-bus.c			X
> numa.c			X
> pcbios.c		X

And these are examples of things that likely *should* be shared, and have 
nothing what-so-ever to do with 32- vs 64-bit issues. For example, neither 
NUMA nor k8 is in *any* way a 32-bit vs 64-bit issue, they are issues with 
chips that can be used as either.

> mmconfig_32.c		X
> mmconfig_64.c			X

And this is obvious, and correct.

> visws.c			X

Now, arguably this should be entirely elsewhere (ie in the "mach-visw" 
directory).

> In the filename there is NOTHING for most of
> the non-shared code that tell that this file is
> used by only i386 or x86_64.

.. and that's because of historical issues, and has nothing to do with the 
merge, and everything to do with all the problems that the merge is 
supposed to eventually help us FIX!

> The exception is mmconfig that is prefixed with _32 versus _64.
> But as I have understood the mails floating around using _32,_64
> is a way to say here are a potential candidate for futher merging.

Yes. But so are others.

> If legacy.c numa.c, pcibios.c and visws.c placed in a directory named i386
> then it would be obvious that this is i386 only.

But none of it is "i386 only" and putting it in a directory of its own 
would be stupid and wrong. The visws.c thing is platform-specific thing, 
and the fact that the platform happens to be 32-bit is totally secondary 
to the much bigger issue of the *platform*, so again, it would be totally 
wrong to split it up by wordsize.

> I dunno if this will address the concern of Andi about mixing i386 and x86_64
> but to me at least things would be so much more obvious if the original
> relationship are spelled out.

I obviously disagree violently. We should absolutely *not* make any of 
this depend on word-size. Quite the reverse!

		Linus

Re: x86 merge - a little feedback

[Sam Ravnborg]

> 
> > If legacy.c numa.c, pcibios.c and visws.c placed in a directory named i386
> > then it would be obvious that this is i386 only.
> 
> But none of it is "i386 only" and putting it in a directory of its own 
> would be stupid and wrong. The visws.c thing is platform-specific thing, 
> and the fact that the platform happens to be 32-bit is totally secondary 
> to the much bigger issue of the *platform*, so again, it would be totally 
> wrong to split it up by wordsize.

In other words - of all directories I used the worst one to prove my point.
I had no idea that legacy.c, numa.c and pcbios.c was candidates for x86_64 usage.

The point I try to make and which seems to have been lost in platform/wordsize
inputs are that there is a reason for being able to see which of the two
architectures a given file belong to.
Try to grep for csum_partial in x86/lib and you will get this:

checksum.S: * Changes:     Ingo Molnar, converted csum_partial_copy() to 2.1 exception
checksum.S:unsigned int csum_partial(const unsigned char * buff, int len, unsigned int sum)
checksum.S:ENTRY(csum_partial)
checksum.S:ENDPROC(csum_partial)
checksum.S:ENTRY(csum_partial)
checksum.S:ENDPROC(csum_partial)
checksum.S:unsigned int csum_partial_copy_generic (const char *src, char *dst,
checksum.S: * Copy from ds while checksumming, otherwise like csum_partial
checksum.S:ENTRY(csum_partial_copy_generic)
checksum.S:ENDPROC(csum_partial_copy_generic)
checksum.S:ENTRY(csum_partial_copy_generic)
checksum.S:ENDPROC(csum_partial_copy_generic)
csum-copy.S:ENTRY(csum_partial_copy_generic)
csum-copy.S:ENDPROC(csum_partial_copy_generic)
csum-partial.c:__wsum csum_partial(const void *buff, int len, __wsum sum)
csum-partial.c:EXPORT_SYMBOL(csum_partial);
csum-partial.c: return csum_fold(csum_partial(buff,len,0));
csum-wrappers.c: * csum_partial_copy_from_user - Copy and checksum from user space. 
csum-wrappers.c:csum_partial_copy_from_user(const void __user *src, void *dst,
csum-wrappers.c:                isum = csum_partial_copy_generic((__force const void *)src,
csum-wrappers.c:EXPORT_SYMBOL(csum_partial_copy_from_user);
csum-wrappers.c: * csum_partial_copy_to_user - Copy and checksum to user space. 
csum-wrappers.c:csum_partial_copy_to_user(const void *src, void __user *dst,
csum-wrappers.c:        return csum_partial_copy_generic(src, (void __force *)dst,len,isum,NULL,errp); 
csum-wrappers.c:EXPORT_SYMBOL(csum_partial_copy_to_user);
csum-wrappers.c: * csum_partial_copy_nocheck - Copy and checksum.
csum-wrappers.c:csum_partial_copy_nocheck(const void *src, void *dst, int len, __wsum sum)
csum-wrappers.c:        return csum_partial_copy_generic(src,dst,len,sum,NULL,NULL);
csum-wrappers.c:EXPORT_SYMBOL(csum_partial_copy_nocheck);


OK - maybe this is obvious for you and a few others. But for me I get utterly confused
about where to look for the x86_64 version.
Diving into the Makefile I can figure it out.

But thats one indirection too much.

As an example where this plays out better are in x86/crypto.
When grepping for aes_dec_blk I got following output:

aes_32.c:asmlinkage void aes_dec_blk(struct crypto_tfm *tfm, u8 *dst, const u8 *src);
aes_32.c:       aes_dec_blk(tfm, dst, src);
aes_64.c:asmlinkage void aes_dec_blk(struct crypto_tfm *tfm, u8 *out, const u8 *in);
aes_64.c:       aes_dec_blk(tfm, dst, src);
aes-i586-asm.S:/* void aes_dec_blk(struct crypto_tfm *tfm, u8 *out_blk, const u8 *in_blk) */
aes-i586-asm.S:.global  aes_dec_blk
aes-i586-asm.S:aes_dec_blk:
aes-x86_64-asm.S:/* void aes_dec_blk(struct crypto_tfm *tfm, u8 *out, const u8 *in) */
aes-x86_64-asm.S:       entry(aes_dec_blk,240,dec128,dec192)

See how obvious it is what are x86_64 specific and what are i386 specific.
And that despite the mixed naming convention used.

All files that _truely_ belongs to only one of the two architectures ought to
be named such that this is obvious when grepping like the above examples shows.

Using the wordsize to distingush the filename seems to cause confusion since there
are files that _truely_ only belongs to i386 but is not 32 bit specific because
they exist only on i386 because they are not needed on any x86_64 system.

	Sam