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

[Stephane Eranian]

Hi,

Sorry for the delay, I was on vacation and travelling.
I will look at your proposed patch tomorrow. I ran into
this warning myself recently. There is a cgroup dependency
of task_lock(). On context switch, we do need to check
the outgoing and incoming cgroups to check if we need to
stop/start cgroup events in perf_events.



On Wed, Nov 2, 2011 at 7:23 PM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Nov 01, 2011 at 10:37:28AM +0800, Li Zefan 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?
>>
>> - 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.
>
> Looks sane to me, for whatever that might be worth.
>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂThanx, Paul
>
>> ---
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index d1a1bee..f5e05ce 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -302,7 +302,10 @@ static inline void update_cgrp_time_from_event(struct perf_event *event)
>> Â Â Â if (!is_cgroup_event(event))
>> Â Â Â Â Â Â Â return;
>>
>> + Â Â rcu_read_lock();
>> Â Â Â cgrp = perf_cgroup_from_task(current);
>> + Â Â rcu_read_unlock();
>> +
>> Â Â Â /*
>> Â Â Â Â* Do not update time when cgroup is not active
>> Â Â Â Â*/
>> @@ -325,9 +328,11 @@ perf_cgroup_set_timestamp(struct task_struct *task,
>> Â Â Â if (!task || !ctx->nr_cgroups)
>> Â Â Â Â Â Â Â return;
>>
>> + Â Â rcu_read_lock();
>> Â Â Â cgrp = perf_cgroup_from_task(task);
>> Â Â Â info = this_cpu_ptr(cgrp->info);
>> Â Â Â info->timestamp = ctx->timestamp;
>> + Â Â rcu_read_unlock();
>> Â}
>>
>> Â#define PERF_CGROUP_SWOUT Â Â0x1 /* cgroup switch out every event */
>> @@ -406,6 +411,8 @@ static inline void perf_cgroup_sched_out(struct task_struct *task,
>> Â Â Â struct perf_cgroup *cgrp1;
>> Â Â Â struct perf_cgroup *cgrp2 = NULL;
>>
>> + Â Â rcu_read_lock();
>> +
>> Â Â Â /*
>> Â Â Â Â* we come here when we know perf_cgroup_events > 0
>> Â Â Â Â*/
>> @@ -418,6 +425,8 @@ static inline void perf_cgroup_sched_out(struct task_struct *task,
>> Â Â Â if (next)
>> Â Â Â Â Â Â Â cgrp2 = perf_cgroup_from_task(next);
>>
>> + Â Â rcu_read_unlock();
>> +
>> Â Â Â /*
>> Â Â Â Â* only schedule out current cgroup events if we know
>> Â Â Â Â* that we are switching to a different cgroup. Otherwise,
>> @@ -433,6 +442,8 @@ static inline void perf_cgroup_sched_in(struct task_struct *prev,
>> Â Â Â struct perf_cgroup *cgrp1;
>> Â Â Â struct perf_cgroup *cgrp2 = NULL;
>>
>> + Â Â rcu_read_lock();
>> +
>> Â Â Â /*
>> Â Â Â Â* we come here when we know perf_cgroup_events > 0
>> Â Â Â Â*/
>> @@ -441,6 +452,8 @@ static inline void perf_cgroup_sched_in(struct task_struct *prev,
>> Â Â Â /* prev can never be NULL */
>> Â Â Â cgrp2 = perf_cgroup_from_task(prev);
>>
>> + Â Â rcu_read_unlock();
>> +
>> Â Â Â /*
>> Â Â Â Â* only need to schedule in cgroup events if we are changing
>> Â Â Â Â* cgroup during ctxsw. Cgroup events were not scheduled
>>
>
>
--
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/