Re: [GIT PULL rcu/next] RCU commits for 3.1

[Li Zefan <lizf@cn.fujitsu.com>]

Stephane Eranian wrote:
> Paul,
> 
> On Tue, Nov 1, 2011 at 2:37 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> (I shoud have cced Stephane Eranian instead of Turner..)
>>
>> Paul E. McKenney wrote:
>>> On Mon, Oct 31, 2011 at 04:09:19PM +0800, Li Zefan wrote:
>>>> (Let's cc Peter and Paul Turner for this perf cgroup issue.)
>>>>
>>>>> Thank you for the analysis.  Does the following patch fix this problem?
>>>>>
>>>>>                                                     Thanx, Paul
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>> fs: Add RCU protection in set_task_comm()
>>>>>
>>>>> Running "perf stat true" results in the following RCU-lockdep splat:
>>>>>
>>>>> ===============================
>>>>> [ INFO: suspicious RCU usage. ]
>>>>> -------------------------------
>>>>> include/linux/cgroup.h:548 suspicious rcu_dereference_check() usage!
>>>>>
>>>>> other info that might help us debug this:
>>>>>
>>>>> rcu_scheduler_active = 1, debug_locks = 0
>>>>> 1 lock held by true/655:
>>>>> #0:  (&sig->cred_guard_mutex){+.+.+.}, at: [<810d1bd7>] prepare_bprm_creds+0x27/0x70
>>>>>
>>>>> stack backtrace:
>>>>> Pid: 655, comm: true Not tainted 3.1.0-tip-01868-g1271bd2-dirty #161079
>>>>> Call Trace:
>>>>> [<81abe239>] ? printk+0x18/0x1a
>>>>> [<81064920>] lockdep_rcu_suspicious+0xc0/0xd0
>>>>> [<8108aa02>] perf_event_enable_on_exec+0x1d2/0x1e0
>>>>> [<81063764>] ? __lock_release+0x54/0xb0
>>>>> [<8108cca8>] perf_event_comm+0x18/0x60
>>>>> [<810d1abd>] ? set_task_comm+0x5d/0x80
>>>>> [<81af622d>] ? _raw_spin_unlock+0x1d/0x40
>>>>> [<810d1ac4>] set_task_comm+0x64/0x80
>>>>> [<810d25fd>] setup_new_exec+0xbd/0x1d0
>>>>> [<810d1b61>] ? flush_old_exec+0x81/0xa0
>>>>> [<8110753e>] load_elf_binary+0x28e/0xa00
>>>>> [<810d2101>] ? search_binary_handler+0xd1/0x1d0
>>>>> [<81063764>] ? __lock_release+0x54/0xb0
>>>>> [<811072b0>] ? load_elf_library+0x260/0x260
>>>>> [<810d2108>] search_binary_handler+0xd8/0x1d0
>>>>> [<810d2060>] ? search_binary_handler+0x30/0x1d0
>>>>> [<810d242f>] do_execve_common+0x22f/0x2a0
>>>>> [<810d24b2>] do_execve+0x12/0x20
>>>>> [<81009592>] sys_execve+0x32/0x70
>>>>> [<81af7752>] ptregs_execve+0x12/0x20
>>>>> [<81af76d4>] ? sysenter_do_call+0x12/0x36
>>>>>
>>>>> Li Zefan noted that this is due to set_task_comm() dropping the task
>>>>> lock before invoking perf_event_comm(), which could in fact result in
>>>>> the task being freed up before perf_event_comm() completed tracing in
>>>>> the case where one task invokes set_task_comm() on another task -- which
>>>>> actually does occur via comm_write(), which can be invoked via /proc.
>>>>>
>>>>
>>>> This is not true. The caller should ensure @tsk is valid during
>>>> set_task_comm().
>>>>
>>>> The warning comes from perf_cgroup_from_task(). We can trigger this warning
>>>> in some other cases where perf cgroup is used, for example:
>>>
>>> I must defer to your greater knowledge of this situation.  What patch
>>> would you propose?
>>>
>>
>> With the following patch, we should see no rcu warning from perf, but as I
>> don't know the internel of perf, I guess we have to defer to Peter and
>> Stephane. ;)
>>
>> I have two doubts:
>>
>> - in perf_cgroup_sched_out/in(), we retrieve the task's cgroup twice in the function
>> and it's callee perf_cgroup_switch(), but the task can move to another cgroup between
>> two calls, so they might return two different cgroup pointers. Does it matter?
>>
> We don't retrieve the task cgroup twice. We retrieve the cgroup for
> each of the two
> tasks: current and prev or next.
> 
> I don't understand what you mean by 'between two calls'. Two calls of
> which function?
> 

perf_cgroup_sched_out(task, next)
{
	cgrp1 = perf_cgroup_from_task(task);
	...
	perf_cgroup_switch(task, PERF_CGROUP_SWOUT);
}

perf_cgroup_switch(task)
{
	...
	cpuctx->cgrp = perf_cgroup_from_task(task);
}

So we call perf_cgroup_from_task() twice on @task. Just want to be sure the code
is not problematic.

>> - in perf_cgroup_switch():
>>
>>         cpuctx->cgrp = perf_cgroup_from_task(task);
>>
>> but seems the cgroup is not pinned, so cpuctx->cgrp can be invalid in later use.
>>
> What do you mean by cgroup pinning?
> 
> If a task migrates from one cgroup to another, the cgroup code calls
> ss->attach_task
> which ends up in perf_cgroup_attach_task() if the task is currently
> running on a CPU.
> If so perf_cgroup_switch() is eventually called and it will update
> cpuctx->cgrp. If the
> tasks is not running anywhere, then there is nothing to do, state will
> be updated when
> the task is scheduled back in.
> 

Thanks for clarification!