Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
lines.
Signed-off-by: Veaceslav Falico <[email protected]>
---
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);
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 <[email protected]>
> ---
>
> 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.
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
Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
lines in copy_signal().
Signed-off-by: Veaceslav Falico <[email protected]>
---
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);
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 <[email protected]>
---
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;
}
/**
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 <[email protected]>
---
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)
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 <[email protected]>
---
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(¤t->sighand->siglock);
sig->audit_tty = current->signal->audit_tty;
spin_unlock_irq(¤t->sighand->siglock);
- sig->tty_audit_buf = NULL;
}
/**
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 <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
> ---
>
> 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.
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 <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
> ---
>
> 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(¤t->sighand->siglock);
> sig->audit_tty = current->signal->audit_tty;
> spin_unlock_irq(¤t->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.
----- "Oleg Nesterov" <[email protected]> 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(¤t->sighand->siglock);
> > sig->audit_tty = current->signal->audit_tty;
> > spin_unlock_irq(¤t->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
On 12/05, Miloslav Trmac wrote:
>
> ----- "Oleg Nesterov" <[email protected]> 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(¤t->sighand->siglock);
> > > sig->audit_tty = current->signal->audit_tty;
> > > spin_unlock_irq(¤t->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.
----- "Oleg Nesterov" <[email protected]> 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
* Veaceslav Falico <[email protected]> [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 <[email protected]>
Good cleanup, thanks!
--
Balbir
* Veaceslav Falico <[email protected]> [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 <[email protected]>
As Oleg suggested, please reorder the cleanup to remove the empty
functions.
--
Balbir
On 12/06, Miloslav Trmac wrote:
>
> ----- "Oleg Nesterov" <[email protected]> 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.
(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 <[email protected]>
---
--- 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;
On Fri, 4 Dec 2009 15:28:14 +0100
Veaceslav Falico <[email protected]> wrote:
> Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
> lines in copy_signal().
>
> Signed-off-by: Veaceslav Falico <[email protected]>
> ---
>
> 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.
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
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(-)
---
Use kmem_cache_zalloc() on signal creation and remove unneeded initialization
lines in copy_signal().
Signed-off-by: Veaceslav Falico <[email protected]>
Acked-by: Oleg Nesterov <[email protected]>
---
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;
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 <[email protected]>
---
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.
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 <[email protected]>
Acked-by: Oleg Nesterov <[email protected]>
---
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);
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 <[email protected]>
---
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(¤t->sighand->siglock);
sig->audit_tty = current->signal->audit_tty;
spin_unlock_irq(¤t->sighand->siglock);
- sig->tty_audit_buf = NULL;
}
/**