[v2,1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
Commit Message
Currently sched.h and wait.h have circular dependency between both.
wait.h defines macros wake_up*() which use macros TASK_* defined by
sched.h. But as sched.h indirectly includes wait.h, such wait.h header
file can't include sched.h too. The side effect is when some file
includes wait.h and tries to use its wake_up*() macros, it's necessary
to include sched.h also.
This patch moves all TASK_* macros from linux/sched.h to a new header
file linux/task_state.h. This way, both sched.h and wait.h can include
task_state.h and fix the circular dependency. No need to include sched.h
anymore when wake_up*() macros are used.
Signed-off-by: David Cohen <dacohen@gmail.com>
---
include/linux/sched.h | 58 +-----------------------------------------
include/linux/task_state.h | 61 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/wait.h | 1 +
3 files changed, 63 insertions(+), 57 deletions(-)
create mode 100644 include/linux/task_state.h
Comments
On Mon, 2011-02-21 at 16:38 +0200, David Cohen wrote:
> Currently sched.h and wait.h have circular dependency between both.
> wait.h defines macros wake_up*() which use macros TASK_* defined by
> sched.h. But as sched.h indirectly includes wait.h, such wait.h header
> file can't include sched.h too. The side effect is when some file
> includes wait.h and tries to use its wake_up*() macros, it's necessary
> to include sched.h also.
> This patch moves all TASK_* macros from linux/sched.h to a new header
> file linux/task_state.h. This way, both sched.h and wait.h can include
> task_state.h and fix the circular dependency. No need to include sched.h
> anymore when wake_up*() macros are used.
I think Alexey already told you what you done wrong.
Also, I really don't like the task_state.h header, it assumes a lot of
things it doesn't include itself and only works because its using macros
and not inlines at it probably should.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 21, 2011 at 5:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2011-02-21 at 16:38 +0200, David Cohen wrote:
>> Currently sched.h and wait.h have circular dependency between both.
>> wait.h defines macros wake_up*() which use macros TASK_* defined by
>> sched.h. But as sched.h indirectly includes wait.h, such wait.h header
>> file can't include sched.h too. The side effect is when some file
>> includes wait.h and tries to use its wake_up*() macros, it's necessary
>> to include sched.h also.
>> This patch moves all TASK_* macros from linux/sched.h to a new header
>> file linux/task_state.h. This way, both sched.h and wait.h can include
>> task_state.h and fix the circular dependency. No need to include sched.h
>> anymore when wake_up*() macros are used.
>
> I think Alexey already told you what you done wrong.
>
> Also, I really don't like the task_state.h header, it assumes a lot of
> things it doesn't include itself and only works because its using macros
> and not inlines at it probably should.
Like wait.h I'd say. The main issue is wait.h uses TASK_* macros but
cannot properly include sched.h as it would create a circular
dependency. So a file including wait.h is able to compile because the
dependency of sched.h relies on wake_up*() macros and it's not always
used.
We can still drop everything else from task_state.h but the TASK_*
macros and then the problem you just pointed out won't exist anymore.
What do you think about it?
Br,
David
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2011-02-21 at 18:03 +0200, David Cohen wrote:
> On Mon, Feb 21, 2011 at 5:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, 2011-02-21 at 16:38 +0200, David Cohen wrote:
> >> Currently sched.h and wait.h have circular dependency between both.
> >> wait.h defines macros wake_up*() which use macros TASK_* defined by
> >> sched.h. But as sched.h indirectly includes wait.h, such wait.h header
> >> file can't include sched.h too. The side effect is when some file
> >> includes wait.h and tries to use its wake_up*() macros, it's necessary
> >> to include sched.h also.
> >> This patch moves all TASK_* macros from linux/sched.h to a new header
> >> file linux/task_state.h. This way, both sched.h and wait.h can include
> >> task_state.h and fix the circular dependency. No need to include sched.h
> >> anymore when wake_up*() macros are used.
> >
> > I think Alexey already told you what you done wrong.
> >
> > Also, I really don't like the task_state.h header, it assumes a lot of
> > things it doesn't include itself and only works because its using macros
> > and not inlines at it probably should.
>
> Like wait.h I'd say. The main issue is wait.h uses TASK_* macros but
> cannot properly include sched.h as it would create a circular
> dependency. So a file including wait.h is able to compile because the
> dependency of sched.h relies on wake_up*() macros and it's not always
> used.
> We can still drop everything else from task_state.h but the TASK_*
> macros and then the problem you just pointed out won't exist anymore.
> What do you think about it?
I'd much rather see a real cleanup.. eg. remove the need for sched.h to
include wait.h.
afaict its needed because struct signal_struct and struct sighand_struct
include a wait_queue_head_t. The inclusion seems to come through
completion.h, but afaict we don't actually need to include completion.h
because all we have is a pointer to a completion, which is perfectly
fine with an incomplete type.
This all would suggest we move the signal bits into their own header
(include/linux/signal.h already exists and seems inviting).
And then make sched.c include signal.h and completion.h.
But then, there might be a 'good' reason these signal bits live in
sched.h and not on their own, but I wouldn't know..
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
On Mon, Feb 21, 2011 at 05:20:45PM +0100, Peter Zijlstra wrote:
> > > I think Alexey already told you what you done wrong.
> > >
> > > Also, I really don't like the task_state.h header, it assumes a lot of
> > > things it doesn't include itself and only works because its using macros
> > > and not inlines at it probably should.
> >
> > Like wait.h I'd say. The main issue is wait.h uses TASK_* macros but
> > cannot properly include sched.h as it would create a circular
> > dependency. So a file including wait.h is able to compile because the
> > dependency of sched.h relies on wake_up*() macros and it's not always
> > used.
> > We can still drop everything else from task_state.h but the TASK_*
> > macros and then the problem you just pointed out won't exist anymore.
> > What do you think about it?
>
> I'd much rather see a real cleanup.. eg. remove the need for sched.h to
> include wait.h.
isn't that exactly what he's trying to achieve ? Moving TASK_* to its
own header is one approach, what other approach do you suggest ?
> afaict its needed because struct signal_struct and struct sighand_struct
> include a wait_queue_head_t. The inclusion seems to come through
yes.
> completion.h, but afaict we don't actually need to include completion.h
> because all we have is a pointer to a completion, which is perfectly
> fine with an incomplete type.
so maybe just dropping completion.h from sched.h would do it.
> This all would suggest we move the signal bits into their own header
> (include/linux/signal.h already exists and seems inviting).
>
> And then make sched.c include signal.h and completion.h.
you wouldn't prevent the underlying problem which is the need to include
sched.h whenever you include wait.h and use wake_up*()
On Mon, 2011-02-21 at 18:29 +0200, Felipe Balbi wrote:
> Hi,
>
> On Mon, Feb 21, 2011 at 05:20:45PM +0100, Peter Zijlstra wrote:
> > > > I think Alexey already told you what you done wrong.
> > > >
> > > > Also, I really don't like the task_state.h header, it assumes a lot of
> > > > things it doesn't include itself and only works because its using macros
> > > > and not inlines at it probably should.
> > >
> > > Like wait.h I'd say. The main issue is wait.h uses TASK_* macros but
> > > cannot properly include sched.h as it would create a circular
> > > dependency. So a file including wait.h is able to compile because the
> > > dependency of sched.h relies on wake_up*() macros and it's not always
> > > used.
> > > We can still drop everything else from task_state.h but the TASK_*
> > > macros and then the problem you just pointed out won't exist anymore.
> > > What do you think about it?
> >
> > I'd much rather see a real cleanup.. eg. remove the need for sched.h to
> > include wait.h.
>
> isn't that exactly what he's trying to achieve ? Moving TASK_* to its
> own header is one approach, what other approach do you suggest ?
No, he's making a bigger mess, and didn't I just make another
suggestion?
> > afaict its needed because struct signal_struct and struct sighand_struct
> > include a wait_queue_head_t. The inclusion seems to come through
>
> yes.
Is that a qualified statement that, yes, that is the only inclusion
path?
> > completion.h, but afaict we don't actually need to include completion.h
> > because all we have is a pointer to a completion, which is perfectly
> > fine with an incomplete type.
>
> so maybe just dropping completion.h from sched.h would do it.
No, that will result in non-compilation due to wait_queue_head_t usage.
> > This all would suggest we move the signal bits into their own header
> > (include/linux/signal.h already exists and seems inviting).
> >
> > And then make sched.c include signal.h and completion.h.
>
> you wouldn't prevent the underlying problem which is the need to include
> sched.h whenever you include wait.h and use wake_up*()
If you'd applied your brain for a second before hitting reply you'd have
noticed that at this point you'd (likely) be able to include sched.h
from wait.h. which is the right way about, you need to be able to
schedule in order to build waitqueues.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
On Mon, Feb 21, 2011 at 05:43:27PM +0100, Peter Zijlstra wrote:
> > > And then make sched.c include signal.h and completion.h.
> >
> > you wouldn't prevent the underlying problem which is the need to include
> > sched.h whenever you include wait.h and use wake_up*()
>
> If you'd applied your brain for a second before hitting reply you'd have
> noticed that at this point you'd (likely) be able to include sched.h
> from wait.h. which is the right way about, you need to be able to
> schedule in order to build waitqueues.
someone's in a good mood today ;-)
What you seem to have missed is that sched.h doesn't include wait.h, it
includes completion.h and completion.h needs wait.h due the
wait_queue_head_t it uses.
If someone finds a cleaner way to drop that need, then I'm all for it as
my original suggestion to the original patch was to include sched.h in
wait.h, but it turned out that it's not possible due to the reasons
already explained.
On Mon, 2011-02-21 at 18:54 +0200, Felipe Balbi wrote:
> What you seem to have missed is that sched.h doesn't include wait.h, it
> includes completion.h and completion.h needs wait.h due the
> wait_queue_head_t it uses.
Yeah, so? sched.h doesn't need completion.h, but like with wait.h I'd
argue the other way around, completion.h would want to include sched.h
> If someone finds a cleaner way to drop that need, then I'm all for it as
> my original suggestion to the original patch was to include sched.h in
> wait.h, but it turned out that it's not possible due to the reasons
> already explained.
Feh,. I'm saying the proposed solution stinks and if you want to make
things better you need to work on fixing whatever is in the way of
including sched.h from wait.h.
1) remove the inclusion of completion.h -- easy we can live with an
incomplete type.
2) move the other wait_queue_head_t users (signal_struct sighand_struct)
out of sched.h
3) ...
4) profit!
Just isolating the TASK_state bits isn't going to be enough, wait.h also
wants wake_up goo and schedule*(), therefore either include sched.h from
whatever .c file you're using wait.h bits or do the above cleanup.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 21, 2011 at 06:06:02PM +0100, Peter Zijlstra wrote:
> On Mon, 2011-02-21 at 18:54 +0200, Felipe Balbi wrote:
>
> > What you seem to have missed is that sched.h doesn't include wait.h, it
> > includes completion.h and completion.h needs wait.h due the
> > wait_queue_head_t it uses.
>
> Yeah, so? sched.h doesn't need completion.h, but like with wait.h I'd
> argue the other way around, completion.h would want to include sched.h
ok, now I get what you proposed. Still, we could have lived without the
sarcasm, but that's not subject to patching.
On 02/21, Peter Zijlstra wrote:
>
> afaict its needed because struct signal_struct and struct sighand_struct
> include a wait_queue_head_t. The inclusion seems to come through
> completion.h, but afaict we don't actually need to include completion.h
> because all we have is a pointer to a completion, which is perfectly
> fine with an incomplete type.
>
> This all would suggest we move the signal bits into their own header
> (include/linux/signal.h already exists and seems inviting).
Agreed, sched.h contatins a lot of garbage, including the signal bits.
As for signal_struct in particular I am not really sure, it is just
misnamed. It is in fact "struct process" or "struct thread_group". But
dequeue_signal/etc should go into signal.h.
The only problem, it is not clear how to test such a change.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/21, Oleg Nesterov wrote:
>
> On 02/21, Peter Zijlstra wrote:
> >
> > afaict its needed because struct signal_struct and struct sighand_struct
> > include a wait_queue_head_t. The inclusion seems to come through
> > completion.h, but afaict we don't actually need to include completion.h
> > because all we have is a pointer to a completion, which is perfectly
> > fine with an incomplete type.
> >
> > This all would suggest we move the signal bits into their own header
> > (include/linux/signal.h already exists and seems inviting).
>
> Agreed, sched.h contatins a lot of garbage, including the signal bits.
>
> As for signal_struct in particular I am not really sure, it is just
> misnamed. It is in fact "struct process" or "struct thread_group". But
> dequeue_signal/etc should go into signal.h.
>
> The only problem, it is not clear how to test such a change.
Ah. sched.h includes signal.h, the testing is not the problem.
So, we can (at least) safely move some declarations.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 21, 2011 at 06:06:02PM +0100, Peter Zijlstra wrote:
> 1) remove the inclusion of completion.h -- easy we can live with an
> incomplete type.
ACK
> 2) move the other wait_queue_head_t users (signal_struct sighand_struct)
> out of sched.h
>
> 3) ...
Compile test! :^)
> 4) profit!
>
> Just isolating the TASK_state bits isn't going to be enough, wait.h also
> wants wake_up goo and schedule*(), therefore either include sched.h from
> whatever .c file you're using wait.h bits or do the above cleanup.
Speaking of junk in sched.h, I'll send mm_types.h removal next merge window
and maybe cred.h.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 21, 2011 at 7:27 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/21, Oleg Nesterov wrote:
>>
>> On 02/21, Peter Zijlstra wrote:
>> >
>> > afaict its needed because struct signal_struct and struct sighand_struct
>> > include a wait_queue_head_t. The inclusion seems to come through
>> > completion.h, but afaict we don't actually need to include completion.h
>> > because all we have is a pointer to a completion, which is perfectly
>> > fine with an incomplete type.
>> >
>> > This all would suggest we move the signal bits into their own header
>> > (include/linux/signal.h already exists and seems inviting).
>>
>> Agreed, sched.h contatins a lot of garbage, including the signal bits.
>>
>> As for signal_struct in particular I am not really sure, it is just
>> misnamed. It is in fact "struct process" or "struct thread_group". But
>> dequeue_signal/etc should go into signal.h.
>>
>> The only problem, it is not clear how to test such a change.
>
> Ah. sched.h includes signal.h, the testing is not the problem.
If sched.h includes signal.h and we move wait_queue_head_t users to
signal.h, it means signal.h should include wait.h and then it is a
problem to include sched.h in wait.h.
>
> So, we can (at least) safely move some declarations.
Safely, yes, but it won't solve the issue for TASK_* in wait.h.
Br,
David
>
> Oleg.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -90,6 +90,7 @@ struct sched_param {
#include <linux/task_io_accounting.h>
#include <linux/latencytop.h>
#include <linux/cred.h>
+#include <linux/task_state.h>
#include <asm/processor.h>
@@ -169,63 +170,6 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
#endif
/*
- * Task state bitmask. NOTE! These bits are also
- * encoded in fs/proc/array.c: get_task_state().
- *
- * We have two separate sets of flags: task->state
- * is about runnability, while task->exit_state are
- * about the task exiting. Confusing, but this way
- * modifying one set can't modify the other one by
- * mistake.
- */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-#define __TASK_STOPPED 4
-#define __TASK_TRACED 8
-/* in tsk->exit_state */
-#define EXIT_ZOMBIE 16
-#define EXIT_DEAD 32
-/* in tsk->state again */
-#define TASK_DEAD 64
-#define TASK_WAKEKILL 128
-#define TASK_WAKING 256
-#define TASK_STATE_MAX 512
-
-#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW"
-
-extern char ___assert_task_state[1 - 2*!!(
- sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
-
-/* Convenience macros for the sake of set_task_state */
-#define TASK_KILLABLE (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
-#define TASK_STOPPED (TASK_WAKEKILL | __TASK_STOPPED)
-#define TASK_TRACED (TASK_WAKEKILL | __TASK_TRACED)
-
-/* Convenience macros for the sake of wake_up */
-#define TASK_NORMAL (TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE)
-#define TASK_ALL (TASK_NORMAL | __TASK_STOPPED | __TASK_TRACED)
-
-/* get_task_state() */
-#define TASK_REPORT (TASK_RUNNING | TASK_INTERRUPTIBLE | \
- TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
- __TASK_TRACED)
-
-#define task_is_traced(task) ((task->state & __TASK_TRACED) != 0)
-#define task_is_stopped(task) ((task->state & __TASK_STOPPED) != 0)
-#define task_is_dead(task) ((task)->exit_state != 0)
-#define task_is_stopped_or_traced(task) \
- ((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
-#define task_contributes_to_load(task) \
- ((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
- (task->flags & PF_FREEZING) == 0)
-
-#define __set_task_state(tsk, state_value) \
- do { (tsk)->state = (state_value); } while (0)
-#define set_task_state(tsk, state_value) \
- set_mb((tsk)->state, (state_value))
-
-/*
* set_current_state() includes a barrier so that the write of current->state
* is correctly serialised wrt the caller's subsequent test of whether to
* actually sleep:
new file mode 100644
@@ -0,0 +1,61 @@
+#ifndef _LINUX_TASK_H
+#define _LINUX_TASK_H
+
+/*
+ * Task state bitmask. NOTE! These bits are also
+ * encoded in fs/proc/array.c: get_task_state().
+ *
+ * We have two separate sets of flags: task->state
+ * is about runnability, while task->exit_state are
+ * about the task exiting. Confusing, but this way
+ * modifying one set can't modify the other one by
+ * mistake.
+ */
+#define TASK_RUNNING 0
+#define TASK_INTERRUPTIBLE 1
+#define TASK_UNINTERRUPTIBLE 2
+#define __TASK_STOPPED 4
+#define __TASK_TRACED 8
+/* in tsk->exit_state */
+#define EXIT_ZOMBIE 16
+#define EXIT_DEAD 32
+/* in tsk->state again */
+#define TASK_DEAD 64
+#define TASK_WAKEKILL 128
+#define TASK_WAKING 256
+#define TASK_STATE_MAX 512
+
+#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW"
+
+extern char ___assert_task_state[1 - 2*!!(
+ sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
+
+/* Convenience macros for the sake of set_task_state */
+#define TASK_KILLABLE (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
+#define TASK_STOPPED (TASK_WAKEKILL | __TASK_STOPPED)
+#define TASK_TRACED (TASK_WAKEKILL | __TASK_TRACED)
+
+/* Convenience macros for the sake of wake_up */
+#define TASK_NORMAL (TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE)
+#define TASK_ALL (TASK_NORMAL | __TASK_STOPPED | __TASK_TRACED)
+
+/* get_task_state() */
+#define TASK_REPORT (TASK_RUNNING | TASK_INTERRUPTIBLE | \
+ TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
+ __TASK_TRACED)
+
+#define task_is_traced(task) ((task->state & __TASK_TRACED) != 0)
+#define task_is_stopped(task) ((task->state & __TASK_STOPPED) != 0)
+#define task_is_dead(task) ((task)->exit_state != 0)
+#define task_is_stopped_or_traced(task) \
+ ((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
+#define task_contributes_to_load(task) \
+ ((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
+ (task->flags & PF_FREEZING) == 0)
+
+#define __set_task_state(tsk, state_value) \
+ do { (tsk)->state = (state_value); } while (0)
+#define set_task_state(tsk, state_value) \
+ set_mb((tsk)->state, (state_value))
+
+#endif
@@ -22,6 +22,7 @@
#include <linux/list.h>
#include <linux/stddef.h>
#include <linux/spinlock.h>
+#include <linux/task_state.h>
#include <asm/system.h>
#include <asm/current.h>