All of lore.kernel.org
 help / color / mirror / Atom feed
From: Li Zefan <lizf@cn.fujitsu.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Miles Lane <miles.lane@gmail.com>,
	paulmck@linux.vnet.ibm.com, Vivek Goyal <vgoyal@redhat.com>,
	Eric Paris <eparis@redhat.com>,
	Lai Jiangshan <laijs@cn.fujitsu.com>, Ingo Molnar <mingo@elte.hu>,
	LKML <linux-kernel@vger.kernel.org>,
	nauman@google.com, eric.dumazet@gmail.com,
	netdev@vger.kernel.org, Jens Axboe <jens.axboe@oracle.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Paul Menage <menage@google.com>
Subject: Re: [PATCH] sched: silence PROVE_RCU in sched_fork()
Date: Wed, 23 Jun 2010 15:25:50 +0800	[thread overview]
Message-ID: <4C21B6FE.8060801@cn.fujitsu.com> (raw)
In-Reply-To: <1277199893.1875.690.camel@laptop>

Peter Zijlstra wrote:
> Paul Menage, Li Zefan, any comments?
> 
> 
> ---
> Because cgroup_fork() is ran before sched_fork() [ from copy_process() ]
> and the child's pid is not yet visible the child is pinned to its
> cgroup. Therefore we can silence this warning.
> 

The explanation is correct.

We silenced another warning according to the same reason.

See freezer_fork() in kernel/cgroup_freezer.c

> A nicer solution would be moving cgroup_fork() to right after
> dup_task_struct() and exclude PF_STARTING from task_subsys_state().
> 

Seems PF_STARTING is set in copy_flags(), that's after dup_task_struct() and
before cgroup_fork(). But it's cleared after copy_process(), that's after
the task is linked into tasklist, so this doesn't seem work...

> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

For this patch:

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

> ---
>  kernel/sched.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index b697606..2e79518 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -2561,7 +2561,16 @@ void sched_fork(struct task_struct *p, int clone_flags)
>  	if (p->sched_class->task_fork)
>  		p->sched_class->task_fork(p);
>  
> +	/*
> +	 * The child is not yet in the pid-hash so no cgroup attach races,
> +	 * and the cgroup is pinned to this child due to cgroup_fork()
> +	 * is ran before sched_fork().
> +	 *
> +	 * Silence PROVE_RCU.
> +	 */
> +	rcu_read_lock();
>  	set_task_cpu(p, cpu);
> +	rcu_read_unlock();
>  
>  #if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
>  	if (likely(sched_info_on()))
> 
> 
> 

WARNING: multiple messages have this Message-ID (diff)
From: Li Zefan <lizf@cn.fujitsu.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Miles Lane <miles.lane@gmail.com>,
	paulmck@linux.vnet.ibm.com, Vivek Goyal <vgoyal@redhat.com>,
	Eric Paris <eparis@redhat.com>,
	Lai Jiangshan <laijs@cn.fujitsu.com>, Ingo Molnar <mingo@elte.hu>,
	LKML <linux-kernel@vger.kernel.org>,
	nauman@google.com, eric.dumazet@gmail.com,
	netdev@vger.kernel.org, Jens Axboe <jens.axboe@oracle.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Paul Menage <menage@google.com>
Subject: Re: [PATCH] sched: silence PROVE_RCU in sched_fork()
Date: Wed, 23 Jun 2010 15:25:50 +0800	[thread overview]
Message-ID: <4C21B6FE.8060801@cn.fujitsu.com> (raw)
In-Reply-To: <1277199893.1875.690.camel@laptop>

Peter Zijlstra wrote:
> Paul Menage, Li Zefan, any comments?
> 
> 
> ---
> Because cgroup_fork() is ran before sched_fork() [ from copy_process() ]
> and the child's pid is not yet visible the child is pinned to its
> cgroup. Therefore we can silence this warning.
> 

The explanation is correct.

We silenced another warning according to the same reason.

See freezer_fork() in kernel/cgroup_freezer.c

> A nicer solution would be moving cgroup_fork() to right after
> dup_task_struct() and exclude PF_STARTING from task_subsys_state().
> 

Seems PF_STARTING is set in copy_flags(), that's after dup_task_struct() and
before cgroup_fork(). But it's cleared after copy_process(), that's after
the task is linked into tasklist, so this doesn't seem work...

> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

For this patch:

Reviewed-by: Li Zefan <lizf@cn.fujitsu.com>

> ---
>  kernel/sched.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index b697606..2e79518 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -2561,7 +2561,16 @@ void sched_fork(struct task_struct *p, int clone_flags)
>  	if (p->sched_class->task_fork)
>  		p->sched_class->task_fork(p);
>  
> +	/*
> +	 * The child is not yet in the pid-hash so no cgroup attach races,
> +	 * and the cgroup is pinned to this child due to cgroup_fork()
> +	 * is ran before sched_fork().
> +	 *
> +	 * Silence PROVE_RCU.
> +	 */
> +	rcu_read_lock();
>  	set_task_cpu(p, cpu);
> +	rcu_read_unlock();
>  
>  #if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
>  	if (likely(sched_info_on()))
> 
> 
> 

  reply	other threads:[~2010-06-23  7:22 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-07 18:14 2.6.35-rc2-git1 - include/linux/cgroup.h:534 invoked rcu_dereference_check() without protection! Miles Lane
2010-06-07 18:14 ` Miles Lane
2010-06-08  0:19 ` Paul E. McKenney
2010-06-08  4:16   ` Miles Lane
2010-06-08  4:16     ` Miles Lane
2010-06-08  8:40     ` Peter Zijlstra
2010-06-08  8:40       ` Peter Zijlstra
2010-06-08 13:14       ` Miles Lane
2010-06-08 13:14         ` Miles Lane
2010-06-08 13:34         ` Peter Zijlstra
2010-06-08 13:34           ` Peter Zijlstra
2010-06-09 15:11           ` Miles Lane
2010-06-09 15:11             ` Miles Lane
2010-06-09 15:24             ` Peter Zijlstra
2010-06-09 15:24               ` Peter Zijlstra
2010-06-09 15:29               ` Miles Lane
2010-06-09 15:29                 ` Miles Lane
2010-06-09 17:00                 ` Miles Lane
2010-06-09 17:00                   ` Miles Lane
2010-06-09 17:57                   ` Peter Zijlstra
2010-06-09 17:57                     ` Peter Zijlstra
2010-06-09 18:15                     ` Peter Zijlstra
2010-06-09 18:15                       ` Peter Zijlstra
2010-06-09 21:15                       ` Miles Lane
2010-06-09 21:15                         ` Miles Lane
2010-06-09 21:20                         ` Peter Zijlstra
2010-06-09 21:20                           ` Peter Zijlstra
     [not found]                           ` <AANLkTilsDv59zbL7rbAgJByz_vSnZ0QlbK3dVJUX2VYQ@mail.gmail.com>
2010-06-22  9:44                             ` [PATCH] sched: silence PROVE_RCU in sched_fork() Peter Zijlstra
2010-06-23  7:25                               ` Li Zefan [this message]
2010-06-23  7:25                                 ` Li Zefan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C21B6FE.8060801@cn.fujitsu.com \
    --to=lizf@cn.fujitsu.com \
    --cc=eparis@redhat.com \
    --cc=eric.dumazet@gmail.com \
    --cc=guijianfeng@cn.fujitsu.com \
    --cc=jens.axboe@oracle.com \
    --cc=johannes@sipsolutions.net \
    --cc=laijs@cn.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=menage@google.com \
    --cc=miles.lane@gmail.com \
    --cc=mingo@elte.hu \
    --cc=nauman@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=vgoyal@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.