Sam Ravnborg took a look at the x86 unification patches and commented, "from the mails and discussions I expected it be be obvious what was i386 only, what was shared and what was x86_64 only." He listed 16 files in x86/pci and noted, "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." Andi Kleen concurred, "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?"

Linus Torvalds disagreed saying, "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." He added, "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'." Linus continued:

"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)."

From:	Sam Ravnborg [email blocked]
Subject: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 22:12:19 +0200

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



From:	Thomas Gleixner [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 22:25:14 +0200

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



From:	Linus Torvalds [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 14:24:05 -0700 (PDT)



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


From:	Andi Kleen [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 21:38:10 +0100


> 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



From:	Adrian Bunk [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 23:14:22 +0200

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



From:	Andi Kleen [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 22:34:23 +0100


>
> 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



From:	Linus Torvalds [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 14:51:13 -0700 (PDT)



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



From:	Linus Torvalds [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Tue, 11 Sep 2007 14:21:12 -0700 (PDT)



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



From:	Sam Ravnborg [email blocked]
Subject: Re: x86 merge - a little feedback
Date:	Wed, 12 Sep 2007 21:09:53 +0200

> 
> > 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

• Archive of above thread