On 12/13, Michael Holzheu wrote:
>
> On Mon, 2010-12-13 at 14:05 +0100, Michael Holzheu wrote:
> > > And this looks racy, or I missed something again. group_dead can be
> > > true, but this doesn't mean all other threads have already passed
> > > taskstats_exit()->fill_tgid_exit()->delayacct_add_tsk().
> >
> > I think you are right.
> >
> > One way to fix that could be to separate the aggregation from the
> > sending. We could call fill_tgid_exit()->delayacct_add_tsk() before
> > atomic_dec_and_test(&tsk->signal->live) in do_exit() and
> > taskstats_exit() with the sender part afterwards.
Yes, I think this should fix the race. Some nits below...
> --- a/include/linux/taskstats_kern.h
> +++ b/include/linux/taskstats_kern.h
> @@ -21,7 +21,8 @@ static inline void taskstats_tgid_free(s
> kmem_cache_free(taskstats_cache, sig->stats);
> }
>
> -extern void taskstats_exit(struct task_struct *, int group_dead);
> +extern void taskstats_exit_notify(struct task_struct *, int group_dead);
> +extern void taskstats_exit_add_thread(struct task_struct *);
You forgot to update the !CONFIG_TASKSTATS case ;)
> -static void fill_tgid_exit(struct task_struct *tsk)
> +static void alloc_signal_stats(struct task_struct *tsk)
> +{
> + struct signal_struct *sig = tsk->signal;
> + struct taskstats *stats;
> +
> + /* No problem if kmem_cache_zalloc() fails */
> + stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
> +
> + spin_lock_irq(&tsk->sighand->siglock);
> + if (!sig->stats) {
> + sig->stats = stats;
> + stats = NULL;
> + }
> + spin_unlock_irq(&tsk->sighand->siglock);
> +
> + if (stats)
> + kmem_cache_free(taskstats_cache, stats);
> +}
> +
> +void taskstats_exit_add_thread(struct task_struct *tsk)
> {
> unsigned long flags;
>
> + if (tsk->signal->stats == NULL && !thread_group_empty(tsk))
> + alloc_signal_stats(tsk);
> +
> spin_lock_irqsave(&tsk->sighand->siglock, flags);
> if (!tsk->signal->stats)
> goto ret;
Well. I do not like the fact we take ->siglock unconditionally.
And _irqsave is not needed. And we take it twice if sig->stats == NULL.
And "if (!tsk->signal->stats)" under ->siglock in
taskstats_exit_add_thread() looks a bit ugly...
How about
void taskstats_exit_add_thread(struct task_struct *tsk)
{
struct taskstats *stats = NULL;
if (!tsk->signal->stats) {
if (thread_group_empty(tsk)
return;
stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
if (!stats)
return;
}
spin_lock_irq(&tsk->sighand->siglock);
if (!tsk->signal->stats) {
tsk->signal->stats = stats;
stats = NULL;
}
/*
* Each accounting subsystem calls its functions here to
* accumalate its per-task stats for tsk, into the per-tgid structure
*
* per-task-foo(tsk->signal->stats, tsk);
*/
delayacct_add_tsk(tsk->signal->stats, tsk);
spin_unlock_irq(&tsk->sighand->siglock);
if (unlikely(stats))
kmem_cache_free(taskstats_cache, stats);
}
?
Note that it absorbs alloc_signal_stats().
But up to you, probably this is matter of taste.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
