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

[Shailabh Nagar <nagar@watson.ibm.com>]

Arjan van de Ven wrote:

> 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). 

Arjan,

Didn't get how stats_lock is nesting within tasklist_lock in copy_process ?
The call to cleanup_signal (or any call to taskstats_tgid_alloc/free) 
seems to be happening
outside of holding the tasklist lock ? Or maybe I'm missing something.

Changing to an irqsave variant isn't a problem of course...

--Shailabh

> 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 ?
>
>
>  

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/