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