Message-ID: <CABPqkBR=dB3E1ba-cR7L5A2uknidBJTMdMBNFzyr+zsmjuxacw@mail.gmail.com>
Date:	Fri, 4 Nov 2011 09:02:17 +0000
From:	Stephane Eranian <eranian@...gle.com>
To:	Li Zefan <lizf@...fujitsu.com>
Cc:	paulmck@...ux.vnet.ibm.com, Ingo Molnar <mingo@...e.hu>,
	eric.dumazet@...il.com, shaohua.li@...el.com, ak@...ux.intel.com,
	mhocko@...e.cz, alex.shi@...el.com, efault@....de,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Paul Turner <pjt@...gle.com>
Subject: Re: [GIT PULL rcu/next] RCU commits for 3.1

On Fri, Nov 4, 2011 at 8:44 AM, Li Zefan <lizf@...fujitsu.com> wrote:
> Stephane Eranian wrote:
>> Paul,
>>
>> On Tue, Nov 1, 2011 at 2:37 AM, Li Zefan <lizf@...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);
> }
>
Ok, yes it may happen that we call it twice.

I tested your patch and it looks good to me. I would make the
following adjustments though:
- perf_cgroup_set_timestamp(), move rcu_read_unlock() before
info->timestamp = as it is not needed
  for this statement.


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

Powered by blists - more mailing lists