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

[Stephane Eranian]

Hi,

Some second thoughts on this.

We get the warning because:

#define task_subsys_state_check(task, subsys_id, __c)                   \
        rcu_dereference_check(task->cgroups->subsys[subsys_id],         \
                              lockdep_is_held(&task->alloc_lock) ||     \
                              cgroup_lock_is_held() || (__c))


In other words, we need the alloc_lock held.

What I don't quite understand in your patch in the connection
between rcu and this particular task lock. Unless holding
the rcu read lock implies you necessarily hold the alloc_lock.

Can you explain?



On Fri, Nov 4, 2011 at 9:02 AM, Stephane Eranian <eranian@xxxxxxxxxx> wrote:
> On Fri, Nov 4, 2011 at 8:44 AM, Li Zefan <lizf@xxxxxxxxxxxxxx> wrote:
>> Stephane Eranian wrote:
>>> Paul,
>>>
>>> On Tue, Nov 1, 2011 at 2:37 AM, Li Zefan <lizf@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/