linux-kernel - Re: [patch v2 4/4] taskstats: Export "cdata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101213143306.GA22840@redhat.com>
Date:	Mon, 13 Dec 2010 15:33:06 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Michael Holzheu <holzheu@...ux.vnet.ibm.com>
Cc:	Shailabh Nagar <nagar1234@...ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	John stultz <johnstul@...ibm.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Roland McGrath <roland@...hat.com>, Valdis.Kletnieks@...edu,
	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org
Subject: Re: [patch v2 4/4] taskstats: Export "cdata_wait" CPU times with
	taskstats

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/