[PATCH] wait: include linux/sched.h

[PATCH] wait: include linux/sched.h

[Kaywinnit L. Frye]

signal_pending(), schedule*() and TASK_* are used in various macros in
wait.h and all defined in linux/sched.h

Cc: Michal Nazarewicz <m.nazarewicz@samsung.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Kaywinnit L. Frye <kaywinnit.lee.frye.2497@gmail.com>
---
 include/linux/wait.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3efc9f3..667a3d7 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -22,6 +22,7 @@
 #include <linux/list.h>
 #include <linux/stddef.h>
 #include <linux/spinlock.h>
+#include <linux/sched.h>
 #include <asm/system.h>
 #include <asm/current.h>
 
-- 
1.7.2.5

Re: [PATCH] wait: include linux/sched.h

[Ingo Molnar]

* Kaywinnit L. Frye <kaywinnit.lee.frye.2497@gmail.com> wrote:

> signal_pending(), schedule*() and TASK_* are used in various macros in
> wait.h and all defined in linux/sched.h
> 
> Cc: Michal Nazarewicz <m.nazarewicz@samsung.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Kaywinnit L. Frye <kaywinnit.lee.frye.2497@gmail.com>
> ---
>  include/linux/wait.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 3efc9f3..667a3d7 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -22,6 +22,7 @@
>  #include <linux/list.h>
>  #include <linux/stddef.h>
>  #include <linux/spinlock.h>
> +#include <linux/sched.h>
>  #include <asm/system.h>
>  #include <asm/current.h>

I'm quite sure this will break the build all around the place, because sched.h 
itself uses wait.h primitives.

To fix these super-headers like sched.h and to make wait.h self-sufficient the 
right and cleanest approach would be to split data types and primitive accessor 
functions from higher level helper methods (which inevitably mix different 
domains) and put them into two separate files.

We have a few such split files in the kernel tree, spinlock_types.h and 
spinlock_api.h, and this concept works reasonably well.

So here we'd need wait_types.h and wait_api.h and of course sched_types.h and 
sched_api.h.

For simplicity of migration wait_api.h could be wait.h itself and sched_api.h 
could be sched.h itself.

That way we'd stop the exponential explosion of header file inter-dependencies 
at its source and would create a clean hierarchy of data types (the *_types.h 
files, which never include *_api.h files), plus the real *_api.h files that .c 
files would use.

As a result we'd probably have faster build times and we could also remove a 
lot of CPP macro hell which is currently used to work around dependency 
spaghetti bugs.

Thanks,

	Ingo

Re: [PATCH] wait: include linux/sched.h

[Peter Zijlstra]

On Fri, 2011-05-06 at 09:54 +0200, Ingo Molnar wrote:

> > diff --git a/include/linux/wait.h b/include/linux/wait.h
> > index 3efc9f3..667a3d7 100644
> > --- a/include/linux/wait.h
> > +++ b/include/linux/wait.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/list.h>
> >  #include <linux/stddef.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/sched.h>
> >  #include <asm/system.h>
> >  #include <asm/current.h>
> 
> I'm quite sure this will break the build all around the place, because sched.h 
> itself uses wait.h primitives.

I'm very sure it will, people keep proposing this.

> To fix these super-headers like sched.h and to make wait.h self-sufficient the 
> right and cleanest approach would be to split data types and primitive accessor 
> functions from higher level helper methods (which inevitably mix different 
> domains) and put them into two separate files.
> 
> We have a few such split files in the kernel tree, spinlock_types.h and 
> spinlock_api.h, and this concept works reasonably well.
> 
> So here we'd need wait_types.h and wait_api.h and of course sched_types.h and 
> sched_api.h.
> 
> For simplicity of migration wait_api.h could be wait.h itself and sched_api.h 
> could be sched.h itself.

Argh, please don't. I've been arguing against that for a while now.

The right thing to do is to clean up sched.h and remove a lot of the
non-scheduler bits in there, like all the process and signal bits.

Re: [PATCH] wait: include linux/sched.h

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Fri, 2011-05-06 at 09:54 +0200, Ingo Molnar wrote:
> 
> > > diff --git a/include/linux/wait.h b/include/linux/wait.h
> > > index 3efc9f3..667a3d7 100644
> > > --- a/include/linux/wait.h
> > > +++ b/include/linux/wait.h
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/list.h>
> > >  #include <linux/stddef.h>
> > >  #include <linux/spinlock.h>
> > > +#include <linux/sched.h>
> > >  #include <asm/system.h>
> > >  #include <asm/current.h>
> > 
> > I'm quite sure this will break the build all around the place, because sched.h 
> > itself uses wait.h primitives.
> 
> I'm very sure it will, people keep proposing this.
> 
> > To fix these super-headers like sched.h and to make wait.h self-sufficient the 
> > right and cleanest approach would be to split data types and primitive accessor 
> > functions from higher level helper methods (which inevitably mix different 
> > domains) and put them into two separate files.
> > 
> > We have a few such split files in the kernel tree, spinlock_types.h and 
> > spinlock_api.h, and this concept works reasonably well.
> > 
> > So here we'd need wait_types.h and wait_api.h and of course sched_types.h and 
> > sched_api.h.
> > 
> > For simplicity of migration wait_api.h could be wait.h itself and sched_api.h 
> > could be sched.h itself.
> 
> Argh, please don't. I've been arguing against that for a while now.

Yours is not a very well argued argument i have to say ;-)

> The right thing to do is to clean up sched.h and remove a lot of the 
> non-scheduler bits in there, like all the process and signal bits.

That, in fact, is the first half of what i suggested - obviously type splitting 
means first moving unrelated bits out of it.

Lets call it 'purifying' the header.

But, and i talk from first-hand experience here, even if we do that the core 
problem creating header spaghetti still remains, even if a header file is 
'pure': helper/utility inline functions using 'other' types which requires the 
inclusion of those other types - this quickly explodes.

Lets see an example, say we have two types and two headers:

header-A.h:

 type A
 method::A1
 method::A2

header-B.h:

 type B
 method::B1
 method::B2

And header-B also includes helper functions, which often have the form of:

 helper-method::B1()
 {
	type A = method::A1();
	type B;

	if (B.field)
		return A;

	return NULL;
 }

Then this 'imports' type A into type B's header file - although type B does not 
really relate to type A, only the helper method B1 has some (often superficial) 
connection to it.

And if we have a similar helper method in header-A, making use of header-B, we 
have inclusion circular dependency. These are typically worked around via ugly
#define()s. So we have tons of ugly defines and messy dependencyes.

So if we separate "types + type-specific methods" from "helper functions" 
(after all the cleanup you suggested as well, i.e. moving unrelated 
functionality out), we have improved the situation materially: both header-A.h 
and header-B.h become 'pure' and there's also header-B-helpers.h that can 
freely mix type-A and type-B methods.

Once that split is done other types can also thus include header-B.h without 
implicitly including header-A and all of its dependencies.

The end result is that we get a clean hiearchy of types.

Obviously sched.h would include a lot of basic types to begin with, because it 
is a fundamental 'container' type (struct task_struct) sitting at a top, 
highlevel node of the type hierarchy - so the initial step you suggest would 
indeed get us most of the benefits.

Thanks,

	Ingo

Re: [PATCH] wait: include linux/sched.h

[Peter Zijlstra]

On Tue, 2011-05-10 at 12:36 +0200, Ingo Molnar wrote:
> 
> Obviously sched.h would include a lot of basic types to begin with, because it 
> is a fundamental 'container' type (struct task_struct) sitting at a top, 
> highlevel node of the type hierarchy - so the initial step you suggest would 
> indeed get us most of the benefits. 

I very well understand the whole header mess, and in particular last
time I looked at the sched.h vs wait.h thing simply removing the non
sched.c related things was enough to resolve this particular problem.

I really don't like the foo_type.h headers much and think we should only
use them as absolute last resort solutions.

I might have mis-read your email, but to me your:

  "To fix these super-headers like sched.h and to make wait.h
self-sufficient the right and cleanest approach would be to split data
types and primitive accessor functions.."

Seems to suggest to begin with that split, instead of starting with sane
cleanups like moving the signal and process (groups of tasks) bits out
into their own headers.

Re: [PATCH] wait: include linux/sched.h

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Tue, 2011-05-10 at 12:36 +0200, Ingo Molnar wrote:
> > 
> > Obviously sched.h would include a lot of basic types to begin with, because it 
> > is a fundamental 'container' type (struct task_struct) sitting at a top, 
> > highlevel node of the type hierarchy - so the initial step you suggest would 
> > indeed get us most of the benefits. 
> 
> I very well understand the whole header mess, and in particular last time I 
> looked at the sched.h vs wait.h thing simply removing the non sched.c related 
> things was enough to resolve this particular problem.

No!

The wait.h problem is this: why do we want to include sched.h in wait.h?

We do not want it:

 - the waitqueue definitions do not need it
 - the main waitqueue accessor methods do not need it

types using waitqueues should not need to include sched.h as well - directly or 
indirectly!

Why does wait.h 'need' sched.h to work standalone right: because a few 
convenience helper functions in wait.h use scheduler functionality and 
scheduler constants.

As a result of this mess much of wait.h is written in hard to read 1970's tech: 
in the C preprocessor macro language.

Note that even *those* helpers do not really need to know the layout of struct 
task_struct (about which most of sched.h's complexity is): they only need to 
know the task state bits and a few scheduler API methods related to the concept 
of scheduling/waiting.

And note in that context it very much makes sense to have wait_types.h and 
wait_api.h (wait.h): all the derivative types that use waitqueues do not need 
all the helper functions like wait_event() and do not need to include scheduler 
lowlevel context switching details ... Only .c files that *use* waitqueues need 
to include the full kit.

But even the first step of purifying the headers will be a big step forward.

> I really don't like the foo_type.h headers much and think we should only use 
> them as absolute last resort solutions.

I understood your point as cleaning up sched.h to be able to include sched.h in 
wait.h and with that solution i disagree violently: sched.h is about struct 
task_struct and wait.h does not need that - nor do the dozens of other types 
need it that use waitqueues.

We can move all the task definitions and APIs from sched.h into wait.h then yes 
that's the first good first step toward purer structure for both sched.h and 
wait.h.

We can also move signal bits into a separate header, to streamline sched.h some 
more - but sched.h will always be a 'container' kind of super-header, by its 
very nature.

Or if you want some other structure for it all then please outine that 
structure. We could certainly move struct task_struct definition into a 
separate task.h and keep all context switch related definitions in sched.h.

Then task.h will need to be included in tons of code that dereferences 
'current' directly. So for pragmatic reasons it's probably better to keep 
struct task_struct in sched.h. But we can do the more complex change as well, 
as long as there's a plan behind it and as long as there's someone doing it ;-)

> I might have mis-read your email, but to me your:
> 
>   "To fix these super-headers like sched.h and to make wait.h
> self-sufficient the right and cleanest approach would be to split data
> types and primitive accessor functions.."
> 
> Seems to suggest to begin with that split, instead of starting with sane
> cleanups like moving the signal and process (groups of tasks) bits out
> into their own headers.

Yeah. Either direction is fine to me and moving out unrelated stuff sure is the 
bulk of the work.

Thanks,

	Ingo