lkml.org 
[lkml]   [2011]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [GIT PULL rcu/next] RCU commits for 3.1
From
On Mon, Nov 7, 2011 at 3:15 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> So far nobody seems to have stated if this is an actual problem or just
> shutting up lockdep-prove-rcu? I very much suspect the latter, in which
> case I really utterly hate the patch because it adds instructions to
> fast-paths just to kill a debug warning.
>
I think the core issue at stake here is not so much the cgroup disappearing.
It cannot go away because it is ref counted (perf_events does the necessary
css_get()/css_put()). But it is rather the task disappearing while we
are operating
on its state.

I don't think task (prev or next) can disappear while we execute
perf_cgroup_sched_out()/perf_cgroup_sched_in() because we are in the context
switch code.

What remains is:
* update_cgrp_time_from_event()
alway operates on current task

* perf_cgroup_set_timestamp()

- perf_event_task_tick() -> cpu_ctx_sched_in() but in this case
it is on the current task
- perf_event_task_sched_in() in context switch code so I assume
it is safe
- __perf_event_enable() but it is called on current

- perf_cgroup_switch()
* perf_cgroup_sched_in()/perf_cgroup_sched_out() -> context switch code

* perf_cgroup_attach()
called from cgroup code. Does not appear to hold task_lock().
the routine already grabs the rcu_read_lock() but it that enough
to guarantee the task cannot
vanish. I would hope so, otherwise I think the cgroup attach
code has a problem.


In summary, unless I am mistaken, it looks to me that we may not need
those new rcu_read_lock()
calls after all.

Does anyone have a different analysis?



> On Tue, 2011-11-01 at 10:37 +0800, Li Zefan wrote:
>>
>> 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.
>>
>> ---
>> 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
>>          */
>
> This looks like shutting things up, because what protects the use of
> cgrp after rcu_read_unlock() ?
>
> Similar to the below, this is a stupid patch to shut things up, no
> actual problem there, just making a hot path slow.
>
>> @@ -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();
>>  }
>
> This seems to actually protect the cgrp usage, but is that needed?
>
> It looks to be superfluous, since
> perf_cgroup_attach_task()->__perf_cgroup_move()->perf_cgroup_switch()
> will hold ctx->lock when it switches a task from one cgroup to another
> and perf_cgroup_set_timestamp() should only ever be called while holding
> the ctx->lock since that is what is used to serialize the timestamps.
>
>>  #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,
>
> This only hides a warning and leaves a race.
>
>> @@ -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
>>
>
> idem.
>
> So no, this patch utterly sucks, it adds code to hot paths just to quiet
> debug warnings in two cases and the remaining two cases annotates a
> warning away while leaving an actual problem unfixed.
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-11-07 17:19    [W:0.101 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site