[RFC] [PATCH] subarch cleanup

[RFC] [PATCH] subarch cleanup

[john stultz]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

James, All,

	This is a small patch to try to somewhat cleanup the subarch code.
First it moves all the subarch .h files out of arch/i386/mach-xyz into
include/asm-i386/mach-xyz, then it changes the include patch to include
include/asm-i386/mach-xyz and include/asm-i386/mach-generic when
compiling. This allows the compiler to use the arch specific .h files
when needed, and then falls back to the generic .h files if no subarch
specific changes are needed. 

Obviously this doesn't work with .c files, so I've split up the Makefile
MACHINE variable into MACHINE_H and MACHINE_C, so subarchs like summit
which does not need any subarch specific .c files can just use the
generic files. 

The patch is bziped due to its size, but that is mainly due to moving
all the .h files, the only file changed is arch/i386/Makefile and I've
inlined that diff below.

Please let me know if you have any comments, flames or suggestions.

thanks
-john

diff -Nru a/arch/i386/Makefile b/arch/i386/Makefile
--- a/arch/i386/Makefile	Mon Nov 18 18:12:25 2002
+++ b/arch/i386/Makefile	Mon Nov 18 18:12:25 2002
@@ -47,9 +47,11 @@
 CFLAGS += $(cflags-y)
 
 ifdef CONFIG_VISWS
-MACHINE	:= mach-visws
+MACHINE_C	:= mach-visws
+MACHINE_H	:= mach-visws
 else
-MACHINE	:= mach-generic
+MACHINE_C	:= mach-generic
+MACHINE_H	:= mach-generic
 endif
 
 HEAD := arch/i386/kernel/head.o arch/i386/kernel/init_task.o
@@ -57,14 +59,14 @@
 libs-y 					+= arch/i386/lib/
 core-y					+= arch/i386/kernel/ \
 					   arch/i386/mm/ \
-					   arch/i386/$(MACHINE)/
+					   arch/i386/$(MACHINE_C)/
 drivers-$(CONFIG_MATH_EMULATION)	+= arch/i386/math-emu/
 drivers-$(CONFIG_PCI)			+= arch/i386/pci/
 # FIXME: is drivers- right ?
 drivers-$(CONFIG_OPROFILE)		+= arch/i386/oprofile/
 
-CFLAGS += -Iarch/i386/$(MACHINE)
-AFLAGS += -Iarch/i386/$(MACHINE)
+CFLAGS += -Iinclude/asm-i386/$(MACHINE_H) -Iinclude/asm-i386/mach-generic
+AFLAGS += -Iinclude/asm-i386/$(MACHINE_H) -Iinclude/asm-i386/mach-generic
 
 makeboot = $(call descend,arch/i386/boot,$(1))



[-- Attachment #2: linux-2.5.48_subarch-cleanup_A0.patch.bz2 --]
[-- Type: application/x-bzip, Size: 8671 bytes --]

Re: [RFC] [PATCH] subarch cleanup

[Sam Ravnborg]

On Tue, Nov 19, 2002 at 04:00:29PM -0800, john stultz wrote:
> James, All,
> 
> 	This is a small patch to try to somewhat cleanup the subarch code.
> First it moves all the subarch .h files out of arch/i386/mach-xyz into
> include/asm-i386/mach-xyz, then it changes the include patch to include
> include/asm-i386/mach-xyz and include/asm-i386/mach-generic when
> compiling. This allows the compiler to use the arch specific .h files
> when needed, and then falls back to the generic .h files if no subarch
> specific changes are needed. 
Why do you need to move the .h files?

>  ifdef CONFIG_VISWS
> -MACHINE	:= mach-visws
> +MACHINE_C	:= mach-visws
> +MACHINE_H	:= mach-visws
>  else
> -MACHINE	:= mach-generic
> +MACHINE_C	:= mach-generic
> +MACHINE_H	:= mach-generic

No reason to have two different variables assigned the same value.
If you are modifying this anyway consider something like:
machine-y               := mach-generic
machine-$(CONFIG_VISWS) := mach-visws

And then replace $(MACHINE) with $(machine-y).
This makes it much cleaner to add summit for example.

> -CFLAGS += -Iarch/i386/$(MACHINE)
> -AFLAGS += -Iarch/i386/$(MACHINE)
> +CFLAGS += -Iinclude/asm-i386/$(MACHINE_H) -Iinclude/asm-i386/mach-generic
> +AFLAGS += -Iinclude/asm-i386/$(MACHINE_H) -Iinclude/asm-i386/mach-generic

What's wrong with:
CFLAGS += -Iarch/i386/$(MACHINE_H) -Iarch/i386/mach-generic
That should achieve the same effect?

	Sam

Re: [RFC] [PATCH] subarch cleanup

[Martin J. Bligh]

> Why do you need to move the .h files?

Because they're in a silly place now. They should be whereever all
the other include files are.

> CFLAGS += -Iarch/i386/$(MACHINE_H) -Iarch/i386/mach-generic
> That should achieve the same effect?

Header files go under include ....

M.

Re: [RFC] [PATCH] subarch cleanup

[Russell King]

On Thu, Nov 21, 2002 at 10:35:43AM -0800, Martin J. Bligh wrote:
> > Why do you need to move the .h files?
> 
> Because they're in a silly place now. They should be whereever all
> the other include files are.
> 
> > CFLAGS += -Iarch/i386/$(MACHINE_H) -Iarch/i386/mach-generic
> > That should achieve the same effect?
> 
> Header files go under include ....

In this instance I'd disagree.  Think about UML.  UML has:

	include/asm-um/*.h
	include/asm-um/arch -> include/asm-i386

When building for UML, what happens if you need to get to a machine
specific file for something, and the i386 include files do:

	#include <asm/mach-generic/foo.h>

Yep, it fails.

Now guess why we in the ARM community haven't even bothered to look at
UML yet?  There's over 1MB of include files that would need to be moved,
along with countless #include statements needing to be fixed up.

For something that would be nice to have, and probably run quite well on
the ARM architecture (due to some nice features ARM has, especially for
UML's jail mode) there isn't enough interest in it to warrant such a
painful reorganisation.

I'd therefore strongly recommend NOT going down the path of adding
subdirectories to include/asm-*.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [RFC] [PATCH] subarch cleanup

[Martin J. Bligh]

>> Header files go under include ....
> 
> In this instance I'd disagree.  Think about UML.  UML has:
> 
> 	include/asm-um/*.h
> 	include/asm-um/arch -> include/asm-i386
> 
> When building for UML, what happens if you need to get to a machine
> specific file for something, and the i386 include files do:
> 
> 	#include <asm/mach-generic/foo.h>
> 
> Yep, it fails.

I don't understand what UML is trying to do here, but if it wants
the architecture specific stuff, it has to do it in the same way 
as the arch itself. That's UML's problem ... it sounds like it's
just making invalid assumptions. Maybe the include paths
need to be in a seperate makefile to avoid maintainance problems.
 
> Now guess why we in the ARM community haven't even bothered to look at
> UML yet?  There's over 1MB of include files that would need to be moved,
> along with countless #include statements needing to be fixed up.
> 
> For something that would be nice to have, and probably run quite well on
> the ARM architecture (due to some nice features ARM has, especially for
> UML's jail mode) there isn't enough interest in it to warrant such a
> painful reorganisation.
> 
> I'd therefore strongly recommend NOT going down the path of adding
> subdirectories to include/asm-*.

Another way to fix this (which might be simpler anyway) is to leave
the Makefiles alone and create:

include/asm-i386/mach_apic.h
	#ifdef CONFIG_XYZ_MACHINE
		#include <asm/mach-xyz/mach_apic.h>
	#else
		#include <asm/mach-generic/mach_apic.h>

Or something along those lines.

Whilst it's a nice idea in principle, the current subarch system does 
not scale to any real number of subarches beyond 1 or 2, and need some 
rejigging.

M.

Re: [RFC] [PATCH] subarch cleanup

[J.E.J. Bottomley]

mbligh@aracnet.com said:
> > Why do you need to move the .h files?
> Because they're in a silly place now. They should be whereever all the
> other include files are.

> > CFLAGS += -Iarch/i386/$(MACHINE_H) -Iarch/i386/mach-generic
> > That should achieve the same effect?

> Header files go under include .... 

That's not necessarily true.  Externally useful header files go in include.  
Header files only used internally to the subsystem go in local directories.

The reason I put them under arch/i386 is because I didn't want the guts of the 
subarch splitup spilling into the kernel core.

While the subarch is local to i386, I think the headers should stay there.  If 
you want to make the subarch a global framework (and thus get agreement with 
Russel and ARM to use it) then putting them under the global include 
directories would probably make sense.

James

Re: [RFC] [PATCH] subarch cleanup

[Martin J. Bligh]

>> Header files go under include .... 
> 
> That's not necessarily true.  Externally useful header files go 
> in include.  Header files only used internally to the subsystem 
> go in local directories.

That's not true either. There are lots of header files under the
include tree that aren't externally useful. And every other header
file is under the include path ... putting them all mixed in with
C files is just making a mess.

> The reason I put them under arch/i386 is because I didn't want the 
> guts of the  subarch splitup spilling into the kernel core.

Que? How is include/asm-i386 any more "kernel core" than arch/i386?

M.

Re: [RFC] [PATCH] subarch cleanup

[J.E.J. Bottomley]

mbligh@aracnet.com said:
> That's not true either. There are lots of header files under the
> include tree that aren't externally useful.

It may be honoured more in the breach than the observance, but it's a custom 
nonetheless.

> And every other header file is under the include path ... putting them
> all mixed in with C files is just making a mess.

No, look at e.g. SCSI.  We have a scsi.h file in drivers/scsi which defines 
subsystem specific things that we only use within SCSI.  We have 
include/scsi/scsi.h which defines things other subsystems can use.

> Que? How is include/asm-i386 any more "kernel core" than arch/i386? 

Because the files are spreading.  I think there's value to keeping something 
tightly contained unless you're going to encourage others to use it.  
Interfaces are dangerous things: If you release them into the wild 
willy-nilly, they can come back and bite you (athough more often they just 
bite other people).

James

Re: [RFC] [PATCH] subarch cleanup

[Christoph Hellwig]

On Fri, Nov 22, 2002 at 10:40:55AM -0600, J.E.J. Bottomley wrote:
> > And every other header file is under the include path ... putting them
> > all mixed in with C files is just making a mess.
> 
> No, look at e.g. SCSI.  We have a scsi.h file in drivers/scsi which defines 
> subsystem specific things that we only use within SCSI.  We have 
> include/scsi/scsi.h which defines things other subsystems can use.

Which is pretty stupid, btw.  There are more than enough scsi HBA drivers
or emulation drivers outside drivers/scsi.  I have a longstanding plan
to rationalize the scsi headers (the current state is really really messy),
and that includes moving everything but the truly midlayer-specific parts
like non-exported function to headers in include/scsi/.

Re: [RFC] [PATCH] subarch cleanup

[Martin J. Bligh]

>> And every other header file is under the include path ... putting them
>> all mixed in with C files is just making a mess.
> 
> No, look at e.g. SCSI.  We have a scsi.h file in drivers/scsi which defines 
> subsystem specific things that we only use within SCSI.  We have 
> include/scsi/scsi.h which defines things other subsystems can use.

The fact that it exists elsewhere does not necessarily make it a 
model to follow ...
 
>> Que? How is include/asm-i386 any more "kernel core" than arch/i386? 
> 
> Because the files are spreading.  I think there's value to keeping something 
> tightly contained unless you're going to encourage others to use it.  
> Interfaces are dangerous things: If you release them into the wild 
> willy-nilly, they can come back and bite you (athough more often they just 
> bite other people).

I think that's somewhat unlikely, but still .... let's go back to the
root of the problem here, and look at how to fix it. It seems that the
current way that subarch is implemented is not workable for current
subarches - we've gone through this before on IRC, and you agreed it
was broken, but let's do it again for the sake of argument, and so that
other people can see the problem ....

Imagine for a second that we have 5 different subarches (say visws, 
voyager, numaq, summit, generic). Assign a letter to each (ABCDE) and
a number to each of the code areas they need to modify:

A: 123456 (generic, thus needs a default for everything).
B: 12
C: 3
D: 45
E: 56

Duplicating all the code sections into all the subarches is an
impractical maintainance nightmare. Yet that's how it seems
to be set up at the moment (kind of OK if you only have 1 subarch
apart from generic, but not in general).

Let's stick to discussing header files at first ... what we need is 
to pick up the machine specific file if there is one, else fall back
and pick up the generic file. The way I'd like to see this fixed is
to have an include path statement that picks things up in order ie
append include/asm/mach-specific then include/asm/mach-generic in
the include search path. The easiest way to do that (to me) seems to
be to convert the #include "foo.h" to #include <foo.h> statements
and break things out as John did - we'll automatically pick up the
correct include file from the path.

If you have a different suggestion for fixing subarch support, please
outline it ....

Martin.

Re: [RFC] [PATCH] subarch cleanup

[J.E.J. Bottomley]

mbligh@aracnet.com said:
> Duplicating all the code sections into all the subarches is an
> impractical maintainance nightmare. Yet that's how it seems to be set
> up at the moment (kind of OK if you only have 1 subarch apart from
> generic, but not in general). 

well, the way it works was modelled on the asm-arch to asm-generic setup (and 
we have currently twenty of those).

But still, I agree that a default fallback is a better way of doing it.

> If you have a different suggestion for fixing subarch support, please
> outline it .... 

Well, I think what Alan does in -ac6 is the correct approach (with 
mach-default fallback, not mach-generic, which is really PC specific).  The 
only difference between Alan and John's patches (apart from mach-default) is 
the _H _C split and the location of the header files.

I've no real objection to the _H _C split, other than it seems a bit 
contorted.  The intent I originally had was that all subarchs would have a 
small setup.c file (copied and modified from mach-generic), so I didn't 
envisage having a subarch which wanted to use the generic setup.c and a 
different _H directory.  Doing a _H _C split reduces simplicity.

As far as the location of _H.  All I'm really fishing for is a better reason 
than "because they're header files" basically because I believe interface 
containment has value.

James

Re: [RFC] [PATCH] subarch cleanup

[john stultz]

On Thu, 2002-11-21 at 10:33, Sam Ravnborg wrote:
> >  ifdef CONFIG_VISWS
> > -MACHINE	:= mach-visws
> > +MACHINE_C	:= mach-visws
> > +MACHINE_H	:= mach-visws
> >  else
> > -MACHINE	:= mach-generic
> > +MACHINE_C	:= mach-generic
> > +MACHINE_H	:= mach-generic
> 
> No reason to have two different variables assigned the same value.

Actually, in some cases, such as summit, we only have one header file
that is different from generic. We don't want to replicate all the
generic .c files, So in that case MACHINE_C := mach-generic and
MACHINE_H := mach-summit. 

There very well could be a better way to do this, but splitting up the
.h files which fall back to generic nicely, and the .c files which don't
seemed to make the most sense. 

> If you are modifying this anyway consider something like:
> machine-y               := mach-generic
> machine-$(CONFIG_VISWS) := mach-visws
> 
> And then replace $(MACHINE) with $(machine-y).
> This makes it much cleaner to add summit for example.

I agree, it could be cleaned up. I'll see if I can't clean it up
further.

thanks for the feedback
-john

Re: [RFC] [PATCH] subarch cleanup

[Sam Ravnborg]

On Thu, Nov 21, 2002 at 10:55:54AM -0800, john stultz wrote:
> Actually, in some cases, such as summit, we only have one header file
> that is different from generic. We don't want to replicate all the
> generic .c files, So in that case MACHINE_C := mach-generic and
> MACHINE_H := mach-summit. 

If we are going to extend the concept of machines for i386 we should
add a default CONFIG option for the generic case.
Then we could do something like:

core-$(CONFIG_MACH_I386GENERIC)  += arch/i386/mach-generic
core-$(CONFIG_MACH_VISWS)        += arch/i386/mach-visws
core-$(CONFIG_MACH_SUMMIT)       += arch/i386/mach-generic

mflags-$(CONFIG_MACH_VISWS)      := asm/mach-visws
mflags-$(CONFIG_MACH_SUMMIT)     := asm/mach-summit
mflags-y                         += asm/mach-generic
AFLAGS += $(mflags-y)
CFLAGS += $(mflags-y)

There is something similar done for arm in newest kernel.

	Sam

Re: [RFC] [PATCH] subarch cleanup

[Alan Cox]

On Thu, 2002-11-21 at 19:15, Sam Ravnborg wrote:
> mflags-$(CONFIG_MACH_VISWS)      := asm/mach-visws
> mflags-$(CONFIG_MACH_SUMMIT)     := asm/mach-summit
> mflags-y                         += asm/mach-generic
> AFLAGS += $(mflags-y)
> CFLAGS += $(mflags-y)
> 
> There is something similar done for arm in newest kernel.

-ac already does this for include's so that mach-default is used if the
system doesnt override it