Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640627 > unrolled thread

[RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest

Started bySteven Rostedt <rostedt@goodmis.org>
First post2017-05-12 19:30 +0200
Last post2017-05-13 02:30 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest Steven Rostedt <rostedt@goodmis.org> - 2017-05-12 19:30 +0200
    Re: [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-12 20:40 +0200
      Re: [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest Steven Rostedt <rostedt@goodmis.org> - 2017-05-12 20:50 +0200
        Re: [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-12 21:00 +0200
    Re: [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest Thomas Gleixner <tglx@linutronix.de> - 2017-05-13 00:20 +0200
      Re: [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest Steven Rostedt <rostedt@goodmis.org> - 2017-05-13 02:30 +0200

#1640627 — [RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-12 19:30 +0200
Subject[RFC][PATCH 2/5] cpu-hotplug: Allow get_online_cpus() to nest
Message-ID<tGmy5-7Uc-1@gated-at.bofh.it>
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Allow get_online_cpus() to be recursive. If a lock is taken while under
"get_online_cpus()", it can call get_online_cpus() as well, just as long as
it is never held without being under get_online_cpus(), but then calling it.

   GOC() -> Lock(X) -> GOC()

is OK, as long as

   Lock(X) -> GOC()

does not exist.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/sched.h | 1 +
 kernel/cpu.c          | 9 +++++++++
 kernel/fork.c         | 1 +
 3 files changed, 11 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 993e7e25a3a5..8f272ab57685 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -813,6 +813,7 @@ struct task_struct {
 	unsigned int			lockdep_recursion;
 	struct held_lock		held_locks[MAX_LOCK_DEPTH];
 	gfp_t				lockdep_reclaim_gfp;
+	int				goc_depth;
 #endif
 
 #ifdef CONFIG_UBSAN
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 983163ef36ee..0dbdf1e69715 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -209,12 +209,21 @@ DEFINE_STATIC_PERCPU_RWSEM(cpu_hotplug_lock);
 
 void get_online_cpus(void)
 {
+#ifdef CONFIG_LOCKDEP
+	if (current->goc_depth++)
+		return;
+#endif
 	percpu_down_read(&cpu_hotplug_lock);
 }
 EXPORT_SYMBOL_GPL(get_online_cpus);
 
 void put_online_cpus(void)
 {
+#ifdef CONFIG_LOCKDEP
+	WARN_ON_ONCE(current->goc_depth < 1);
+	if (--current->goc_depth)
+		return;
+#endif
 	percpu_up_read(&cpu_hotplug_lock);
 }
 EXPORT_SYMBOL_GPL(put_online_cpus);
diff --git a/kernel/fork.c b/kernel/fork.c
index dd5a371c392a..8aedcd011ccc 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1658,6 +1658,7 @@ static __latent_entropy struct task_struct *copy_process(
 	p->lockdep_depth = 0; /* no locks held yet */
 	p->curr_chain_key = 0;
 	p->lockdep_recursion = 0;
+	p->goc_depth = 0;
 #endif
 
 #ifdef CONFIG_DEBUG_MUTEXES
-- 
2.10.2

[toc] | [next] | [standalone]

#1640680

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-05-12 20:40 +0200
Message-ID<tGnDQ-cz-19@gated-at.bofh.it>
In reply to#1640627
On Fri, May 12, 2017 at 01:15:46PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Allow get_online_cpus() to be recursive. If a lock is taken while under
> "get_online_cpus()", it can call get_online_cpus() as well, just as long as
> it is never held without being under get_online_cpus(), but then calling it.
> 
>    GOC() -> Lock(X) -> GOC()
> 
> is OK, as long as
> 
>    Lock(X) -> GOC()
> 
> does not exist.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Does ->goc_depth also need to be initialized in include/linux/init_task.h?

It seems like C-language initialization-to-zero would cover this,
but there is a lot of initialization to zero values in init_task.h
(including but not limited to some RCU stuff).

Other than that, this does look like it might ease use of get_online_cpus()
in combination with CPU-hotplug notifiers, although it would not have
made any difference in any of the RCU use cases.

							Thanx, Paul

> ---
>  include/linux/sched.h | 1 +
>  kernel/cpu.c          | 9 +++++++++
>  kernel/fork.c         | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 993e7e25a3a5..8f272ab57685 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -813,6 +813,7 @@ struct task_struct {
>  	unsigned int			lockdep_recursion;
>  	struct held_lock		held_locks[MAX_LOCK_DEPTH];
>  	gfp_t				lockdep_reclaim_gfp;
> +	int				goc_depth;
>  #endif
> 
>  #ifdef CONFIG_UBSAN
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 983163ef36ee..0dbdf1e69715 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -209,12 +209,21 @@ DEFINE_STATIC_PERCPU_RWSEM(cpu_hotplug_lock);
> 
>  void get_online_cpus(void)
>  {
> +#ifdef CONFIG_LOCKDEP
> +	if (current->goc_depth++)
> +		return;
> +#endif
>  	percpu_down_read(&cpu_hotplug_lock);
>  }
>  EXPORT_SYMBOL_GPL(get_online_cpus);
> 
>  void put_online_cpus(void)
>  {
> +#ifdef CONFIG_LOCKDEP
> +	WARN_ON_ONCE(current->goc_depth < 1);
> +	if (--current->goc_depth)
> +		return;
> +#endif
>  	percpu_up_read(&cpu_hotplug_lock);
>  }
>  EXPORT_SYMBOL_GPL(put_online_cpus);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index dd5a371c392a..8aedcd011ccc 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1658,6 +1658,7 @@ static __latent_entropy struct task_struct *copy_process(
>  	p->lockdep_depth = 0; /* no locks held yet */
>  	p->curr_chain_key = 0;
>  	p->lockdep_recursion = 0;
> +	p->goc_depth = 0;
>  #endif
> 
>  #ifdef CONFIG_DEBUG_MUTEXES
> -- 
> 2.10.2
> 
>

[toc] | [prev] | [next] | [standalone]

#1640684

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-12 20:50 +0200
Message-ID<tGnNv-gZ-7@gated-at.bofh.it>
In reply to#1640680
On Fri, 12 May 2017 11:35:59 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Fri, May 12, 2017 at 01:15:46PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > Allow get_online_cpus() to be recursive. If a lock is taken while under
> > "get_online_cpus()", it can call get_online_cpus() as well, just as long as
> > it is never held without being under get_online_cpus(), but then calling it.
> > 
> >    GOC() -> Lock(X) -> GOC()
> > 
> > is OK, as long as
> > 
> >    Lock(X) -> GOC()
> > 
> > does not exist.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> Does ->goc_depth also need to be initialized in include/linux/init_task.h?
> 
> It seems like C-language initialization-to-zero would cover this,
> but there is a lot of initialization to zero values in init_task.h
> (including but not limited to some RCU stuff).

I assumed that it would just initialize it to zero.

OK, I need to add this:

-- Steve

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index fffe49f..be7f71b 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -375,7 +375,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
 extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
 extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
 
-# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0,
+# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0, .goc_depth = 0,
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)

[toc] | [prev] | [next] | [standalone]

#1640689

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-05-12 21:00 +0200
Message-ID<tGnXb-lv-5@gated-at.bofh.it>
In reply to#1640684
On Fri, May 12, 2017 at 02:40:27PM -0400, Steven Rostedt wrote:
> On Fri, 12 May 2017 11:35:59 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Fri, May 12, 2017 at 01:15:46PM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > 
> > > Allow get_online_cpus() to be recursive. If a lock is taken while under
> > > "get_online_cpus()", it can call get_online_cpus() as well, just as long as
> > > it is never held without being under get_online_cpus(), but then calling it.
> > > 
> > >    GOC() -> Lock(X) -> GOC()
> > > 
> > > is OK, as long as
> > > 
> > >    Lock(X) -> GOC()
> > > 
> > > does not exist.
> > > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> > 
> > Does ->goc_depth also need to be initialized in include/linux/init_task.h?
> > 
> > It seems like C-language initialization-to-zero would cover this,
> > but there is a lot of initialization to zero values in init_task.h
> > (including but not limited to some RCU stuff).
> 
> I assumed that it would just initialize it to zero.
> 
> OK, I need to add this:
> 
> -- Steve
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index fffe49f..be7f71b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -375,7 +375,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map *lock);
>  extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
>  extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
> 
> -# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0,
> +# define INIT_LOCKDEP				.lockdep_recursion = 0, .lockdep_reclaim_gfp = 0, .goc_depth = 0,
> 
>  #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)

Or maybe we should remove a bunch of zero-initialization from that file.
But if it is needed, then that addition to the patch looks good to me.
Given how much zero-initialization there is, I suspect that it is needed
for some strange boot-up reason.  Hard to believe that someone would not
have gotten rid of it otherwise.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1640789

FromThomas Gleixner <tglx@linutronix.de>
Date2017-05-13 00:20 +0200
Message-ID<tGr4K-2RP-11@gated-at.bofh.it>
In reply to#1640627
On Fri, 12 May 2017, Steven Rostedt wrote:
>  void get_online_cpus(void)
>  {
> +#ifdef CONFIG_LOCKDEP
> +	if (current->goc_depth++)
> +		return;

This must be unconditional and not depend on lockdep. The percpu rwsem is
going to deadlock silently otherwise when a writer is waiting ....

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1640814

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-13 02:30 +0200
Message-ID<tGt6y-4nt-5@gated-at.bofh.it>
In reply to#1640789
On Sat, 13 May 2017 00:15:03 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 12 May 2017, Steven Rostedt wrote:
> >  void get_online_cpus(void)
> >  {
> > +#ifdef CONFIG_LOCKDEP
> > +	if (current->goc_depth++)
> > +		return;  
> 
> This must be unconditional and not depend on lockdep. The percpu rwsem is
> going to deadlock silently otherwise when a writer is waiting ....
> 

After I sent the updated changes to Paul, I realized this. But didn't
update it as I turned my attention to seeing if we can still get it
done without it.

But I'm not convinced that we can yet. I'll reply to Peter's email
again later.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web