[PATCH 1/4] add task handling notifier: base definitions

[PATCH 1/4] add task handling notifier: base definitions

[Jan Beulich]

This is the base patch, adding notification for task creation and
deletion.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
---
 include/linux/sched.h |    8 +++++++-
 kernel/fork.c         |   11 +++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

--- 2.6.24-rc5-notify-task.orig/include/linux/sched.h
+++ 2.6.24-rc5-notify-task/include/linux/sched.h
@@ -80,7 +80,7 @@ struct sched_param {
 #include <linux/rcupdate.h>
 #include <linux/futex.h>
 #include <linux/rtmutex.h>
-
+#include <linux/notifier.h>
 #include <linux/time.h>
 #include <linux/param.h>
 #include <linux/resource.h>
@@ -1700,6 +1700,12 @@ extern int do_execve(char *, char __user
 extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
 struct task_struct *fork_idle(int);
 
+#define TASK_NEW 1
+#define TASK_DELETE 2
+
+extern struct blocking_notifier_head task_notifier_list;
+extern struct atomic_notifier_head atomic_task_notifier_list;
+
 extern void set_task_comm(struct task_struct *tsk, char *from);
 extern void get_task_comm(char *to, struct task_struct *tsk);
 
--- 2.6.24-rc5-notify-task.orig/kernel/fork.c
+++ 2.6.24-rc5-notify-task/kernel/fork.c
@@ -46,6 +46,7 @@
 #include <linux/tsacct_kern.h>
 #include <linux/cn_proc.h>
 #include <linux/freezer.h>
+#include <linux/notifier.h>
 #include <linux/delayacct.h>
 #include <linux/taskstats_kern.h>
 #include <linux/random.h>
@@ -71,6 +72,11 @@ DEFINE_PER_CPU(unsigned long, process_co
 
 __cacheline_aligned DEFINE_RWLOCK(tasklist_lock);  /* outer */
 
+BLOCKING_NOTIFIER_HEAD(task_notifier_list);
+EXPORT_SYMBOL_GPL(task_notifier_list);
+ATOMIC_NOTIFIER_HEAD(atomic_task_notifier_list);
+EXPORT_SYMBOL_GPL(atomic_task_notifier_list);
+
 int nr_processes(void)
 {
 	int cpu;
@@ -121,6 +127,9 @@ void __put_task_struct(struct task_struc
 	WARN_ON(atomic_read(&tsk->usage));
 	WARN_ON(tsk == current);
 
+	atomic_notifier_call_chain(&atomic_task_notifier_list,
+				   TASK_DELETE, tsk);
+
 	security_task_free(tsk);
 	free_uid(tsk->user);
 	put_group_info(tsk->group_info);
@@ -1450,6 +1459,8 @@ long do_fork(unsigned long clone_flags,
 			set_tsk_thread_flag(p, TIF_SIGPENDING);
 		}
 
+		blocking_notifier_call_chain(&task_notifier_list, TASK_NEW, p);
+
 		if (!(clone_flags & CLONE_STOPPED))
 			wake_up_new_task(p, clone_flags);
 		else

Re: [PATCH 1/4] add task handling notifier: base definitions

[Andi Kleen]

"Jan Beulich" <jbeulich@novell.com> writes:

> This is the base patch, adding notification for task creation and
> deletion.

I like the basic concept. At some point I suspect we'll even need
a per task dynamic data allocator, that would remove even
more cruft.

Could you change the block notifier calls first to not take their lock
when there is nothing in the list (the common case)? It is 
weird that they don't already do this, but they should definitely
here.

I think the use of the blocking/atomic notifiers needs some comment.
I think I understand why you did it -- exit atomic because sleeping
in exit can be illegal and blocking for fork to allow GFP_KERNEL
allocations -- but that should be documented somewhere.

-Andi

Re: [PATCH 1/4] add task handling notifier: base definitions

[Matthew Helsley]

On Thu, 2007-12-20 at 13:12 +0000, Jan Beulich wrote:
> This is the base patch, adding notification for task creation and
> deletion.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> ---
>  include/linux/sched.h |    8 +++++++-
>  kernel/fork.c         |   11 +++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> --- 2.6.24-rc5-notify-task.orig/include/linux/sched.h
> +++ 2.6.24-rc5-notify-task/include/linux/sched.h
> @@ -80,7 +80,7 @@ struct sched_param {
>  #include <linux/rcupdate.h>
>  #include <linux/futex.h>
>  #include <linux/rtmutex.h>
> -
> +#include <linux/notifier.h>
>  #include <linux/time.h>
>  #include <linux/param.h>
>  #include <linux/resource.h>
> @@ -1700,6 +1700,12 @@ extern int do_execve(char *, char __user
>  extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
>  struct task_struct *fork_idle(int);
> 
> +#define TASK_NEW 1
> +#define TASK_DELETE 2
> +
> +extern struct blocking_notifier_head task_notifier_list;
> +extern struct atomic_notifier_head atomic_task_notifier_list;
> +
>  extern void set_task_comm(struct task_struct *tsk, char *from);
>  extern void get_task_comm(char *to, struct task_struct *tsk);
> 
> --- 2.6.24-rc5-notify-task.orig/kernel/fork.c
> +++ 2.6.24-rc5-notify-task/kernel/fork.c
> @@ -46,6 +46,7 @@
>  #include <linux/tsacct_kern.h>
>  #include <linux/cn_proc.h>
>  #include <linux/freezer.h>
> +#include <linux/notifier.h>
>  #include <linux/delayacct.h>
>  #include <linux/taskstats_kern.h>
>  #include <linux/random.h>
> @@ -71,6 +72,11 @@ DEFINE_PER_CPU(unsigned long, process_co
> 
>  __cacheline_aligned DEFINE_RWLOCK(tasklist_lock);  /* outer */
> 
> +BLOCKING_NOTIFIER_HEAD(task_notifier_list);
> +EXPORT_SYMBOL_GPL(task_notifier_list);
> +ATOMIC_NOTIFIER_HEAD(atomic_task_notifier_list);
> +EXPORT_SYMBOL_GPL(atomic_task_notifier_list);
> +

When these global notifier lists were proposed years ago folks at SGI
loudly objected with concerns over anticipated cache line bouncing on
512+ cpu machines. Is that no longer a concern?

>  int nr_processes(void)
>  {
>  	int cpu;
> @@ -121,6 +127,9 @@ void __put_task_struct(struct task_struc
>  	WARN_ON(atomic_read(&tsk->usage));
>  	WARN_ON(tsk == current);
> 
> +	atomic_notifier_call_chain(&atomic_task_notifier_list,
> +				   TASK_DELETE, tsk);
> +
>  	security_task_free(tsk);
>  	free_uid(tsk->user);
>  	put_group_info(tsk->group_info);

Would the atomic notifier call chain be necessary if you hooked into an
earlier section of do_exit() instead?

Cheers,
	-Matt Helsley

Re: [PATCH 1/4] add task handling notifier: base definitions

[Jan Beulich]

>> +BLOCKING_NOTIFIER_HEAD(task_notifier_list);
>> +EXPORT_SYMBOL_GPL(task_notifier_list);
>> +ATOMIC_NOTIFIER_HEAD(atomic_task_notifier_list);
>> +EXPORT_SYMBOL_GPL(atomic_task_notifier_list);
>> +
>
>When these global notifier lists were proposed years ago folks at SGI
>loudly objected with concerns over anticipated cache line bouncing on
>512+ cpu machines. Is that no longer a concern?

I can't see an alternative, since the serialization is unavoidable.

>> @@ -121,6 +127,9 @@ void __put_task_struct(struct task_struc
>>  	WARN_ON(atomic_read(&tsk->usage));
>>  	WARN_ON(tsk == current);
>> 
>> +	atomic_notifier_call_chain(&atomic_task_notifier_list,
>> +				   TASK_DELETE, tsk);
>> +
>>  	security_task_free(tsk);
>>  	free_uid(tsk->user);
>>  	put_group_info(tsk->group_info);
>
>Would the atomic notifier call chain be necessary if you hooked into an
>earlier section of do_exit() instead?

I'm afraid it is, as I was told that sleeping in the do_exit() path is not
generally possible.

Jan

Re: [PATCH 1/4] add task handling notifier: base definitions

[Matt Helsley]

On Wed, 2008-01-09 at 09:46 +0000, Jan Beulich wrote:
> >> +BLOCKING_NOTIFIER_HEAD(task_notifier_list);
> >> +EXPORT_SYMBOL_GPL(task_notifier_list);
> >> +ATOMIC_NOTIFIER_HEAD(atomic_task_notifier_list);
> >> +EXPORT_SYMBOL_GPL(atomic_task_notifier_list);
> >> +
> >
> >When these global notifier lists were proposed years ago folks at SGI
> >loudly objected with concerns over anticipated cache line bouncing on
> >512+ cpu machines. Is that no longer a concern?
> 
> I can't see an alternative, since the serialization is unavoidable.

There are definitely alternatives. Naturally they are all much more
complex than using a single notifier chain. You could do per-cpu chains
for example (ugly, I know..).

> >> @@ -121,6 +127,9 @@ void __put_task_struct(struct task_struc
> >>  	WARN_ON(atomic_read(&tsk->usage));
> >>  	WARN_ON(tsk == current);
> >> 
> >> +	atomic_notifier_call_chain(&atomic_task_notifier_list,
> >> +				   TASK_DELETE, tsk);
> >> +
> >>  	security_task_free(tsk);
> >>  	free_uid(tsk->user);
> >>  	put_group_info(tsk->group_info);
> >
> >Would the atomic notifier call chain be necessary if you hooked into an
> >earlier section of do_exit() instead?
> 
> I'm afraid it is, as I was told that sleeping in the do_exit() path is not
> generally possible.
> 
> Jan

Odd. Last I checked I thought I saw a bunch of calls in do_exit() that
could sleep. Only in certain sections and at the end did it appear to
prevent sleeping.

Sorry for the late reply.

Cheers,
	-Matt Helsley