Re: + delay-accounting-taskstats-interface-send-tgid-once.patch added to -mm tree

[Arjan van de Ven <arjan@infradead.org>]

also in response to
                           Subject: 
Re: 2.6.17-mm3
                              Date: 
Tue, 27 Jun 2006 16:12:42 +0200
with Message-ID:
<6bffcb0e0606270712w166f04a6u237d695e2bfa1913@mail.gmail.com>

On Mon, 2006-06-26 at 12:06 -0700, akpm@osdl.org wrote:

> +static inline void taskstats_tgid_alloc(struct signal_struct *sig)
> +{
> +	struct taskstats *stats;
> +
> +	stats = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL);
> +	if (!stats)
> +		return;
> +
> +	spin_lock(&sig->stats_lock);
> +	if (!sig->stats) {

+static inline void taskstats_tgid_free(struct signal_struct *sig)
+{
+       struct taskstats *stats = NULL;
+       spin_lock(&sig->stats_lock);
+       if (sig->stats) {
+               stats = sig->stats;
+               sig->stats = NULL;
+       }
+       spin_unlock(&sig->stats_lock);
+       if (stats)
+               kmem_cache_free(taskstats_cache, stats);
+}

this is buggy and deadlock prone!
(found by lockdep)

stats_lock nests within tasklist_lock, which is taken in irq context.
(and thus needs irq_save treatment). But because of this nesting, it is
not allowed to take stats_lock without disabling irqs, or you can have
the following scenario

CPU 0                		CPU 1
(in irq)            	 	(in the code above)
		     		stats_lock is taken
tasklist_lock is taken	     	
stats_lock_is taken <spin>	
				<interrupt happens>
		     		tasklist_lock is taken
		     
which now forms an AB-BA deadlock!


this happens at least in copy_process which can call taskstats_tgid_free
without first disabling interrupts (via cleanup_signal). There may be
many other cases, I've not checked deeper yet.

Solution should be to make these functions use irqsave variant... any
comments from the authors of this patch ?