Prev: kdump: extract log buffer and registers from vmcore on NMI button pressing
Next: [PATCH] ppp_generic: fix multilink fragment sizes
From: Li Zefan on 3 Jun 2010 05:20
Paul E. McKenney wrote:
> On Tue, Jun 01, 2010 at 02:06:13PM +0100, Daniel J Blueman wrote:
>> Hi Paul,
>>
>> With 2.6.35-rc1 and your patch in the context below, we still see
>> "include/linux/cgroup.h:534 invoked rcu_dereference_check() without
>> protection!", so need this additional patch:
>>
>> Acquire read-side RCU lock around task_group() calls, addressing
>> "include/linux/cgroup.h:534 invoked rcu_dereference_check() without
>> protection!" warning.
>>
>> Signed-off-by: Daniel J Blueman <daniel.blueman(a)gmail.com>
>
> Thank you, Daniel! I have queued this for 2.6.35.
>
> I had to apply the patch by hand due to line wrapping. Could you please
> check your email-agent settings? This simple patch was no problem to
> hand apply, but for a larger patch this process would be both tedious
> and error prone.
>
> Thanx, Paul
>
>> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
>> index 217e4a9..50ec9ea 100644
>> --- a/kernel/sched_fair.c
>> +++ b/kernel/sched_fair.c
>> @@ -1241,6 +1241,7 @@ static int wake_affine(struct sched_domain *sd,
>> struct task_struct *p, int sync)
>> * effect of the currently running task from the load
>> * of the current CPU:
>> */
>> + rcu_read_lock();
>> if (sync) {
>> tg = task_group(current);
>> weight = current->se.load.weight;
>> @@ -1250,6 +1251,7 @@ static int wake_affine(struct sched_domain *sd,
>> struct task_struct *p, int sync)
>> }
>>
>> tg = task_group(p);
>> + rcu_read_unlock();

Hmmm.. I think it's not safe to access tg after rcu_read_unlock.

>> weight = p->se.load.weight;
>>
>> imbalance = 100 + (sd->imbalance_pct - 100) / 2;
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Li Zefan on 3 Jun 2010 22:50
> Seems worth reviewing the other uses of task_group():
>
> 1. set_task_rq() -- only a runqueue and a sched_rt_entity leave
> the RCU read-side critical section. Runqueues do persist.
> I don't claim to understand the sched_rt_entity life cycle.
>
> 2. __sched_setscheduler() -- not clear to me that this one is
> protected to begin with. If it is somehow correctly protected,
> it discards the RCU-protected pointer immediately, so is OK
> otherwise.
>
> 3. cpu_cgroup_destroy() -- ditto.
>
> 4. cpu_shares_read_u64() -- ditto.
>
> 5. print_task() -- protected by rcu_read_lock() and discards the
> RCU-protected pointer immediately, so this one is OK.
>
> Any task_group() experts able to weigh in on #2, #3, and #4?
>

#3 and #4 are safe, because it's not calling task_group(), but
cgroup_tg():

struct task_group *tg = cgroup_tg(cgrp);

As long as it's safe to access cgrp, it's safe to access tg.

> Thanx, Paul
>
> Signed-off-by: Paul E. McKenney <paulmck(a)linux.vnet.ibm.com>
>
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> index 50ec9ea..224ef98 100644
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -1251,7 +1251,6 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
> }
>
> tg = task_group(p);
> - rcu_read_unlock();
> weight = p->se.load.weight;
>
> imbalance = 100 + (sd->imbalance_pct - 100) / 2;
> @@ -1268,6 +1267,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
> balanced = !this_load ||
> 100*(this_load + effective_load(tg, this_cpu, weight, weight)) <=
> imbalance*(load + effective_load(tg, prev_cpu, 0, weight));
> + rcu_read_unlock();
>

This is fine.

Another way is :

rcu_read_lock();
tg = task_group(p);
css_get(&tg->css);
rcu_read_unlock();

/* do something */
....

css_put(&tg->css);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: kdump: extract log buffer and registers from vmcore on NMI button pressing
Next: [PATCH] ppp_generic: fix multilink fragment sizes