Re: [RFC][2.6.8-rc1-mm1] perfctr inheritance locking issue

[Manfred Spraul]

Andrew Morton wrote:

> Mikael Pettersson <mikpe@xxxxxxxxx> wrote:
>  
>
> > Andrew,
> >
> > There is another locking problem with the per-process
> > performance counter inheritance changes I sent you.
> >
> > I currently use task_lock(tsk) to synchronise accesses
> > to tsk->thread.perfctr, when that pointer could change.
> >
> > The write_lock_irq(&tasklist_lock) in release_task() is
> > needed to prevent ->parent from changing while releasing the
> > child, but the parent's ->thread.perfctr must also be locked.
> > However, sched.h explicitly forbids holding task_lock()
> > simultaneously with write_lock_irq(&tasklist_lock). Ouch.
> >    
>
> That's ghastly.
>
> * Nests both inside and outside of read_lock(&tasklist_lock).
> * It must not be nested with write_lock_irq(&tasklist_lock),
> * neither inside nor outside.
>
> Manfred, where did you discover the offending code?
>
>  
Think about interrupts: they are permitted to acquire the tasklist_lock 
for read.

Someone does
   read_lock(&tasklist_lock);
   task_lock(tsk);

One example is __do_SAK in tty_io.c, but I think there are further examples.

Now add a softirq that tries to deliver a signal: kill_something_info() 
contains a
   read_lock(&tasklist_lock);

This sequence doesn't deadlock - rw spinlocks starve writers.
But it means that both
   task_lock();
   write_lock_irq(&tasklist_lock);
and
   write_lock_irq(&tasklist_lock);
   task_lock();

can deadlock with the read_lock()/task_lock()/read_lock() sequence.

> Would be better to just sort out the locking, then take task_lock() inside
> tasklist_lock.  That was allegedly the rule in 2.4.
>  
It probably works by chance in 2.4.

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