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

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

[Mikael Pettersson]

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.

My options seem to boil down to one of the following:
1. Forget task_lock(), always take the tasklist_lock.
   This should work but would lock the task list briefly at
   operations like set_cpus_allowed(), and creating/deleting
   a task's perfctr state object. I don't like that.
2. Add a 'spinlock_t perfctr_lock;' to the thread_struct,
   next to the perfctr state pointer. This is much cleaner,
   but increases the size of the thread struct slightly.

I think I prefer option #2. Any objections to that?

/Mikael

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

[Andrew Morton]

Mikael Pettersson <mikpe@csd.uu.se> 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?

> My options seem to boil down to one of the following:
> 1. Forget task_lock(), always take the tasklist_lock.
>    This should work but would lock the task list briefly at
>    operations like set_cpus_allowed(), and creating/deleting
>    a task's perfctr state object. I don't like that.
> 2. Add a 'spinlock_t perfctr_lock;' to the thread_struct,
>    next to the perfctr state pointer. This is much cleaner,
>    but increases the size of the thread struct slightly.
> 
> I think I prefer option #2. Any objections to that?

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

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

[Manfred Spraul]

Andrew Morton wrote:

>Mikael Pettersson <mikpe@csd.uu.se> 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