lkml.org 
[lkml]   [2011]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [GIT PULL rcu/next] RCU commits for 3.1
From
Date
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.

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.




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