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

Groups > linux.kernel > #1170269 > unrolled thread

Re: [RFC][PATCH 09/13] hotplug: Replace hotplug lock with percpu-rwsem

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

  Re: [RFC][PATCH 09/13] hotplug: Replace hotplug lock with  percpu-rwsem Oleg Nesterov <oleg@redhat.com> - 2015-06-23 01:00 +0200
    [PATCH 0/3] percpu-rwsem: introduce percpu_rw_semaphore->recursive  mode Oleg Nesterov <oleg@redhat.com> - 2015-06-29 02:00 +0200
      [PATCH 2/3] percpu-rwsem: don't use percpu_rw_semaphore->rw_sem to  exclude writers Oleg Nesterov <oleg@redhat.com> - 2015-06-29 02:00 +0200
      [PATCH 3/3] percpu-rwsem: introduce percpu_rw_semaphore->recursive  mode Oleg Nesterov <oleg@redhat.com> - 2015-06-29 02:00 +0200
      [PATCH 1/3] rcusync: introduce rcu_sync_struct->exclusive mode Oleg Nesterov <oleg@redhat.com> - 2015-06-29 02:00 +0200

#1170269 — Re: [RFC][PATCH 09/13] hotplug: Replace hotplug lock with percpu-rwsem

On 06/22, Peter Zijlstra wrote:
>
> The cpu hotplug lock is a rwsem with read-in-write and read-in-read
> recursion. Implement it as such.

And this patch fixes the problem afaics. Currently cpu_hotplug_begin()
can livelock because it doesn't stop the new readers. With this patch
this is no longer possible.


> -static inline void percpu_down_read(struct percpu_rw_semaphore *sem)
> +static inline void _percpu_down_read(struct percpu_rw_semaphore *sem)
>  {
>  	might_sleep();
>
> -	rwsem_acquire_read(&sem->rw_sem.dep_map, 0, 0, _RET_IP_);
> -
>  	preempt_disable();
>  	/*
>  	 * We are in an RCU-sched read-side critical section, so the writer
> @@ -46,6 +44,12 @@ static inline void percpu_down_read(stru
>  	 */
>  }
>
> +static inline void percpu_down_read(struct percpu_rw_semaphore *sem)
> +{
> +	rwsem_acquire_read(&sem->rw_sem.dep_map, 0, 0, _RET_IP_);
> +	_percpu_down_read(sem);
> +}

...

>  void get_online_cpus(void)
>  {
>  	might_sleep();
> -	if (cpu_hotplug.active_writer == current)
> +
> +	/* read in write recursion */
> +	if (cpu_hotplug.writer == current)
> +		return;
> +
> +	/* read in read recursion */
> +	if (current->cpuhp_ref++)
>  		return;
> -	cpuhp_lock_acquire_read();
> -	mutex_lock(&cpu_hotplug.lock);
> -	atomic_inc(&cpu_hotplug.refcount);
> -	mutex_unlock(&cpu_hotplug.lock);
> +
> +	lock_map_acquire_read(&cpu_hotplug.rwsem.rw_sem.dep_map);
> +	_percpu_down_read(&cpu_hotplug.rwsem);
>  }

Confused... Why do we need _percpu_down_read()? Can't get_online_cpus()
just use percpu_down_read() ?

Yes, percpu_down_read() is not recursive, like the normal down_read().
But this does not matter because we rely on ->cpuhp_ref anyway?


> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1410,6 +1410,8 @@ static struct task_struct *copy_process(
>  	p->sequential_io_avg	= 0;
>  #endif
>
> +	cpu_hotplug_init_task(p);

This is probably unnecessary, copy_process() should not be called under
get_online_cpus().

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1173584 — [PATCH 0/3] percpu-rwsem: introduce percpu_rw_semaphore->recursive mode

On 06/24, Oleg Nesterov wrote:
>
> So we need percpu_down_write_dont_block_readers(). I already thought
> about this before, I'll try to make the patch tomorrow on top of your
> changes.

Never say tomorrow...

> This means that we do not need task_struct->cpuhp_ref, but we can't
> avoid livelock we currently have: cpu_hotplug_begin() can never succeed
> if the new readers come fast enough.

Like with any other "recursive" lock.

Peter, I know you don't like the 1st patch. And yes, we could add another
mutex into percpu_rw_semaphore instead. But I think it would be better
to rely on rcu_sync_enter(). As for completion, we can remove it later.
Nevermind, the actual change is 3/3 and it looks simple.

Oleg.

--
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/

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

#1173585 — [PATCH 2/3] percpu-rwsem: don't use percpu_rw_semaphore->rw_sem to exclude writers

percpu_down_write() does down_write() to exclude both the readers and
other writers. We can rely on rcu_sync_enter() in exclusive mode and
take ->rw_sem right before wait_event().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/locking/percpu-rwsem.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index 014d2f4..609c13b 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -139,8 +139,6 @@ static bool readers_active_check(struct percpu_rw_semaphore *sem)
 
 void percpu_down_write(struct percpu_rw_semaphore *sem)
 {
-	down_write(&sem->rw_sem);
-
 	/* Notify readers to take the slow path. */
 	rcu_sync_enter(&sem->rss);
 
@@ -158,6 +156,7 @@ void percpu_down_write(struct percpu_rw_semaphore *sem)
 	 * therefore will wait for them.
 	 */
 
+	down_write(&sem->rw_sem);
 	/* Wait for all now active readers to complete. */
 	wait_event(sem->writer, readers_active_check(sem));
 }
-- 
1.5.5.1

--
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/

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

#1173587 — [PATCH 3/3] percpu-rwsem: introduce percpu_rw_semaphore->recursive mode

Add percpu_rw_semaphore->recursive boolean. If it is true then the
recursive percpu_down_read() is safe, percpu_down_write() doesn't
exclude the new readers, like cpu_hotplug_begin().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/percpu-rwsem.h  |   15 ++++++++++-----
 kernel/events/uprobes.c       |    2 +-
 kernel/locking/percpu-rwsem.c |   15 +++++++++++----
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index 9202e73..9441abd 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -13,16 +13,18 @@ struct percpu_rw_semaphore {
 	int			state;
 	struct rcu_sync_struct	rss;
 	wait_queue_head_t	writer;
+	bool			recursive;
 	struct rw_semaphore	rw_sem;
 };
 
-#define DEFINE_STATIC_PERCPU_RWSEM(name)				\
+#define DEFINE_STATIC_PERCPU_RWSEM(name, rec)				\
 static DEFINE_PER_CPU(unsigned int, __percpu_rwsem_refcount_##name);	\
 static struct percpu_rw_semaphore name = {				\
 	.refcount = &__percpu_rwsem_refcount_##name,			\
 	.state = 0,							\
 	.rss = __RCU_SYNC_INITIALIZER(name.rss, RCU_SCHED_SYNC, 1),	\
 	.writer = __WAIT_QUEUE_HEAD_INITIALIZER(name.writer),		\
+	.recursive = rec,						\
 	.rw_sem = __RWSEM_INITIALIZER(name.rw_sem),			\
 }
 
@@ -37,7 +39,10 @@ static inline void percpu_down_read(struct percpu_rw_semaphore *sem)
 {
 	might_sleep();
 
-	rwsem_acquire_read(&sem->rw_sem.dep_map, 0, 0, _RET_IP_);
+	if (sem->recursive)
+		rwlock_acquire_read(&sem->rw_sem.dep_map, 0, 0, _RET_IP_);
+	else
+		rwsem_acquire_read(&sem->rw_sem.dep_map, 0, 0, _RET_IP_);
 
 	preempt_disable();
 	/*
@@ -97,14 +102,14 @@ static inline void percpu_up_read(struct percpu_rw_semaphore *sem)
 extern void percpu_down_write(struct percpu_rw_semaphore *);
 extern void percpu_up_write(struct percpu_rw_semaphore *);
 
-extern int __percpu_init_rwsem(struct percpu_rw_semaphore *,
+extern int __percpu_init_rwsem(struct percpu_rw_semaphore *, bool,
 				const char *, struct lock_class_key *);
 extern void percpu_free_rwsem(struct percpu_rw_semaphore *);
 
-#define percpu_init_rwsem(sem)					\
+#define percpu_init_rwsem(sem, recursive)			\
 ({								\
 	static struct lock_class_key rwsem_key;			\
-	__percpu_init_rwsem(sem, #sem, &rwsem_key);		\
+	__percpu_init_rwsem(sem, recursive, #sem, &rwsem_key);	\
 })
 
 #endif
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index cb346f2..a4813a1 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1985,7 +1985,7 @@ static int __init init_uprobes(void)
 	for (i = 0; i < UPROBES_HASH_SZ; i++)
 		mutex_init(&uprobes_mmap_mutex[i]);
 
-	if (percpu_init_rwsem(&dup_mmap_sem))
+	if (percpu_init_rwsem(&dup_mmap_sem, false))
 		return -ENOMEM;
 
 	return register_die_notifier(&uprobe_exception_nb);
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index 609c13b..3db7c45 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -10,7 +10,7 @@
 
 enum { readers_slow, readers_block };
 
-int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
+int __percpu_init_rwsem(struct percpu_rw_semaphore *sem, bool recursive,
 			const char *name, struct lock_class_key *rwsem_key)
 {
 	sem->refcount = alloc_percpu(unsigned int);
@@ -20,6 +20,7 @@ int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
 	sem->state = readers_slow;
 	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC, true);
 	init_waitqueue_head(&sem->writer);
+	sem->recursive = recursive;
 	__init_rwsem(&sem->rw_sem, name, rwsem_key);
 
 	return 0;
@@ -124,9 +125,15 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
  */
 static bool readers_active_check(struct percpu_rw_semaphore *sem)
 {
-	if (per_cpu_sum(*sem->refcount) != 0)
+	if (sem->recursive && !down_write_trylock(&sem->rw_sem))
 		return false;
 
+	if (per_cpu_sum(*sem->refcount) != 0) {
+		if (sem->recursive)
+			up_write(&sem->rw_sem);
+		return false;
+	}
+
 	/*
 	 * If we observed the decrement; ensure we see the entire critical
 	 * section.
@@ -155,8 +162,8 @@ void percpu_down_write(struct percpu_rw_semaphore *sem)
 	 * then we are guaranteed to see their sem->refcount increment, and
 	 * therefore will wait for them.
 	 */
-
-	down_write(&sem->rw_sem);
+	if (!sem->recursive)
+		down_write(&sem->rw_sem);
 	/* Wait for all now active readers to complete. */
 	wait_event(sem->writer, readers_active_check(sem));
 }
-- 
1.5.5.1

--
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/

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

#1173589 — [PATCH 1/3] rcusync: introduce rcu_sync_struct->exclusive mode

Add rcu_sync_struct->exclusive boolean set by rcu_sync_init(), it
obviously controls the exclusiveness of rcu_sync_enter(). This is
what percpu_down_write() actually wants.

We turn ->gp_wait into "struct completion gp_comp", it is used as
a resource counter in "exclusive" mode. Otherwise we only use its
completion->wait member for wait_event/wake_up_all. We never mix
the completion/wait_queue_head_t operations.

TODO: we can cleanup this logic and avoid "struct completion", but
this needs a bit more changes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/percpu-rwsem.h  |    2 +-
 include/linux/rcusync.h       |   29 ++++++++++++++++-------------
 kernel/locking/percpu-rwsem.c |    2 +-
 kernel/rcu/sync.c             |   25 ++++++++++++++++++++-----
 4 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index e12ce86..9202e73 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -21,7 +21,7 @@ static DEFINE_PER_CPU(unsigned int, __percpu_rwsem_refcount_##name);	\
 static struct percpu_rw_semaphore name = {				\
 	.refcount = &__percpu_rwsem_refcount_##name,			\
 	.state = 0,							\
-	.rss = __RCU_SYNC_INITIALIZER(name.rss, RCU_SCHED_SYNC),	\
+	.rss = __RCU_SYNC_INITIALIZER(name.rss, RCU_SCHED_SYNC, 1),	\
 	.writer = __WAIT_QUEUE_HEAD_INITIALIZER(name.writer),		\
 	.rw_sem = __RWSEM_INITIALIZER(name.rw_sem),			\
 }
diff --git a/include/linux/rcusync.h b/include/linux/rcusync.h
index 0135838..aaea86a 100644
--- a/include/linux/rcusync.h
+++ b/include/linux/rcusync.h
@@ -1,7 +1,7 @@
 #ifndef _LINUX_RCUSYNC_H_
 #define _LINUX_RCUSYNC_H_
 
-#include <linux/wait.h>
+#include <linux/completion.h>
 #include <linux/rcupdate.h>
 
 enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
@@ -9,11 +9,12 @@ enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
 struct rcu_sync_struct {
 	int			gp_state;
 	int			gp_count;
-	wait_queue_head_t	gp_wait;
+	struct completion	gp_comp;
 
 	int			cb_state;
 	struct rcu_head		cb_head;
 
+	bool			exclusive;
 	enum rcu_sync_type	gp_type;
 };
 
@@ -28,30 +29,32 @@ static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
 #endif
 }
 
-extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
+extern void rcu_sync_init(struct rcu_sync_struct *,
+				enum rcu_sync_type, bool excl);
 extern void rcu_sync_enter(struct rcu_sync_struct *);
 extern void rcu_sync_exit(struct rcu_sync_struct *);
 extern void rcu_sync_dtor(struct rcu_sync_struct *);
 
-#define __RCU_SYNC_INITIALIZER(name, type) {				\
+#define __RCU_SYNC_INITIALIZER(name, type, excl) {			\
 		.gp_state = 0,						\
 		.gp_count = 0,						\
-		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
+		.gp_comp = COMPLETION_INITIALIZER(name.gp_comp),	\
 		.cb_state = 0,						\
+		.exclusive = excl,					\
 		.gp_type = type,					\
 	}
 
-#define	__DEFINE_RCU_SYNC(name, type)	\
-	struct rcu_sync_struct name = __RCU_SYNC_INITIALIZER(name, type)
+#define	__DEFINE_RCU_SYNC(name, type, excl)	\
+	struct rcu_sync_struct name = __RCU_SYNC_INITIALIZER(name, type, excl)
 
-#define DEFINE_RCU_SYNC(name)		\
-	__DEFINE_RCU_SYNC(name, RCU_SYNC)
+#define DEFINE_RCU_SYNC(name, excl)		\
+	__DEFINE_RCU_SYNC(name, RCU_SYNC, excl)
 
-#define DEFINE_RCU_SCHED_SYNC(name)	\
-	__DEFINE_RCU_SYNC(name, RCU_SCHED_SYNC)
+#define DEFINE_RCU_SCHED_SYNC(name, excl)	\
+	__DEFINE_RCU_SYNC(name, RCU_SCHED_SYNC, excl)
 
-#define DEFINE_RCU_BH_SYNC(name)	\
-	__DEFINE_RCU_SYNC(name, RCU_BH_SYNC)
+#define DEFINE_RCU_BH_SYNC(name, excl)	\
+	__DEFINE_RCU_SYNC(name, RCU_BH_SYNC, excl)
 
 #endif /* _LINUX_RCUSYNC_H_ */
 
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index 915646c..014d2f4 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -18,7 +18,7 @@ int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
 		return -ENOMEM;
 
 	sem->state = readers_slow;
-	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC);
+	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC, true);
 	init_waitqueue_head(&sem->writer);
 	__init_rwsem(&sem->rw_sem, name, rwsem_key);
 
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 8835ad1..03ddc61 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -38,7 +38,8 @@ static const struct {
 enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
 enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
 
-#define	rss_lock	gp_wait.lock
+#define	rss_lock	gp_comp.wait.lock
+#define	gp_wait		gp_comp.wait
 
 #ifdef CONFIG_PROVE_RCU
 bool __rcu_sync_is_idle(struct rcu_sync_struct *rss)
@@ -49,10 +50,12 @@ bool __rcu_sync_is_idle(struct rcu_sync_struct *rss)
 EXPORT_SYMBOL_GPL(__rcu_sync_is_idle);
 #endif
 
-void rcu_sync_init(struct rcu_sync_struct *rss, enum rcu_sync_type type)
+void rcu_sync_init(struct rcu_sync_struct *rss,
+			enum rcu_sync_type type, bool excl)
 {
 	memset(rss, 0, sizeof(*rss));
-	init_waitqueue_head(&rss->gp_wait);
+	init_completion(&rss->gp_comp);
+	rss->exclusive = excl;
 	rss->gp_type = type;
 }
 
@@ -72,9 +75,13 @@ void rcu_sync_enter(struct rcu_sync_struct *rss)
 	if (need_sync) {
 		gp_ops[rss->gp_type].sync();
 		rss->gp_state = GP_PASSED;
-		wake_up_all(&rss->gp_wait);
+		if (!rss->exclusive)
+			wake_up_all(&rss->gp_wait);
 	} else if (need_wait) {
-		wait_event(rss->gp_wait, rss->gp_state == GP_PASSED);
+		if (!rss->exclusive)
+			wait_event(rss->gp_wait, rss->gp_state == GP_PASSED);
+		else
+			wait_for_completion(&rss->gp_comp);
 	} else {
 		/*
 		 * Possible when there's a pending CB from a rcu_sync_exit().
@@ -119,6 +126,12 @@ static void rcu_sync_func(struct rcu_head *rcu)
 	spin_unlock_irqrestore(&rss->rss_lock, flags);
 }
 
+static inline void __complete_locked(struct completion *x)
+{
+	x->done++;
+	__wake_up_locked(&x->wait, TASK_NORMAL, 1);
+}
+
 void rcu_sync_exit(struct rcu_sync_struct *rss)
 {
 	spin_lock_irq(&rss->rss_lock);
@@ -129,6 +142,8 @@ void rcu_sync_exit(struct rcu_sync_struct *rss)
 		} else if (rss->cb_state == CB_PENDING) {
 			rss->cb_state = CB_REPLAY;
 		}
+	} else if (rss->exclusive) {
+		__complete_locked(&rss->gp_comp);
 	}
 	spin_unlock_irq(&rss->rss_lock);
 }
-- 
1.5.5.1

--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web