[PATCH 1/4] copy_signal cleanup: use zalloc and remove initializations

[PATCH 1/4] copy_signal cleanup: use zalloc and remove initializations

[Veaceslav Falico]

Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
lines.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

diff --git a/kernel/fork.c b/kernel/fork.c
index 166b8c4..160477d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -825,17 +825,6 @@ static void posix_cpu_timers_init_group(struct signal_struct *sig)
 	/* Thread group counters. */
 	thread_group_cputime_init(sig);
 
-	/* Expiration times and increments. */
-	sig->it[CPUCLOCK_PROF].expires = cputime_zero;
-	sig->it[CPUCLOCK_PROF].incr = cputime_zero;
-	sig->it[CPUCLOCK_VIRT].expires = cputime_zero;
-	sig->it[CPUCLOCK_VIRT].incr = cputime_zero;
-
-	/* Cached expiration times. */
-	sig->cputime_expires.prof_exp = cputime_zero;
-	sig->cputime_expires.virt_exp = cputime_zero;
-	sig->cputime_expires.sched_exp = 0;
-
 	if (sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY) {
 		sig->cputime_expires.prof_exp =
 			secs_to_cputime(sig->rlim[RLIMIT_CPU].rlim_cur);
@@ -855,7 +844,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	if (clone_flags & CLONE_THREAD)
 		return 0;
 
-	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
+	sig = kmem_cache_zalloc(signal_cachep, GFP_KERNEL);
 	tsk->signal = sig;
 	if (!sig)
 		return -ENOMEM;
@@ -863,33 +852,16 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	atomic_set(&sig->count, 1);
 	atomic_set(&sig->live, 1);
 	init_waitqueue_head(&sig->wait_chldexit);
-	sig->flags = 0;
 	if (clone_flags & CLONE_NEWPID)
 		sig->flags |= SIGNAL_UNKILLABLE;
-	sig->group_exit_code = 0;
-	sig->group_exit_task = NULL;
-	sig->group_stop_count = 0;
 	sig->curr_target = tsk;
 	init_sigpending(&sig->shared_pending);
 	INIT_LIST_HEAD(&sig->posix_timers);
 
 	hrtimer_init(&sig->real_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	sig->it_real_incr.tv64 = 0;
 	sig->real_timer.function = it_real_fn;
 
-	sig->leader = 0;	/* session leadership doesn't inherit */
-	sig->tty_old_pgrp = NULL;
-	sig->tty = NULL;
-
-	sig->utime = sig->stime = sig->cutime = sig->cstime = cputime_zero;
-	sig->gtime = cputime_zero;
-	sig->cgtime = cputime_zero;
-	sig->nvcsw = sig->nivcsw = sig->cnvcsw = sig->cnivcsw = 0;
-	sig->min_flt = sig->maj_flt = sig->cmin_flt = sig->cmaj_flt = 0;
-	sig->inblock = sig->oublock = sig->cinblock = sig->coublock = 0;
-	sig->maxrss = sig->cmaxrss = 0;
 	task_io_accounting_init(&sig->ioac);
-	sig->sum_sched_runtime = 0;
 	taskstats_tgid_init(sig);
 
 	task_lock(current->group_leader);

Re: [PATCH 1/4] copy_signal cleanup: use zalloc and remove initializations

[Oleg Nesterov]

On 12/01, Veaceslav Falico wrote:
>
> Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
> lines.

I like this series very much. Not only it cleanups and lessens the code,
it opens the possibility to do more cleanups. Say, we can simplify
exit_notify() a bit, we can remove ->group_exit_task check since we know
->notify_count < 0 can be false positive.

A couple of nits though,

> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 166b8c4..160477d 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -825,17 +825,6 @@ static void posix_cpu_timers_init_group(struct signal_struct *sig)
>  	/* Thread group counters. */
>  	thread_group_cputime_init(sig);
>
> -	/* Expiration times and increments. */
> -	sig->it[CPUCLOCK_PROF].expires = cputime_zero;
> -	sig->it[CPUCLOCK_PROF].incr = cputime_zero;
> -	sig->it[CPUCLOCK_VIRT].expires = cputime_zero;
> -	sig->it[CPUCLOCK_VIRT].incr = cputime_zero;
> -
> -	/* Cached expiration times. */
> -	sig->cputime_expires.prof_exp = cputime_zero;
> -	sig->cputime_expires.virt_exp = cputime_zero;
> -	sig->cputime_expires.sched_exp = 0;
> -
>  	if (sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY) {
>  		sig->cputime_expires.prof_exp =
>  			secs_to_cputime(sig->rlim[RLIMIT_CPU].rlim_cur);

Personally I don't mind, but perhaps it is better to move this change
into 3/4 which changes thread_group_cputime_init().

> @@ -855,7 +844,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
>  	if (clone_flags & CLONE_THREAD)
>  		return 0;
>
> -	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
> +	sig = kmem_cache_zalloc(signal_cachep, GFP_KERNEL);
>
> [...snip...]

Imho, very nice change.

Oleg.

Re: [PATCH 1/4] copy_signal cleanup: use zalloc and remove initializations

[Veaceslav Falico]

On Wed, Dec 02, 2009 at 02:57:59PM +0100, Oleg Nesterov wrote:
> 
> Personally I don't mind, but perhaps it is better to move this change
> into 3/4 which changes thread_group_cputime_init().
> 

Thank you for your review. I'll repost the fixed patchset tommorow.

--
Veaceslav 

[PATCH v2 1/4] copy_signal cleanup: use zalloc and remove initializations

[Veaceslav Falico]

Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
lines in copy_signal().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

diff --git a/kernel/fork.c b/kernel/fork.c
index 166b8c4..160477d 100644
@@ -855,7 +844,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	if (clone_flags & CLONE_THREAD)
 		return 0;
 
-	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
+	sig = kmem_cache_zalloc(signal_cachep, GFP_KERNEL);
 	tsk->signal = sig;
 	if (!sig)
 		return -ENOMEM;
@@ -863,33 +852,16 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	atomic_set(&sig->count, 1);
 	atomic_set(&sig->live, 1);
 	init_waitqueue_head(&sig->wait_chldexit);
-	sig->flags = 0;
 	if (clone_flags & CLONE_NEWPID)
 		sig->flags |= SIGNAL_UNKILLABLE;
-	sig->group_exit_code = 0;
-	sig->group_exit_task = NULL;
-	sig->group_stop_count = 0;
 	sig->curr_target = tsk;
 	init_sigpending(&sig->shared_pending);
 	INIT_LIST_HEAD(&sig->posix_timers);
 
 	hrtimer_init(&sig->real_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	sig->it_real_incr.tv64 = 0;
 	sig->real_timer.function = it_real_fn;
 
-	sig->leader = 0;	/* session leadership doesn't inherit */
-	sig->tty_old_pgrp = NULL;
-	sig->tty = NULL;
-
-	sig->utime = sig->stime = sig->cutime = sig->cstime = cputime_zero;
-	sig->gtime = cputime_zero;
-	sig->cgtime = cputime_zero;
-	sig->nvcsw = sig->nivcsw = sig->cnvcsw = sig->cnivcsw = 0;
-	sig->min_flt = sig->maj_flt = sig->cmin_flt = sig->cmaj_flt = 0;
-	sig->inblock = sig->oublock = sig->cinblock = sig->coublock = 0;
-	sig->maxrss = sig->cmaxrss = 0;
 	task_io_accounting_init(&sig->ioac);
-	sig->sum_sched_runtime = 0;
 	taskstats_tgid_init(sig);
 
 	task_lock(current->group_leader);

[PATCH v2 2/4] copy_signal cleanup: clean acct_init_pacct() and taskstats_tgid_init()

[Veaceslav Falico]

Remove unneeded initializations in acct_init_pacct() and taskstats_tgid_init().
These are accessed only via copy_signal() and are useless after using
kmem_cache_zalloc() in copy_signal().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

diff --git a/include/linux/taskstats_kern.h b/include/linux/taskstats_kern.h
index 3398f45..d66167a 100644
--- a/include/linux/taskstats_kern.h
+++ b/include/linux/taskstats_kern.h
@@ -16,7 +16,6 @@ extern struct mutex taskstats_exit_mutex;
 
 static inline void taskstats_tgid_init(struct signal_struct *sig)
 {
-	sig->stats = NULL;
 }
 
 static inline void taskstats_tgid_free(struct signal_struct *sig)
diff --git a/kernel/acct.c b/kernel/acct.c
index 9a4715a..8909c26 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -592,8 +592,6 @@ out:
  */
 void acct_init_pacct(struct pacct_struct *pacct)
 {
-	memset(pacct, 0, sizeof(struct pacct_struct));
-	pacct->ac_utime = pacct->ac_stime = cputime_zero;
 }
 
 /**

[PATCH v2 3/4] copy_signal cleanup: clean thread_group_cputime_init()

[Veaceslav Falico]

Remove unneeded initializations in thread_group_cputime_init() and
in posix_cpu_timers_init_group(). They are useless after 
kmem_cache_zalloc() was used in copy_signal().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 75e6e60..4778ef7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -825,17 +825,6 @@ static void posix_cpu_timers_init_group(struct
signal_struct *sig)
    /* Thread group counters. */
    thread_group_cputime_init(sig);
 
-   /* Expiration times and increments. */
-   sig->it[CPUCLOCK_PROF].expires = cputime_zero;
-   sig->it[CPUCLOCK_PROF].incr = cputime_zero;
-   sig->it[CPUCLOCK_VIRT].expires = cputime_zero;
-   sig->it[CPUCLOCK_VIRT].incr = cputime_zero;
-
-   /* Cached expiration times. */
-   sig->cputime_expires.prof_exp = cputime_zero;
-   sig->cputime_expires.virt_exp = cputime_zero;
-   sig->cputime_expires.sched_exp = 0;
-
    if (sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY) {
        sig->cputime_expires.prof_exp =
            secs_to_cputime(sig->rlim[RLIMIT_CPU].rlim_cur);
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2419,9 +2419,7 @@ void thread_group_cputimer(struct task_struct *tsk, struct task_cputime *times);
 
 static inline void thread_group_cputime_init(struct signal_struct *sig)
 {
-	sig->cputimer.cputime = INIT_CPUTIME;
 	spin_lock_init(&sig->cputimer.lock);
-	sig->cputimer.running = 0;
 }
 
 static inline void thread_group_cputime_free(struct signal_struct *sig)

[PATCH v2 4/4] copy_signal cleanup: clean tty_audit_fork()

[Veaceslav Falico]

Remove unneeded initialization in tty_audit_fork().
It is called only via copy_signal() and is useless after
the kmem_cache_zalloc() was used.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
index ac16fbe..283a15b 100644
--- a/drivers/char/tty_audit.c
+++ b/drivers/char/tty_audit.c
@@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig)
 	spin_lock_irq(&current->sighand->siglock);
 	sig->audit_tty = current->signal->audit_tty;
 	spin_unlock_irq(&current->sighand->siglock);
-	sig->tty_audit_buf = NULL;
 }
 
 /**

Re: [PATCH v2 1/4] copy_signal cleanup: use zalloc and remove initializations

[Oleg Nesterov]

On 12/04, Veaceslav Falico wrote:
>
> Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
> lines in copy_signal().
>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 166b8c4..160477d 100644
> @@ -855,7 +844,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
>  	if (clone_flags & CLONE_THREAD)
>  		return 0;
>
> -	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
> +	sig = kmem_cache_zalloc(signal_cachep, GFP_KERNEL);
> ...

I think this is nice cleanup,

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v2 2/4] copy_signal cleanup: clean acct_init_pacct() and taskstats_tgid_init()

[Oleg Nesterov]

On 12/04, Veaceslav Falico wrote:
>
> Remove unneeded initializations in acct_init_pacct() and taskstats_tgid_init().
> These are accessed only via copy_signal() and are useless after using
> kmem_cache_zalloc() in copy_signal().
>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> diff --git a/include/linux/taskstats_kern.h b/include/linux/taskstats_kern.h
> index 3398f45..d66167a 100644
> --- a/include/linux/taskstats_kern.h
> +++ b/include/linux/taskstats_kern.h
> @@ -16,7 +16,6 @@ extern struct mutex taskstats_exit_mutex;
>
>  static inline void taskstats_tgid_init(struct signal_struct *sig)
>  {
> -	sig->stats = NULL;
>  }
>
>  static inline void taskstats_tgid_free(struct signal_struct *sig)
> diff --git a/kernel/acct.c b/kernel/acct.c
> index 9a4715a..8909c26 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -592,8 +592,6 @@ out:
>   */
>  void acct_init_pacct(struct pacct_struct *pacct)
>  {
> -	memset(pacct, 0, sizeof(struct pacct_struct));
> -	pacct->ac_utime = pacct->ac_stime = cputime_zero;
>  }

Looks good to me, but I'd suggest you to send the next patch (on top
of this) which kills both these helpers. Otherwise we have empty
definitions with and without CONFIG_TASKSTATS, a bit strange.

Oleg.

Re: [PATCH v2 3/4] copy_signal cleanup: clean thread_group_cputime_init()

[Oleg Nesterov]

On 12/04, Veaceslav Falico wrote:
>
> Remove unneeded initializations in thread_group_cputime_init() and
> in posix_cpu_timers_init_group(). They are useless after
> kmem_cache_zalloc() was used in copy_signal().
>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 75e6e60..4778ef7 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -825,17 +825,6 @@ static void posix_cpu_timers_init_group(struct
> signal_struct *sig)
>     /* Thread group counters. */
>     thread_group_cputime_init(sig);
>
> -   /* Expiration times and increments. */
> -   sig->it[CPUCLOCK_PROF].expires = cputime_zero;
> -   sig->it[CPUCLOCK_PROF].incr = cputime_zero;
> -   sig->it[CPUCLOCK_VIRT].expires = cputime_zero;
> -   sig->it[CPUCLOCK_VIRT].incr = cputime_zero;
> -
> -   /* Cached expiration times. */
> -   sig->cputime_expires.prof_exp = cputime_zero;
> -   sig->cputime_expires.virt_exp = cputime_zero;
> -   sig->cputime_expires.sched_exp = 0;
> -
>     if (sig->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY) {
>         sig->cputime_expires.prof_exp =
>             secs_to_cputime(sig->rlim[RLIMIT_CPU].rlim_cur);
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2419,9 +2419,7 @@ void thread_group_cputimer(struct task_struct *tsk, struct task_cputime *times);
>
>  static inline void thread_group_cputime_init(struct signal_struct *sig)
>  {
> -	sig->cputimer.cputime = INIT_CPUTIME;
>  	spin_lock_init(&sig->cputimer.lock);
> -	sig->cputimer.running = 0;
>  }

Perhaps it makes sense to move thread_group_cputimer() into kernel/fork.c,
or even fold it into its single caller, posix_cpu_timers_init_group().

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v2 4/4] copy_signal cleanup: clean tty_audit_fork()

[Oleg Nesterov]

On 12/04, Veaceslav Falico wrote:
>
> Remove unneeded initialization in tty_audit_fork().
> It is called only via copy_signal() and is useless after
> the kmem_cache_zalloc() was used.
>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
> index ac16fbe..283a15b 100644
> --- a/drivers/char/tty_audit.c
> +++ b/drivers/char/tty_audit.c
> @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig)
>  	spin_lock_irq(&current->sighand->siglock);
>  	sig->audit_tty = current->signal->audit_tty;
>  	spin_unlock_irq(&current->sighand->siglock);
> -	sig->tty_audit_buf = NULL;
>  }

Can't comment the changes in audit code, but the patch looks obviously
correct.



Off-topic question to this who understands this code.

But afaics we can also remove ->siglock from this helper and make
it really trivial for being inline. ->siglock buys nothing, we just
read a boolean. In fact, after the quick grep I do not understand
how ->siglock is connected to ->audit_tty. OK, it protects tty_audit_buf,
but why we always take ->siglock to access ->audit_tty ?

Oleg.

Re: [PATCH v2 4/4] copy_signal cleanup: clean tty_audit_fork()

[Miloslav Trmac]

----- "Oleg Nesterov" <oleg@redhat.com> wrote:
> On 12/04, Veaceslav Falico wrote:
> > diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
> > index ac16fbe..283a15b 100644
> > --- a/drivers/char/tty_audit.c
> > +++ b/drivers/char/tty_audit.c
> > @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig)
> >  	spin_lock_irq(&current->sighand->siglock);
> >  	sig->audit_tty = current->signal->audit_tty;
> >  	spin_unlock_irq(&current->sighand->siglock);
> > -	sig->tty_audit_buf = NULL;
> >  }
> 
> Off-topic question to this who understands this code.
> 
> But afaics we can also remove ->siglock from this helper and make
> it really trivial for being inline. ->siglock buys nothing, we just
> read a boolean. In fact, after the quick grep I do not understand
> how ->siglock is connected to ->audit_tty. OK, it protects
> tty_audit_buf,
> but why we always take ->siglock to access ->audit_tty ?
AFAIK there is no explicit documentation of the atomicity semantics expected by the Linux kernel (both from the hardware and from the compiler), so every access to the boolean is protected by a lock, to be on the safe side.
    Mirek

Re: [PATCH v2 4/4] copy_signal cleanup: clean tty_audit_fork()

[Oleg Nesterov]

On 12/05, Miloslav Trmac wrote:
>
> ----- "Oleg Nesterov" <oleg@redhat.com> wrote:
> > On 12/04, Veaceslav Falico wrote:
> > > diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
> > > index ac16fbe..283a15b 100644
> > > --- a/drivers/char/tty_audit.c
> > > +++ b/drivers/char/tty_audit.c
> > > @@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig)
> > >  	spin_lock_irq(&current->sighand->siglock);
> > >  	sig->audit_tty = current->signal->audit_tty;
> > >  	spin_unlock_irq(&current->sighand->siglock);
> > > -	sig->tty_audit_buf = NULL;
> > >  }
> >
> > Off-topic question to this who understands this code.
> >
> > But afaics we can also remove ->siglock from this helper and make
> > it really trivial for being inline. ->siglock buys nothing, we just
> > read a boolean. In fact, after the quick grep I do not understand
> > how ->siglock is connected to ->audit_tty. OK, it protects
> > tty_audit_buf,
> > but why we always take ->siglock to access ->audit_tty ?
> AFAIK there is no explicit documentation of the atomicity semantics
> expected by the Linux kernel (both from the hardware and from the compiler),
> so every access to the boolean is protected by a lock, to be on the safe side.

Not sure I understand, but the kernel relies on fact it is always safe
to load/store a word.

What atomicity semantics do you mean and how ->siglock can help? Sure,
we can race with AUDIT_TTY_SET, but this can happen with or without
this lock. This "race" is unavoidable and harmless.

I believe every spin_lock(siglock) around ->audit_tty is bogus.

Oleg.

Re: [PATCH v2 1/4] copy_signal cleanup: use zalloc and remove initializations

[Balbir Singh]

* Veaceslav Falico <vfalico@redhat.com> [2009-12-04 15:28:14]:

> Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
> lines in copy_signal().
> 
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

Good cleanup, thanks!

-- 
	Balbir

Re: [PATCH v2 2/4] copy_signal cleanup: clean acct_init_pacct() and taskstats_tgid_init()

[Balbir Singh]

* Veaceslav Falico <vfalico@redhat.com> [2009-12-04 15:29:01]:

> Remove unneeded initializations in acct_init_pacct() and taskstats_tgid_init().
> These are accessed only via copy_signal() and are useless after using
> kmem_cache_zalloc() in copy_signal().
> 
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

As Oleg suggested, please reorder the cleanup to remove the empty
functions.

-- 
	Balbir

[PATCH] kill taskstats_tgid_init() and acct_init_pacct() and cleanup copy_signal()

[Veaceslav Falico]

(on top of previous patchset)

Kill taskstats_tgid_init() and acct_init_pacct() because we don't
need them any more. Cleanup copy_signal() from these functions
and remove the call to task_io_accounting_init() (we can't kill
it because it's still being used by copy_process(), so just removing
it from copy_signal()).

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

--- a/include/linux/taskstats_kern.h	2009-12-07 20:20:02.000000000 +0100
+++ b/include/linux/taskstats_kern.h	2009-12-07 20:20:48.000000000 +0100
@@ -14,10 +14,6 @@
 extern struct kmem_cache *taskstats_cache;
 extern struct mutex taskstats_exit_mutex;
 
-static inline void taskstats_tgid_init(struct signal_struct *sig)
-{
-}
-
 static inline void taskstats_tgid_free(struct signal_struct *sig)
 {
 	if (sig->stats)
@@ -29,8 +25,6 @@ extern void taskstats_init_early(void);
 #else
 static inline void taskstats_exit(struct task_struct *tsk, int group_dead)
 {}
-static inline void taskstats_tgid_init(struct signal_struct *sig)
-{}
 static inline void taskstats_tgid_free(struct signal_struct *sig)
 {}
 static inline void taskstats_init_early(void)
--- a/kernel/acct.c	2009-12-07 20:23:43.000000000 +0100
+++ b/kernel/acct.c	2009-12-07 20:24:30.000000000 +0100
@@ -587,14 +587,6 @@ out:
 }
 
 /**
- * acct_init_pacct - initialize a new pacct_struct
- * @pacct: per-process accounting info struct to initialize
- */
-void acct_init_pacct(struct pacct_struct *pacct)
-{
-}
-
-/**
  * acct_collect - collect accounting information into pacct_struct
  * @exitcode: task exit code
  * @group_dead: not 0, if this thread is the last one in the process.
--- a/include/linux/acct.h	2009-12-07 20:26:35.000000000 +0100
+++ b/include/linux/acct.h	2009-12-07 20:26:43.000000000 +0100
@@ -123,14 +123,12 @@ struct pacct_struct;
 struct pid_namespace;
 extern void acct_auto_close_mnt(struct vfsmount *m);
 extern void acct_auto_close(struct super_block *sb);
-extern void acct_init_pacct(struct pacct_struct *pacct);
 extern void acct_collect(long exitcode, int group_dead);
 extern void acct_process(void);
 extern void acct_exit_ns(struct pid_namespace *);
 #else
 #define acct_auto_close_mnt(x)	do { } while (0)
 #define acct_auto_close(x)	do { } while (0)
-#define acct_init_pacct(x)	do { } while (0)
 #define acct_collect(x,y)	do { } while (0)
 #define acct_process()		do { } while (0)
 #define acct_exit_ns(ns)	do { } while (0)
--- a/kernel/fork.c	2009-12-07 20:25:18.000000000 +0100
+++ b/kernel/fork.c	2009-12-07 20:36:49.000000000 +0100
@@ -861,17 +861,12 @@ static int copy_signal(unsigned long clo
 	hrtimer_init(&sig->real_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	sig->real_timer.function = it_real_fn;
 
-	task_io_accounting_init(&sig->ioac);
-	taskstats_tgid_init(sig);
-
 	task_lock(current->group_leader);
 	memcpy(sig->rlim, current->signal->rlim, sizeof sig->rlim);
 	task_unlock(current->group_leader);
 
 	posix_cpu_timers_init_group(sig);
 
-	acct_init_pacct(&sig->pacct);
-
 	tty_audit_fork(sig);
 
 	sig->oom_adj = current->signal->oom_adj;

Re: [PATCH v2 1/4] copy_signal cleanup: use zalloc and remove initializations

[Andrew Morton]

On Fri, 4 Dec 2009 15:28:14 +0100
Veaceslav Falico <vfalico@redhat.com> wrote:

> Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
> lines in copy_signal().
> 
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 166b8c4..160477d 100644
> @@ -855,7 +844,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)

For some reason this patch is missing its headers and doesn't apply.

I fixed that up and it still doesn't apply, because there are pending
changes in this area in -mm.

Normally I'd fix that up, but this one is looking a bit non-trivial, so
I'm afraid I'm going to ask you to redo the patches against
http://userweb.kernel.org/~akpm/mmotm/

Alternatively, we could just hold them off until 2.6.34.  Wait until
2.6.33-rc1 is released then resend the patch series based on
2.6.33-rc1.  Perhaps this is a better approach, as we are now in the
2.6.33 merge window and it's a bit late to be introducing new material.

Either way, please do ensure that the next patch series retains all the
acked-by:'s which people have sent. 

Thanks. 

Re: [PATCH v2 1/4] copy_signal cleanup: use zalloc and remove initializations

[Veaceslav Falico]

On Mon, Dec 07, 2009 at 12:36:15PM -0800, Andrew Morton wrote:
> 
> For some reason this patch is missing its headers and doesn't apply.
> 

I'm really sorry, but I don't really understand what is wrong with them
and why they don't apply :(. Could you please help me with that?

>
> I fixed that up and it still doesn't apply, because there are pending
> changes in this area in -mm.
> 
> Normally I'd fix that up, but this one is looking a bit non-trivial, so
> I'm afraid I'm going to ask you to redo the patches against
> http://userweb.kernel.org/~akpm/mmotm/
>

Thank you, will redo the patchset against -mm shortly.

--
Veaceslav 

[PATCH v3 0/4 -mmotm] copy_signal() cleanup

[Veaceslav Falico]

Use kmem_cache_zalloc() instead of kmem_cache_alloc() and remove all
unneeded initializations from copy_signal() and several other functions.
Should be applied on top of 0cf55e1ec08bb5a22e068309e2d8ba1180ab4239
commit.

 drivers/char/tty_audit.c       |    1 -
 include/linux/acct.h           |    2 --
 include/linux/sched.h          |    2 --
 include/linux/taskstats_kern.h |    7 -------
 kernel/acct.c                  |   10 ----------
 kernel/fork.c                  |   38 +-------------------------------------
 6 files changed, 1 insertions(+), 59 deletions(-)
---

[PATCH v3 1/4 -mmotm] copy_signal() cleanup: use zalloc and remove initializations

[Veaceslav Falico]

Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
lines in copy_signal().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
---
diff --git a/kernel/fork.c b/kernel/fork.c
index 404e6ca..a9252bf 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -859,7 +848,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	if (clone_flags & CLONE_THREAD)
 		return 0;
 
-	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
+	sig = kmem_cache_zalloc(signal_cachep, GFP_KERNEL);
 	tsk->signal = sig;
 	if (!sig)
 		return -ENOMEM;
@@ -867,46 +856,21 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	atomic_set(&sig->count, 1);
 	atomic_set(&sig->live, 1);
 	init_waitqueue_head(&sig->wait_chldexit);
-	sig->flags = 0;
 	if (clone_flags & CLONE_NEWPID)
 		sig->flags |= SIGNAL_UNKILLABLE;
-	sig->group_exit_code = 0;
-	sig->group_exit_task = NULL;
-	sig->group_stop_count = 0;
 	sig->curr_target = tsk;
 	init_sigpending(&sig->shared_pending);
 	INIT_LIST_HEAD(&sig->posix_timers);
 
 	hrtimer_init(&sig->real_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	sig->it_real_incr.tv64 = 0;
 	sig->real_timer.function = it_real_fn;
 
-	sig->leader = 0;	/* session leadership doesn't inherit */
-	sig->tty_old_pgrp = NULL;
-	sig->tty = NULL;
-
-	sig->utime = sig->stime = sig->cutime = sig->cstime = cputime_zero;
-	sig->gtime = cputime_zero;
-	sig->cgtime = cputime_zero;
-#ifndef CONFIG_VIRT_CPU_ACCOUNTING
-	sig->prev_utime = sig->prev_stime = cputime_zero;
-#endif
-	sig->nvcsw = sig->nivcsw = sig->cnvcsw = sig->cnivcsw = 0;
-	sig->min_flt = sig->maj_flt = sig->cmin_flt = sig->cmaj_flt = 0;
-	sig->inblock = sig->oublock = sig->cinblock = sig->coublock = 0;
-	sig->maxrss = sig->cmaxrss = 0;
-	task_io_accounting_init(&sig->ioac);
-	sig->sum_sched_runtime = 0;
-	taskstats_tgid_init(sig);
-
 	task_lock(current->group_leader);
 	memcpy(sig->rlim, current->signal->rlim, sizeof sig->rlim);
 	task_unlock(current->group_leader);
 
 	posix_cpu_timers_init_group(sig);
 
-	acct_init_pacct(&sig->pacct);
-
 	tty_audit_fork(sig);
 
 	sig->oom_adj = current->signal->oom_adj;

[PATCH v3 2/4 -mmotm] copy_signal() cleanup: kill taskstats_tgid_init() and acct_init_pacct()

[Veaceslav Falico]

Kill unused functions taskstats_tgid_init() and acct_init_pacct() because we
don't use them anywhere after using kmem_cache_zalloc() in copy_signal().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
diff --git a/include/linux/acct.h b/include/linux/acct.h
index 882dc72..93f4609 100644
--- a/include/linux/acct.h
+++ b/include/linux/acct.h
@@ -123,14 +123,12 @@ struct pacct_struct;
 struct pid_namespace;
 extern void acct_auto_close_mnt(struct vfsmount *m);
 extern void acct_auto_close(struct super_block *sb);
-extern void acct_init_pacct(struct pacct_struct *pacct);
 extern void acct_collect(long exitcode, int group_dead);
 extern void acct_process(void);
 extern void acct_exit_ns(struct pid_namespace *);
 #else
 #define acct_auto_close_mnt(x)	do { } while (0)
 #define acct_auto_close(x)	do { } while (0)
-#define acct_init_pacct(x)	do { } while (0)
 #define acct_collect(x,y)	do { } while (0)
 #define acct_process()		do { } while (0)
 #define acct_exit_ns(ns)	do { } while (0)
diff --git a/include/linux/taskstats_kern.h b/include/linux/taskstats_kern.h
index 3398f45..b6523c1 100644
--- a/include/linux/taskstats_kern.h
+++ b/include/linux/taskstats_kern.h
@@ -14,11 +14,6 @@
 extern struct kmem_cache *taskstats_cache;
 extern struct mutex taskstats_exit_mutex;
 
-static inline void taskstats_tgid_init(struct signal_struct *sig)
-{
-	sig->stats = NULL;
-}
-
 static inline void taskstats_tgid_free(struct signal_struct *sig)
 {
 	if (sig->stats)
@@ -30,8 +25,6 @@ extern void taskstats_init_early(void);
 #else
 static inline void taskstats_exit(struct task_struct *tsk, int group_dead)
 {}
-static inline void taskstats_tgid_init(struct signal_struct *sig)
-{}
 static inline void taskstats_tgid_free(struct signal_struct *sig)
 {}
 static inline void taskstats_init_early(void)
diff --git a/kernel/acct.c b/kernel/acct.c
index a6605ca..24f8c81 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -588,16 +588,6 @@ out:
 }
 
 /**
- * acct_init_pacct - initialize a new pacct_struct
- * @pacct: per-process accounting info struct to initialize
- */
-void acct_init_pacct(struct pacct_struct *pacct)
-{
-	memset(pacct, 0, sizeof(struct pacct_struct));
-	pacct->ac_utime = pacct->ac_stime = cputime_zero;
-}
-
-/**
  * acct_collect - collect accounting information into pacct_struct
  * @exitcode: task exit code
  * @group_dead: not 0, if this thread is the last one in the process.

[PATCH v3 3/4 -mmotm] copy_signal() cleanup: clean thread_group_cputime_init()

[Veaceslav Falico]

Remove unneeded initializations in thread_group_cputime_init() and
in posix_cpu_timers_init_group(). They are useless after 
kmem_cache_zalloc() was used in copy_signal().

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
---
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 23b26c7..c26bdb8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2434,9 +2434,7 @@ void thread_group_cputimer(struct task_struct *tsk, struct task_cputime *times);
 
 static inline void thread_group_cputime_init(struct signal_struct *sig)
 {
-	sig->cputimer.cputime = INIT_CPUTIME;
 	spin_lock_init(&sig->cputimer.lock);
-	sig->cputimer.running = 0;
 }
 
 static inline void thread_group_cputime_free(struct signal_struct *sig)
diff --git a/kernel/fork.c b/kernel/fork.c
index 404e6ca..a9252bf 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -829,17 +829,6 @@ static void posix_cpu_timers_init_group(struct signal_struct *sig)
 	/* Thread group counters. */
 	thread_group_cputime_init(sig);
 
-	/* Expiration times and increments. */
-	sig->it[CPUCLOCK_PROF].expires = cputime_zero;
-	sig->it[CPUCLOCK_PROF].incr = cputime_zero;
-	sig->it[CPUCLOCK_VIRT].expires = cputime_zero;
-	sig->it[CPUCLOCK_VIRT].incr = cputime_zero;
-
-	/* Cached expiration times. */
-	sig->cputime_expires.prof_exp = cputime_zero;
-	sig->cputime_expires.virt_exp = cputime_zero;
-	sig->cputime_expires.sched_exp = 0;
-
 	cpu_limit = ACCESS_ONCE(sig->rlim[RLIMIT_CPU].rlim_cur);
 	if (cpu_limit != RLIM_INFINITY) {
 		sig->cputime_expires.prof_exp = secs_to_cputime(cpu_limit);

[PATCH v3 4/4 -mmotm] copy_signal() cleanup: clean tty_audit_fork()

[Veaceslav Falico]

Remove unneeded initialization in tty_audit_fork().
It is called only via copy_signal() and is useless after
the kmem_cache_zalloc() was used.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
index ac16fbe..283a15b 100644
--- a/drivers/char/tty_audit.c
+++ b/drivers/char/tty_audit.c
@@ -148,7 +148,6 @@ void tty_audit_fork(struct signal_struct *sig)
 	spin_lock_irq(&current->sighand->siglock);
 	sig->audit_tty = current->signal->audit_tty;
 	spin_unlock_irq(&current->sighand->siglock);
-	sig->tty_audit_buf = NULL;
 }
 
 /**

Re: [PATCH v2 4/4] copy_signal cleanup: clean tty_audit_fork()

[Oleg Nesterov]

On 12/06, Miloslav Trmac wrote:
>
> ----- "Oleg Nesterov" <oleg@redhat.com> wrote:
> > On 12/05, Miloslav Trmac wrote:
> > > > Off-topic question to this who understands this code.
> > > >
> > > > But afaics we can also remove ->siglock from this helper and make
> > > > it really trivial for being inline. ->siglock buys nothing, we just
> > > > read a boolean. In fact, after the quick grep I do not understand
> > > > how ->siglock is connected to ->audit_tty. OK, it protects
> > > > tty_audit_buf,
> > > > but why we always take ->siglock to access ->audit_tty ?
> > > AFAIK there is no explicit documentation of the atomicity semantics
> > > expected by the Linux kernel (both from the hardware and from the compiler),
> > > so every access to the boolean is protected by a lock, to be on the safe side.
> >
> > Not sure I understand, but the kernel relies on fact it is always safe
> > to load/store a word.
> And is "word" an "unsigned", "unsigned long" or "intptr_t"?  Must it be
> suitably aligned, and if so, what is "suitably"?

Sure, it must be aligned.

> Where is this documented?

Perhaps nowhere, I do not know. If this is not documented, probably
it would be nice to add a note.

> > What atomicity semantics do you mean and how ->siglock can help?
> At the very least, "any access will read the last value stored and not result
> in undefined behavior, even if other threads attempt to access the value".
> In user-space, per POSIX, the only way to guarantee this is using explicit
> synchronization primitives.

We have numerous examples in kernel code which rely on this fact.

If we are talking about copy_process() pathes, please look at, say,
sched_fork(). Say, we read current->normal_prio lockless, while another
thread could change ->normal_prio in parallel.

Oleg.

Re: [PATCH v2 4/4] copy_signal cleanup: clean tty_audit_fork()

[Miloslav Trmac]

----- "Oleg Nesterov" <oleg@redhat.com> wrote:
> On 12/05, Miloslav Trmac wrote:
> > > Off-topic question to this who understands this code.
> > >
> > > But afaics we can also remove ->siglock from this helper and make
> > > it really trivial for being inline. ->siglock buys nothing, we just
> > > read a boolean. In fact, after the quick grep I do not understand
> > > how ->siglock is connected to ->audit_tty. OK, it protects
> > > tty_audit_buf,
> > > but why we always take ->siglock to access ->audit_tty ?
> > AFAIK there is no explicit documentation of the atomicity semantics
> > expected by the Linux kernel (both from the hardware and from the compiler),
> > so every access to the boolean is protected by a lock, to be on the safe side.
> 
> Not sure I understand, but the kernel relies on fact it is always safe
> to load/store a word.
And is "word" an "unsigned", "unsigned long" or "intptr_t"?  Must it be suitably aligned, and if so, what is "suitably"?  Where is this documented?  

> What atomicity semantics do you mean and how ->siglock can help?
At the very least, "any access will read the last value stored and not result in undefined behavior, even if other threads attempt to access the value".  In user-space, per POSIX, the only way to guarantee this is using explicit synchronization primitives.

> I believe every spin_lock(siglock) around ->audit_tty is bogus.
It might very well be.  Again, I just wanted to be sure, given my limited understanding of the Linux memory model assumptions.
    Mirek