Skip to contentSkip to conversation

Optimize percpu-rwsem

RFC PATCH In review Peter Zijlstra 9 participants 36 messages May 26, 2015 – Jun 19, 2015

Subsystems (6)
Documentation perf events RCU Locking primitives +2 more subsystems
lore
Share & tools
lore

Peter Zijlstra

author maintainer
#0 lore raw

cover Optimize percpu-rwsem

Hi all,

This is a derived work of the cpu hotplug lock rework I did in 2013 which never really went anywhere because Linus didn't like it.

This applies those same optimizations to the percpu-rwsem. Seeing how we did all the work it seemed a waste to not use it at all.

author maintainer
#1 lore raw

RFC PATCH 1/5 rcu: Create rcu_sync infrastructure

From: Oleg Nesterov <[email protected]>

It is functionally equivalent to

        struct rcu_sync_struct {
                atomic_t counter;
        };

        static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
        {
                return atomic_read(&rss->counter) == 0;
        }

        static inline void rcu_sync_enter(struct rcu_sync_struct *rss)
        {
                atomic_inc(&rss->counter);
                synchronize_sched();
        }

        static inline void rcu_sync_exit(struct rcu_sync_struct *rss)
        {
                synchronize_sched();
                atomic_dec(&rss->counter);
        }

except: it records the state and synchronize_sched() is only called by rcu_sync_enter() and only if necessary.

Reviewed-by Paul E. McKenney <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rcusync.h |   64 ++++++++++++++++++++++++++++
 kernel/rcu/Makefile     |    2 
 kernel/rcu/sync.c       |  108 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+), 1 deletion(-)
include/linux/rcusync.h new file +64 -0
@@ -0,0 +1,64 @@
+#ifndef _LINUX_RCUSYNC_H_
+#define _LINUX_RCUSYNC_H_
+
+#include <linux/wait.h>
+#include <linux/rcupdate.h>
+
+struct rcu_sync_struct {
+	int			gp_state;
+	int			gp_count;
+	wait_queue_head_t	gp_wait;
+
+	int			cb_state;
+	struct rcu_head		cb_head;
+
+	void (*sync)(void);
+	void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
+};
+
+#define ___RCU_SYNC_INIT(name)						\
+	.gp_state = 0,							\
+	.gp_count = 0,							\
+	.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),		\
+	.cb_state = 0
+
+#define __RCU_SCHED_SYNC_INIT(name) {					\
+	___RCU_SYNC_INIT(name),						\
+	.sync = synchronize_sched,					\
+	.call = call_rcu_sched,						\
+}
+
+#define __RCU_BH_SYNC_INIT(name) {					\
+	___RCU_SYNC_INIT(name),						\
+	.sync = synchronize_rcu_bh,					\
+	.call = call_rcu_bh,						\
+}
+
+#define __RCU_SYNC_INIT(name) {						\
+	___RCU_SYNC_INIT(name),						\
+	.sync = synchronize_rcu,					\
+	.call = call_rcu,						\
+}
+
+#define DEFINE_RCU_SCHED_SYNC(name)					\
+	struct rcu_sync_struct name = __RCU_SCHED_SYNC_INIT(name)
+
+#define DEFINE_RCU_BH_SYNC(name)					\
+	struct rcu_sync_struct name = __RCU_BH_SYNC_INIT(name)
+
+#define DEFINE_RCU_SYNC(name)						\
+	struct rcu_sync_struct name = __RCU_SYNC_INIT(name)
+
+static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
+{
+	return !rss->gp_state; /* GP_IDLE */
+}
+
+enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
+
+extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
+extern void rcu_sync_enter(struct rcu_sync_struct *);
+extern void rcu_sync_exit(struct rcu_sync_struct *);
+
+#endif /* _LINUX_RCUSYNC_H_ */
+
kernel/rcu/Makefile +1 -1
@@ -1,4 +1,4 @@
-obj-y += update.o
+obj-y += update.o sync.o
 obj-$(CONFIG_SRCU) += srcu.o
 obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
 obj-$(CONFIG_TREE_RCU) += tree.o
kernel/rcu/sync.c new file +108 -0

108 lines not on this page · raw message

#2 lore raw

Re: [RFC][PATCH 1/5] rcu: Create rcu_sync infrastructure

Quoted 4 lines · Peter Zijlstra

From: Oleg Nesterov <[email protected]>

It is functionally equivalent to

Quoted 14 lines
        struct rcu_sync_struct {
                atomic_t counter;
        };

        static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
        {
                return atomic_read(&rss->counter) == 0;
        }

        static inline void rcu_sync_enter(struct rcu_sync_struct *rss)
        {
                atomic_inc(&rss->counter);
                synchronize_sched();
        }

For vanilla RCU, this is called get_state_synchronize_rcu().

Quoted 8 lines
        static inline void rcu_sync_exit(struct rcu_sync_struct *rss)
        {
                synchronize_sched();
                atomic_dec(&rss->counter);
        }

except: it records the state and synchronize_sched() is only called by rcu_sync_enter() and only if necessary.

Again for vanilla RCU, this is called cond_synchronize_rcu().

These functions are quite small, so the other flavors can be created easily if needed.

Thanx, Paul

Quoted 197 lines
Reviewed-by Paul E. McKenney <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rcusync.h |   64 ++++++++++++++++++++++++++++
 kernel/rcu/Makefile     |    2 
 kernel/rcu/sync.c       |  108 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+), 1 deletion(-)
include/linux/rcusync.h new file +64 -0
@@ -0,0 +1,64 @@
+#ifndef _LINUX_RCUSYNC_H_
+#define _LINUX_RCUSYNC_H_
+
+#include <linux/wait.h>
+#include <linux/rcupdate.h>
+
+struct rcu_sync_struct {
+	int			gp_state;
+	int			gp_count;
+	wait_queue_head_t	gp_wait;
+
+	int			cb_state;
+	struct rcu_head		cb_head;
+
+	void (*sync)(void);
+	void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
+};
+
+#define ___RCU_SYNC_INIT(name)						\
+	.gp_state = 0,							\
+	.gp_count = 0,							\
+	.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),		\
+	.cb_state = 0
+
+#define __RCU_SCHED_SYNC_INIT(name) {					\
+	___RCU_SYNC_INIT(name),						\
+	.sync = synchronize_sched,					\
+	.call = call_rcu_sched,						\
+}
+
+#define __RCU_BH_SYNC_INIT(name) {					\
+	___RCU_SYNC_INIT(name),						\
+	.sync = synchronize_rcu_bh,					\
+	.call = call_rcu_bh,						\
+}
+
+#define __RCU_SYNC_INIT(name) {						\
+	___RCU_SYNC_INIT(name),						\
+	.sync = synchronize_rcu,					\
+	.call = call_rcu,						\
+}
+
+#define DEFINE_RCU_SCHED_SYNC(name)					\
+	struct rcu_sync_struct name = __RCU_SCHED_SYNC_INIT(name)
+
+#define DEFINE_RCU_BH_SYNC(name)					\
+	struct rcu_sync_struct name = __RCU_BH_SYNC_INIT(name)
+
+#define DEFINE_RCU_SYNC(name)						\
+	struct rcu_sync_struct name = __RCU_SYNC_INIT(name)
+
+static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
+{
+	return !rss->gp_state; /* GP_IDLE */
+}
+
+enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
+
+extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
+extern void rcu_sync_enter(struct rcu_sync_struct *);
+extern void rcu_sync_exit(struct rcu_sync_struct *);
+
+#endif /* _LINUX_RCUSYNC_H_ */
+
kernel/rcu/Makefile +1 -1
@@ -1,4 +1,4 @@
-obj-y += update.o
+obj-y += update.o sync.o
 obj-$(CONFIG_SRCU) += srcu.o
 obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
 obj-$(CONFIG_TREE_RCU) += tree.o
kernel/rcu/sync.c new file +108 -0
@@ -0,0 +1,108 @@
+
+#include <linux/rcusync.h>
+#include <linux/sched.h>
+
+enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
+enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
+
+#define	rss_lock	gp_wait.lock
+
+void rcu_sync_init(struct rcu_sync_struct *rss, enum rcu_sync_type type)
+{
+	memset(rss, 0, sizeof(*rss));
+	init_waitqueue_head(&rss->gp_wait);
+
+	switch (type) {
+	case RCU_SYNC:
+		rss->sync = synchronize_rcu;
+		rss->call = call_rcu;
+		break;
+
+	case RCU_SCHED_SYNC:
+		rss->sync = synchronize_sched;
+		rss->call = call_rcu_sched;
+		break;
+
+	case RCU_BH_SYNC:
+		rss->sync = synchronize_rcu_bh;
+		rss->call = call_rcu_bh;
+		break;
+	}
+}
+
+void rcu_sync_enter(struct rcu_sync_struct *rss)
+{
+	bool need_wait, need_sync;
+
+	spin_lock_irq(&rss->rss_lock);
+	need_wait = rss->gp_count++;
+	need_sync = rss->gp_state == GP_IDLE;
+	if (need_sync)
+		rss->gp_state = GP_PENDING;
+	spin_unlock_irq(&rss->rss_lock);
+
+	BUG_ON(need_wait && need_sync);
+
+	if (need_sync) {
+		rss->sync();
+		rss->gp_state = GP_PASSED;
+		wake_up_all(&rss->gp_wait);
+	} else if (need_wait) {
+		wait_event(rss->gp_wait, rss->gp_state == GP_PASSED);
+	} else {
+		/*
+		 * Possible when there's a pending CB from a rcu_sync_exit().
+		 * Nobody has yet been allowed the 'fast' path and thus we can
+		 * avoid doing any sync(). The callback will get 'dropped'.
+		 */
+		BUG_ON(rss->gp_state != GP_PASSED);
+	}
+}
+
+static void rcu_sync_func(struct rcu_head *rcu)
+{
+	struct rcu_sync_struct *rss =
+		container_of(rcu, struct rcu_sync_struct, cb_head);
+	unsigned long flags;
+
+
+	BUG_ON(rss->gp_state != GP_PASSED);
+	BUG_ON(rss->cb_state == CB_IDLE);
+
+	spin_lock_irqsave(&rss->rss_lock, flags);
+	if (rss->gp_count) {
+		/*
+		 * A new rcu_sync_begin() has happened; drop the callback.
+		 */
+		rss->cb_state = CB_IDLE;
+	} else if (rss->cb_state == CB_REPLAY) {
+		/*
+		 * A new rcu_sync_exit() has happened; requeue the callback
+		 * to catch a later GP.
+		 */
+		rss->cb_state = CB_PENDING;
+		rss->call(&rss->cb_head, rcu_sync_func);
+	} else {
+		/*
+		 * We're at least a GP after rcu_sync_exit(); eveybody will now
+		 * have observed the write side critical section. Let 'em rip!.
+		 */
+		rss->cb_state = CB_IDLE;
+		rss->gp_state = GP_IDLE;
+	}
+	spin_unlock_irqrestore(&rss->rss_lock, flags);
+}
+
+void rcu_sync_exit(struct rcu_sync_struct *rss)
+{
+	spin_lock_irq(&rss->rss_lock);
+	if (!--rss->gp_count) {
+		if (rss->cb_state == CB_IDLE) {
+			rss->cb_state = CB_PENDING;
+			rss->call(&rss->cb_head, rcu_sync_func);
+		} else if (rss->cb_state == CB_PENDING) {
+			rss->cb_state = CB_REPLAY;
+		}
+	}
+	spin_unlock_irq(&rss->rss_lock);
+}
#3 lore raw

Re: [RFC][PATCH 1/5] rcu: Create rcu_sync infrastructure

Quoted 29 lines · Paul E. McKenney
Quoted 4 lines · Peter Zijlstra

From: Oleg Nesterov <[email protected]>

It is functionally equivalent to

Quoted 14 lines
        struct rcu_sync_struct {
                atomic_t counter;
        };

        static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
        {
                return atomic_read(&rss->counter) == 0;
        }

        static inline void rcu_sync_enter(struct rcu_sync_struct *rss)
        {
                atomic_inc(&rss->counter);
                synchronize_sched();
        }

For vanilla RCU, this is called get_state_synchronize_rcu().

Quoted 6 lines
        static inline void rcu_sync_exit(struct rcu_sync_struct *rss)
        {
                synchronize_sched();
                atomic_dec(&rss->counter);
        }
Quoted 4 lines
Quoted 2 lines

except: it records the state and synchronize_sched() is only called by rcu_sync_enter() and only if necessary.

Again for vanilla RCU, this is called cond_synchronize_rcu().

Hmm. I do not understand... I think rcu_sync doesn't need get_state/cond_synchronize.

The first caller of rcu_sync_enter() always needs sync(). The next one could use cond_synchronize_rcu(), but for what? The 2nd one will wait for the end of gp started by the first caller, and this is more optimal?

Note that rcu_sync_enter/rcu_sync_func never call sync() unless strictly necessary.

Or I misunderstood you?

Oleg.

#4 lore raw

Re: [RFC][PATCH 1/5] rcu: Create rcu_sync infrastructure

Quoted 39 lines · Oleg Nesterov
Quoted 29 lines · Paul E. McKenney
Quoted 4 lines · Peter Zijlstra

From: Oleg Nesterov <[email protected]>

It is functionally equivalent to

Quoted 14 lines
        struct rcu_sync_struct {
                atomic_t counter;
        };

        static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
        {
                return atomic_read(&rss->counter) == 0;
        }

        static inline void rcu_sync_enter(struct rcu_sync_struct *rss)
        {
                atomic_inc(&rss->counter);
                synchronize_sched();
        }

For vanilla RCU, this is called get_state_synchronize_rcu().

Quoted 6 lines
        static inline void rcu_sync_exit(struct rcu_sync_struct *rss)
        {
                synchronize_sched();
                atomic_dec(&rss->counter);
        }
Quoted 4 lines
Quoted 2 lines

except: it records the state and synchronize_sched() is only called by rcu_sync_enter() and only if necessary.

Again for vanilla RCU, this is called cond_synchronize_rcu().

Hmm. I do not understand... I think rcu_sync doesn't need get_state/cond_synchronize.

Quoted 6 lines

The first caller of rcu_sync_enter() always needs sync(). The next one could use cond_synchronize_rcu(), but for what? The 2nd one will wait for the end of gp started by the first caller, and this is more optimal?

Note that rcu_sync_enter/rcu_sync_func never call sync() unless strictly necessary.

If you meant that rcu_sync_exit() could use cond_synchronize_rcu(), this doesn't look right too... We always need another synchronize_rcu() after the last writer does rcu_sync_exit(). Except rcu_sync_exit() uses call_rcu() and thus it never blocks.

Quoted 1 line

Or I misunderstood you?

Yes...

Oleg.

#5 lore raw

Re: [RFC][PATCH 1/5] rcu: Create rcu_sync infrastructure

In reply to Oleg Nesterov
Quoted 38 lines · Oleg Nesterov
Quoted 29 lines · Paul E. McKenney
Quoted 4 lines · Peter Zijlstra

From: Oleg Nesterov <[email protected]>

It is functionally equivalent to

Quoted 14 lines
        struct rcu_sync_struct {
                atomic_t counter;
        };

        static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
        {
                return atomic_read(&rss->counter) == 0;
        }

        static inline void rcu_sync_enter(struct rcu_sync_struct *rss)
        {
                atomic_inc(&rss->counter);
                synchronize_sched();
        }

For vanilla RCU, this is called get_state_synchronize_rcu().

Quoted 6 lines
        static inline void rcu_sync_exit(struct rcu_sync_struct *rss)
        {
                synchronize_sched();
                atomic_dec(&rss->counter);
        }
Quoted 4 lines
Quoted 2 lines

except: it records the state and synchronize_sched() is only called by rcu_sync_enter() and only if necessary.

Again for vanilla RCU, this is called cond_synchronize_rcu().

Hmm. I do not understand... I think rcu_sync doesn't need get_state/cond_synchronize.

Quoted 8 lines

The first caller of rcu_sync_enter() always needs sync(). The next one could use cond_synchronize_rcu(), but for what? The 2nd one will wait for the end of gp started by the first caller, and this is more optimal?

Note that rcu_sync_enter/rcu_sync_func never call sync() unless strictly necessary.

Or I misunderstood you?

Probably me forgetting exactly what your APIs above are doing.

I clearly need to take a closer look when fully awake. ;-)

Thanx, Paul

author maintainer
#6 lore raw

RFC PATCH 2/5 rcusync: Introduce struct rcu_sync_ops

In reply to Peter Zijlstra

From: Oleg Nesterov <[email protected]>

Add the new struct rcu_sync_ops which holds sync/call methods, and turn the function pointers in rcu_sync_struct into an array of struct rcu_sync_ops.

This simplifies the "init" helpers, and this way it is simpler to add the new methods we need, especially ifdef'ed.

Reviewed-by Paul E. McKenney <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rcusync.h |   60 ++++++++++++++++++------------------------------
 kernel/rcu/sync.c       |   43 +++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 58 deletions(-)
include/linux/rcusync.h +23 -37
@@ -4,6 +4,8 @@
 #include <linux/wait.h>
 #include <linux/rcupdate.h>
 
+enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
+
 struct rcu_sync_struct {
 	int			gp_state;
 	int			gp_count;

@@ -12,53 +14,37 @@ struct rcu_sync_struct {
 	int			cb_state;
 	struct rcu_head		cb_head;
 
-	void (*sync)(void);
-	void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
+	enum rcu_sync_type	gp_type;
 };
 
-#define ___RCU_SYNC_INIT(name)						\
-	.gp_state = 0,							\
-	.gp_count = 0,							\
-	.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),		\
-	.cb_state = 0
-
-#define __RCU_SCHED_SYNC_INIT(name) {					\
-	___RCU_SYNC_INIT(name),						\
-	.sync = synchronize_sched,					\
-	.call = call_rcu_sched,						\
-}
-
-#define __RCU_BH_SYNC_INIT(name) {					\
-	___RCU_SYNC_INIT(name),						\
-	.sync = synchronize_rcu_bh,					\
-	.call = call_rcu_bh,						\
-}
-
-#define __RCU_SYNC_INIT(name) {						\
-	___RCU_SYNC_INIT(name),						\
-	.sync = synchronize_rcu,					\
-	.call = call_rcu,						\
-}
-
-#define DEFINE_RCU_SCHED_SYNC(name)					\
-	struct rcu_sync_struct name = __RCU_SCHED_SYNC_INIT(name)
-
-#define DEFINE_RCU_BH_SYNC(name)					\
-	struct rcu_sync_struct name = __RCU_BH_SYNC_INIT(name)
-
-#define DEFINE_RCU_SYNC(name)						\
-	struct rcu_sync_struct name = __RCU_SYNC_INIT(name)
-
 static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
 {
 	return !rss->gp_state; /* GP_IDLE */
 }
 
-enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
-
 extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
 extern void rcu_sync_enter(struct rcu_sync_struct *);
 extern void rcu_sync_exit(struct rcu_sync_struct *);
 
+#define __RCU_SYNC_INITIALIZER(name, type) {				\
+		.gp_state = 0,						\
+		.gp_count = 0,						\
+		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
+		.cb_state = 0,						\
+		.gp_type = type,					\
+	}
+
+#define	__DEFINE_RCU_SYNC(name, type)	\
+	struct rcu_sync_struct name = __RCU_SYNC_INITIALIZER(name, type)
+
+#define DEFINE_RCU_SYNC(name)		\
+	__DEFINE_RCU_SYNC(name, RCU_SYNC)
+
+#define DEFINE_RCU_SCHED_SYNC(name)	\
+	__DEFINE_RCU_SYNC(name, RCU_SCHED_SYNC)
+
+#define DEFINE_RCU_BH_SYNC(name)	\
+	__DEFINE_RCU_SYNC(name, RCU_BH_SYNC)
+
 #endif /* _LINUX_RCUSYNC_H_ */
kernel/rcu/sync.c +22 -21

73 lines not on this page · raw message

author maintainer
#7 lore raw

RFC PATCH 3/5 rcusync: Add the CONFIG_PROVE_RCU checks

In reply to Peter Zijlstra

From: Oleg Nesterov <[email protected]>

It would be nice to validate that the caller of rcu_sync_is_idle() holds the corresponding type of RCU read-side lock. Add the new rcu_sync_ops->held() method and change rcu_sync_is_idle() to WARN() if it returns false.

This obviously penalizes the readers (fast-path), but only if CONFIG_PROVE_RCU.

Reviewed-by Paul E. McKenney <[email protected]>
Suggested-by "Paul E. McKenney" <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rcusync.h |    6 ++++++
 kernel/rcu/sync.c       |   21 +++++++++++++++++++++
 2 files changed, 27 insertions(+)
include/linux/rcusync.h +6 -0
@@ -17,9 +17,15 @@ struct rcu_sync_struct {
 	enum rcu_sync_type	gp_type;
 };
 
+extern bool __rcu_sync_is_idle(struct rcu_sync_struct *);
+
 static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
 {
+#ifdef CONFIG_PROVE_RCU
+	return __rcu_sync_is_idle(rss);
+#else
 	return !rss->gp_state; /* GP_IDLE */
+#endif
 }
 
 extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
kernel/rcu/sync.c +21 -0
@@ -1,21 +1,33 @@
 #include <linux/rcusync.h>
 #include <linux/sched.h>
 
+#ifdef CONFIG_PROVE_RCU
+#define __INIT_HELD(func)	.held = func,
+#else
+#define __INIT_HELD(func)
+#endif
+
 static const struct {
 	void (*sync)(void);
 	void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
+#ifdef CONFIG_PROVE_RCU
+	int  (*held)(void);
+#endif
 } gp_ops[] = {
 	[RCU_SYNC] = {
 		.sync = synchronize_rcu,
 		.call = call_rcu,
+		__INIT_HELD(rcu_read_lock_held)
 	},
 	[RCU_SCHED_SYNC] = {
 		.sync = synchronize_sched,
 		.call = call_rcu_sched,
+		__INIT_HELD(rcu_read_lock_sched_held)
 	},
 	[RCU_BH_SYNC] = {
 		.sync = synchronize_rcu_bh,
 		.call = call_rcu_bh,
+		__INIT_HELD(rcu_read_lock_bh_held)
 	},
 };
 

@@ -24,6 +36,15 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLA
 
 #define	rss_lock	gp_wait.lock
 
+#ifdef CONFIG_PROVE_RCU
+bool __rcu_sync_is_idle(struct rcu_sync_struct *rss)
+{
+	WARN_ON(!gp_ops[rss->gp_type].held());
+	return rss->gp_state == GP_IDLE;
+}
+EXPORT_SYMBOL_GPL(__rcu_sync_is_idle);
+#endif
+
 void rcu_sync_init(struct rcu_sync_struct *rss, enum rcu_sync_type type)
 {
 	memset(rss, 0, sizeof(*rss));
author maintainer
#8 lore raw

RFC PATCH 4/5 rcusync: Introduce rcu_sync_dtor()

In reply to Peter Zijlstra

From: Oleg Nesterov <[email protected]>

Add the new rcu_sync_ops->wait() method and the new helper, rcu_sync_dtor().

It is needed if you are going to, say, kfree(rcu_sync_object). It simply calls ops->wait() to "flush" the potentially pending rcu callback.

Reviewed-by Paul E. McKenney <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/rcusync.h |    1 +
 kernel/rcu/sync.c       |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)
include/linux/rcusync.h +1 -0
@@ -31,6 +31,7 @@ static inline bool rcu_sync_is_idle(stru
 extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
 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) {				\
 		.gp_state = 0,						\
kernel/rcu/sync.c +22 -0
@@ -10,6 +10,7 @@
 static const struct {
 	void (*sync)(void);
 	void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
+	void (*wait)(void);
 #ifdef CONFIG_PROVE_RCU
 	int  (*held)(void);
 #endif

@@ -17,16 +18,19 @@ static const struct {
 	[RCU_SYNC] = {
 		.sync = synchronize_rcu,
 		.call = call_rcu,
+		.wait = rcu_barrier,
 		__INIT_HELD(rcu_read_lock_held)
 	},
 	[RCU_SCHED_SYNC] = {
 		.sync = synchronize_sched,
 		.call = call_rcu_sched,
+		.wait = rcu_barrier_sched,
 		__INIT_HELD(rcu_read_lock_sched_held)
 	},
 	[RCU_BH_SYNC] = {
 		.sync = synchronize_rcu_bh,
 		.call = call_rcu_bh,
+		.wait = rcu_barrier_bh,
 		__INIT_HELD(rcu_read_lock_bh_held)
 	},
 };

@@ -128,3 +132,21 @@ void rcu_sync_exit(struct rcu_sync_struc
 	}
 	spin_unlock_irq(&rss->rss_lock);
 }
+
+void rcu_sync_dtor(struct rcu_sync_struct *rss)
+{
+	int cb_state;
+
+	BUG_ON(rss->gp_count);
+
+	spin_lock_irq(&rss->rss_lock);
+	if (rss->cb_state == CB_REPLAY)
+		rss->cb_state = CB_PENDING;
+	cb_state = rss->cb_state;
+	spin_unlock_irq(&rss->rss_lock);
+
+	if (cb_state != CB_IDLE) {
+		gp_ops[rss->gp_type].wait();
+		BUG_ON(rss->cb_state != CB_IDLE);
+	}
+}
author maintainer
#9 lore raw

RFC PATCH 5/5 percpu-rwsem: Optimize readers and reduce global impact

In reply to Peter Zijlstra

Currently the percpu-rwsem has two issues:

- it switches to (global) atomic ops while a writer is waiting; which could be quite a while and slows down releasing the readers.

- it employs synchronize_sched_expedited() _twice_ which is evil and should die -- it shoots IPIs around the machine.

This patch cures the first problem by ordering the reader-state vs reader-count (see the comments in __percpu_down_read() and percpu_down_write()). This changes a global atomic op into a full memory barrier, which doesn't have the global cacheline contention.

It cures the second problem by employing the rcu-sync primitives by Oleg which reduces to no sync_sched() calls in the 'normal' case of no write contention -- global locks had better be rare, and has a maximum of one sync_sched() call in case of contention.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/percpu-rwsem.h  |   62 +++++++++-
 kernel/locking/percpu-rwsem.c |  238 +++++++++++++++++++++---------------------
 2 files changed, 176 insertions(+), 124 deletions(-)
include/linux/percpu-rwsem.h +54 -8
@@ -5,18 +5,64 @@
 #include <linux/rwsem.h>
 #include <linux/percpu.h>
 #include <linux/wait.h>
+#include <linux/rcusync.h>
 #include <linux/lockdep.h>
 
 struct percpu_rw_semaphore {
-	unsigned int __percpu	*fast_read_ctr;
-	atomic_t		write_ctr;
+	unsigned int __percpu	*refcount;
+	int			state;
+	struct rcu_sync_struct	rss;
+	wait_queue_head_t	writer;
 	struct rw_semaphore	rw_sem;
-	atomic_t		slow_read_ctr;
-	wait_queue_head_t	write_waitq;
 };
 
-extern void percpu_down_read(struct percpu_rw_semaphore *);
-extern void percpu_up_read(struct percpu_rw_semaphore *);
+extern void __percpu_down_read(struct percpu_rw_semaphore *);
+extern void __percpu_up_read(struct percpu_rw_semaphore *);
+
+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
+	 * cannot both change sem->state from readers_fast and start
+	 * checking counters while we are here. So if we see !sem->state,
+	 * we know that the writer won't be checking until we past the
+	 * preempt_enable() and that once the synchronize_sched() is done, the
+	 * writer will see anything we did within this RCU-sched read-side
+	 * critical section.
+	 */
+	__this_cpu_inc(*sem->refcount);
+	if (unlikely(!rcu_sync_is_idle(&sem->rss)))
+		__percpu_down_read(sem); /* Unconditional memory barrier. */
+	preempt_enable();
+	/*
+	 * The barrier() from preempt_enable() prevents the compiler from
+	 * bleeding the critical section out.
+	 */
+}
+
+static inline void percpu_up_read(struct percpu_rw_semaphore *sem)
+{
+	/*
+	 * The barrier() in preempt_disable() prevents the compiler from
+	 * bleeding the critical section out.
+	 */
+	preempt_disable();
+	/*
+	 * Same as in percpu_down_read().
+	 */
+	if (likely(rcu_sync_is_idle(&sem->rss)))
+		__this_cpu_dec(*sem->refcount);
+	else
+		__percpu_up_read(sem); /* Unconditional memory barrier. */
+	preempt_enable();
+
+	rwsem_release(&sem->rw_sem.dep_map, 1, _RET_IP_);
+}
 
 extern void percpu_down_write(struct percpu_rw_semaphore *);
 extern void percpu_up_write(struct percpu_rw_semaphore *);

@@ -25,10 +71,10 @@ extern int __percpu_init_rwsem(struct pe
 				const char *, struct lock_class_key *);
 extern void percpu_free_rwsem(struct percpu_rw_semaphore *);
 
-#define percpu_init_rwsem(brw)	\
+#define percpu_init_rwsem(sem)					\
 ({								\
 	static struct lock_class_key rwsem_key;			\
-	__percpu_init_rwsem(brw, #brw, &rwsem_key);		\
+	__percpu_init_rwsem(sem, #sem, &rwsem_key);		\
 })
 
 #endif
kernel/locking/percpu-rwsem.c +122 -116

280 lines not on this page · raw message

Reviewed-by on v1 5/5
#10 lore raw

Re: [RFC][PATCH 5/5] percpu-rwsem: Optimize readers and reduce global impact

Sorry for delay, finally I found the time to read this series... The code matches our previous discussion and I believe it is correct.

Reviewed-by Oleg Nesterov <[email protected]>

Just one nit below,

Quoted 6 lines · Peter Zijlstra

struct percpu_rw_semaphore {

include/linux/percpu-rwsem.h +2 -2
-	unsigned int __percpu	*fast_read_ctr;
-	atomic_t		write_ctr;
+	unsigned int __percpu	*refcount;
+	int			state;

....

Quoted 1 line

+enum { readers_slow, readers_block };

Now that we rely on rss_sync() and thus we do not have "readers_fast", I think that "bool reader_should_block" will look better.

Quoted 2 lines

+void percpu_down_write(struct percpu_rw_semaphore *sem)
{

...

so it does

	rcu_sync_enter(&sem->rss);

	state = BLOCK;

	mb();

	wait_event(sem->writer, readers_active_check(sem));

and this looks correct.

The only nontrivial thing we need to ensure is that per_cpu_sum(*sem->refcount) == 0 can't be false positive. False negative is fine.

And this means that if we see the result of this_cpu_dec() we must not miss the result of the previous this_cpu_inc() on another CPU. same or _another_ CPU.

And this is true because if the reader does dec() on another CPU it does up_read() and this is only possible if down_read() didn't see state == BLOCK.

But if it didn't see state == BLOCK then the writer must see the result of the previous down_read()->inc().

IOW, we just rely on STORE-MB-LOAD, just the writer does LOAD multiple times in per_cpu_sum():

DOWN_WRITE: DOWN_READ on CPU X:

state = BLOCK; refcount[X]++;

mb(); mb();

sum = 0; if (state != BLOCK)
sum += refcount[0]; return; /* success* /
sum += refcount[1];
... refcount[X]--;
sum += refcount[NR_CPUS];

If the reader wins and takes the lock, then its addition to refcount[X] must be accounted by the writer.

The writer can obviously miss dec() from the reader, but we rely on wake_up/wait_event in this case.

Oleg.

#11 lore raw

Re: [RFC][PATCH 5/5] percpu-rwsem: Optimize readers and reduce global impact

Quoted 2 lines · Oleg Nesterov

Just one nit below,

Ah, wait, I forgot about the "trivial" percpu_up_write()...

Doesn't it need mb() before "state = readers_slow" to ensure "release" semantics?

Oleg.

#12 lore raw

Re: [RFC][PATCH 5/5] percpu-rwsem: Optimize readers and reduce global impact

Quoted 3 lines · Oleg Nesterov

Doesn't it need mb() before "state = readers_slow" to ensure "release" semantics?

Please don't do any mb() at all. Just do smp_store_release() if release semantics is what you want.

Basically, strive to avoid "smp_mb()" at all costs. It's insanely expensive compared to pretty much all other serialization alternatives.

(And try to make sure to pair it with smp_load_acquire() on the other side for things to make sense)

Linus

#13 lore raw

Re: [RFC][PATCH 5/5] percpu-rwsem: Optimize readers and reduce global impact

Quoted 8 lines · Linus Torvalds
Quoted 3 lines · Oleg Nesterov

Doesn't it need mb() before "state = readers_slow" to ensure "release" semantics?

Please don't do any mb() at all. Just do smp_store_release() if release semantics is what you want.

Agreed!

Quoted 2 lines

(And try to make sure to pair it with smp_load_acquire() on the other side for things to make sense)

Hmm. I think you are right...

__percpu_down_read() lacks another mb() after the "state != BLOCK" check for the same reason, and we can use smp_load_acquire(state) instead.

Oleg.

author maintainer
#14 lore raw

Re: [RFC][PATCH 5/5] percpu-rwsem: Optimize readers and reduce global impact

Quoted 8 lines · Oleg Nesterov
Quoted 4 lines · Oleg Nesterov
Quoted 3 lines

Doesn't it need mb() before "state = readers_slow" to ensure "release" semantics?

__percpu_down_read() lacks another mb() after the "state != BLOCK" check for the same reason, and we can use smp_load_acquire(state) instead.

I made the below modification to the patch.

---

kernel/locking/percpu-rwsem.c +9 -2
@@ -51,7 +51,11 @@ void __percpu_down_read(struct percpu_rw
 
 	smp_mb(); /* A matches D */
 
-	if (likely(sem->state != readers_block))
+	/*
+	 * If !readers_block the critical section starts here, matched by the
+	 * release in percpu_up_write().
+	 */
+	if (likely(smp_load_acquire(sem->state) != readers_block))
 		return;
 
 	/*

@@ -154,8 +158,11 @@ void percpu_up_write(struct percpu_rw_se
 	 * One reason that we cannot just immediately flip to readers_fast is
 	 * that new readers might fail to see the results of this writer's
 	 * critical section.
+	 *
+	 * Therefore we force it through the slow path which guarantees an
+	 * acquire and thereby guarantees the critical section's consistency.
 	 */
-	sem->state = readers_slow;
+	smp_store_release(sem->state, readers_slow);
 
 	/*
 	 * Release the write lock, this will allow readers back in the game.
#15 lore raw

Re: [RFC][PATCH 5/5] percpu-rwsem: Optimize readers and reduce global impact

In reply to Peter Zijlstra
Quoted 18 lines · Peter Zijlstra

Currently the percpu-rwsem has two issues:

- it switches to (global) atomic ops while a writer is waiting; which could be quite a while and slows down releasing the readers.

- it employs synchronize_sched_expedited() _twice_ which is evil and should die -- it shoots IPIs around the machine.

This patch cures the first problem by ordering the reader-state vs reader-count (see the comments in __percpu_down_read() and percpu_down_write()). This changes a global atomic op into a full memory barrier, which doesn't have the global cacheline contention.

It cures the second problem by employing the rcu-sync primitives by Oleg which reduces to no sync_sched() calls in the 'normal' case of no write contention -- global locks had better be rare, and has a maximum of one sync_sched() call in case of contention.

Quoted 5 lines
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/percpu-rwsem.h  |   62 +++++++++-
 kernel/locking/percpu-rwsem.c |  238 +++++++++++++++++++++---------------------
 2 files changed, 176 insertions(+), 124 deletions(-)

[ . . . ]

Quoted 15 lines
kernel/locking/percpu-rwsem.c +4 -3
@@ -8,158 +8,164 @@
 #include <linux/sched.h>
 #include <linux/errno.h>
 
-int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
+enum { readers_slow, readers_block };
+
+int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
 			const char *name, struct lock_class_key *rwsem_key)
 {
-	brw->fast_read_ctr = alloc_percpu(int);
-	if (unlikely(!brw->fast_read_ctr))
+	sem->refcount = alloc_percpu(unsigned int);

kernel/locking/percpu-rwsem.c line 21

Quoted 10 lines
kernel/locking/percpu-rwsem.c +3 -5
+	if (unlikely(!sem->refcount))
 		return -ENOMEM;
 
-	/* ->rw_sem represents the whole percpu_rw_semaphore for lockdep */
-	__init_rwsem(&brw->rw_sem, name, rwsem_key);
-	atomic_set(&brw->write_ctr, 0);
-	atomic_set(&brw->slow_read_ctr, 0);
-	init_waitqueue_head(&brw->write_waitq);
+	sem->state = readers_slow;
+	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC);

But it looks like you need the RCU-sched variant. Please see below for an untested patch providing this support. One benefit of this patch is that it does not add any bloat to Tiny RCU.

Thanx, Paul

------------------------------------------------------------------------

rcu: Add RCU-sched flavors of get-state and cond-sync

The get_state_synchronize_rcu() and cond_synchronize_rcu() functions allow polling for grace-period completion, with an actual wait for a grace period occuring only when cond_synchronize_rcu() is called too soon after the corresponding get_state_synchronize_rcu(). However, these functions work only for vanilla RCU. This commit adds the get_state_synchronize_sched() and cond_synchronize_sched(), which provide the same capability for RCU-sched.

Reported-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
include/linux/rcutiny.h +10 -0
index 3df6c1ec4e25..ff968b7af3a4 100644
@@ -37,6 +37,16 @@ static inline void cond_synchronize_rcu(unsigned long oldstate)
 	might_sleep();
 }
 
+static inline unsigned long get_state_synchronize_sched(void)
+{
+	return 0;
+}
+
+static inline void cond_synchronize_sched(unsigned long oldstate)
+{
+	might_sleep();
+}
+
 static inline void rcu_barrier_bh(void)
 {
 	wait_rcu_gp(call_rcu_bh);
include/linux/rcutree.h +2 -0
index 3fa4a43ab415..80e68d344205 100644
@@ -76,6 +76,8 @@ void rcu_barrier_bh(void);
 void rcu_barrier_sched(void);
 unsigned long get_state_synchronize_rcu(void);
 void cond_synchronize_rcu(unsigned long oldstate);
+unsigned long get_state_synchronize_sched(void);
+void cond_synchronize_sched(unsigned long oldstate);
 
 extern unsigned long rcutorture_testseq;
 extern unsigned long rcutorture_vernum;
kernel/rcu/rcutorture.c +2 -0
index 1cead7806ca6..f256dee0f6b1 100644
@@ -635,6 +635,8 @@ static struct rcu_torture_ops sched_ops = {
 	.deferred_free	= rcu_sched_torture_deferred_free,
 	.sync		= synchronize_sched,
 	.exp_sync	= synchronize_sched_expedited,
+	.get_state	= get_state_synchronize_sched,
+	.cond_sync	= cond_synchronize_sched,
 	.call		= call_rcu_sched,
 	.cb_barrier	= rcu_barrier_sched,
 	.fqs		= rcu_sched_force_quiescent_state,
kernel/rcu/tree.c +52 -0

58 lines not on this page · raw message

#16 lore raw

ring_buffer_attach && cond_synchronize_rcu (Was: percpu-rwsem: Optimize readers and reduce global impact)

Quoted 4 lines · Paul E. McKenney

But it looks like you need the RCU-sched variant. Please see below for an untested patch providing this support. One benefit of this patch is that it does not add any bloat to Tiny RCU.

I don't think so, see another email. But perhaps I am totally confused, please correct me.

Well, actually the first writer (need_sync == T) can use it, but it does not make sense, I think. Because it calls sync() right after it observes GP_IDLE and drops the lock, the window is too small.

Quoted 3 lines

------------------------------------------------------------------------

rcu: Add RCU-sched flavors of get-state and cond-sync

However, to me this patch makes sense anyway. Just I don't think rcu_sync or percpu_rw_semaphore can use the new helpers.

And. I tried to find other users of get_state/cond_sync. Found ring_buffer_attach() and it looks obviously buggy?

Again, perhaps I am totally confused, but don't we need to ensure that we have "synchronize" _between_ list_del() and list_add() ?

IOW. Suppose that ring_buffer_attach() preempts right_after get_state_synchronize_rcu() and gp completes before spin_lock().

In this case cond_synchronize_rcu() does nothing and we reuse ->rb_entry without waiting for gp in between?

Don't we need the patch below? (it also moves the ->rcu_pending check under "if (rb)", to make it more readable imo).

Peter?

Oleg.

kernel/events/core.c +7 -7
@@ -4310,20 +4310,20 @@ static void ring_buffer_attach(struct pe
 		WARN_ON_ONCE(event->rcu_pending);
 
 		old_rb = event->rb;
-		event->rcu_batches = get_state_synchronize_rcu();
-		event->rcu_pending = 1;
-
 		spin_lock_irqsave(&old_rb->event_lock, flags);
 		list_del_rcu(&event->rb_entry);
 		spin_unlock_irqrestore(&old_rb->event_lock, flags);
-	}
 
-	if (event->rcu_pending && rb) {
-		cond_synchronize_rcu(event->rcu_batches);
-		event->rcu_pending = 0;
+		event->rcu_batches = get_state_synchronize_rcu();
+		event->rcu_pending = 1;
 	}
 
 	if (rb) {
+		if (event->rcu_pending) {
+			cond_synchronize_rcu(event->rcu_batches);
+			event->rcu_pending = 0;
+		}
+
 		spin_lock_irqsave(&rb->event_lock, flags);
 		list_add_rcu(&event->rb_entry, &rb->event_list);
 		spin_unlock_irqrestore(&rb->event_lock, flags);
author maintainer
#17 lore raw

Re: ring_buffer_attach && cond_synchronize_rcu (Was: percpu-rwsem: Optimize readers and reduce global impact)

Quoted 2 lines · Oleg Nesterov

And. I tried to find other users of get_state/cond_sync. Found ring_buffer_attach() and it looks obviously buggy?

Urgh, indeed.

Quoted 5 lines

IOW. Suppose that ring_buffer_attach() preempts right_after get_state_synchronize_rcu() and gp completes before spin_lock().

In this case cond_synchronize_rcu() does nothing and we reuse ->rb_entry without waiting for gp in between?

Yes.

Quoted 15 lines

Don't we need the patch below? (it also moves the ->rcu_pending check under "if (rb)", to make it more readable imo).

kernel/events/core.c +2 -0
@@ -4310,20 +4310,20 @@ static void ring_buffer_attach(struct pe
 		WARN_ON_ONCE(event->rcu_pending);
 
 		spin_lock_irqsave(&old_rb->event_lock, flags);
 		list_del_rcu(&event->rb_entry);
 		spin_unlock_irqrestore(&old_rb->event_lock, flags);
 
+		event->rcu_batches = get_state_synchronize_rcu();
+		event->rcu_pending = 1;
 	}

kernel/events/core.c static void ring_buffer_attach(struct pe · line 4328

Quoted 10 lines
kernel/events/core.c +5 -0
 
 	if (rb) {
+		if (event->rcu_pending) {
+			cond_synchronize_rcu(event->rcu_batches);
+			event->rcu_pending = 0;
+		}
+
 		spin_lock_irqsave(&rb->event_lock, flags);
 		list_add_rcu(&event->rb_entry, &rb->event_list);
 		spin_unlock_irqrestore(&rb->event_lock, flags);

Agreed.

author maintainer
#18 lore raw

Re: ring_buffer_attach && cond_synchronize_rcu (Was: percpu-rwsem: Optimize readers and reduce global impact)

I made that the below.

Are you OK with the Changelog edits and the added SoB ?

---
Subject: perf: Fix ring_buffer_attach() RCU sync, again.
From: Oleg Nesterov <[email protected]>
Date: Sat, 30 May 2015 22:04:25 +0200

While looking for other users of get_state/cond_sync. I Found ring_buffer_attach() and it looks obviously buggy?

Don't we need to ensure that we have "synchronize" _between_ list_del() and list_add() ?

IOW. Suppose that ring_buffer_attach() preempts right_after get_state_synchronize_rcu() and gp completes before spin_lock().

In this case cond_synchronize_rcu() does nothing and we reuse ->rb_entry without waiting for gp in between?

It also moves the ->rcu_pending check under "if (rb)", to make it more readable imo.

Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc "Paul E. McKenney" <[email protected]>
Cc Alexander Shishkin <[email protected]>
Fixes b69cf53640da ("perf: Fix a race between ring_buffer_detach() and ring_buffer_attach()")
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 kernel/events/core.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
kernel/events/core.c +7 -7
@@ -4307,20 +4307,20 @@ static void ring_buffer_attach(struct pe
 		WARN_ON_ONCE(event->rcu_pending);
 
 		old_rb = event->rb;
-		event->rcu_batches = get_state_synchronize_rcu();
-		event->rcu_pending = 1;
-
 		spin_lock_irqsave(&old_rb->event_lock, flags);
 		list_del_rcu(&event->rb_entry);
 		spin_unlock_irqrestore(&old_rb->event_lock, flags);
-	}
 
-	if (event->rcu_pending && rb) {
-		cond_synchronize_rcu(event->rcu_batches);
-		event->rcu_pending = 0;
+		event->rcu_batches = get_state_synchronize_rcu();
+		event->rcu_pending = 1;
 	}
 
 	if (rb) {
+		if (event->rcu_pending) {
+			cond_synchronize_rcu(event->rcu_batches);
+			event->rcu_pending = 0;
+		}
+
 		spin_lock_irqsave(&rb->event_lock, flags);
 		list_add_rcu(&event->rb_entry, &rb->event_list);
 		spin_unlock_irqrestore(&rb->event_lock, flags);
#19 lore raw

Re: ring_buffer_attach && cond_synchronize_rcu (Was: percpu-rwsem: Optimize readers and reduce global impact)

Quoted 5 lines · Peter Zijlstra

I made that the below.

Are you OK with the Changelog edits and the added SoB ?

Of course,

Thanks Peter!

Oleg.

Quoted 66 lines

---
Subject: perf: Fix ring_buffer_attach() RCU sync, again.
From: Oleg Nesterov <[email protected]>
Date: Sat, 30 May 2015 22:04:25 +0200

While looking for other users of get_state/cond_sync. I Found ring_buffer_attach() and it looks obviously buggy?

Don't we need to ensure that we have "synchronize" _between_ list_del() and list_add() ?

IOW. Suppose that ring_buffer_attach() preempts right_after get_state_synchronize_rcu() and gp completes before spin_lock().

In this case cond_synchronize_rcu() does nothing and we reuse ->rb_entry without waiting for gp in between?

It also moves the ->rcu_pending check under "if (rb)", to make it more readable imo.

Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc "Paul E. McKenney" <[email protected]>
Cc Alexander Shishkin <[email protected]>
Fixes b69cf53640da ("perf: Fix a race between ring_buffer_detach() and ring_buffer_attach()")
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
---
 kernel/events/core.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
kernel/events/core.c +7 -7
@@ -4307,20 +4307,20 @@ static void ring_buffer_attach(struct pe
 		WARN_ON_ONCE(event->rcu_pending);
 
 		old_rb = event->rb;
-		event->rcu_batches = get_state_synchronize_rcu();
-		event->rcu_pending = 1;
-
 		spin_lock_irqsave(&old_rb->event_lock, flags);
 		list_del_rcu(&event->rb_entry);
 		spin_unlock_irqrestore(&old_rb->event_lock, flags);
-	}
 
-	if (event->rcu_pending && rb) {
-		cond_synchronize_rcu(event->rcu_batches);
-		event->rcu_pending = 0;
+		event->rcu_batches = get_state_synchronize_rcu();
+		event->rcu_pending = 1;
 	}
 
 	if (rb) {
+		if (event->rcu_pending) {
+			cond_synchronize_rcu(event->rcu_batches);
+			event->rcu_pending = 0;
+		}
+
 		spin_lock_irqsave(&rb->event_lock, flags);
 		list_add_rcu(&event->rb_entry, &rb->event_list);
 		spin_unlock_irqrestore(&rb->event_lock, flags);
merge bot
#20 lore raw

[tip:perf/urgent] perf: Fix ring_buffer_attach() RCU sync, again

In reply to Oleg Nesterov
perf: Fix ring_buffer_attach() RCU sync, again
Commit-ID:  2f993cf093643b98477c421fa2b9a98dcc940323
Gitweb:     http://git.kernel.org/tip/2f993cf093643b98477c421fa2b9a98dcc940323
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sat, 30 May 2015 22:04:25 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 19 Jun 2015 09:38:45 +0200

perf: Fix ring_buffer_attach() RCU sync, again

While looking for other users of get_state/cond_sync. I Found ring_buffer_attach() and it looks obviously buggy?

Don't we need to ensure that we have "synchronize" _between_ list_del() and list_add() ?

IOW. Suppose that ring_buffer_attach() preempts right_after get_state_synchronize_rcu() and gp completes before spin_lock().

In this case cond_synchronize_rcu() does nothing and we reuse ->rb_entry without waiting for gp in between?

It also moves the ->rcu_pending check under "if (rb)", to make it more readable imo.

Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Paul E. McKenney <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Fixes b69cf53640da ("perf: Fix a race between ring_buffer_detach() and ring_buffer_attach()")
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/events/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
kernel/events/core.c +7 -7
index eddf1ed..0ceb386 100644
@@ -4331,20 +4331,20 @@ static void ring_buffer_attach(struct perf_event *event,
 		WARN_ON_ONCE(event->rcu_pending);
 
 		old_rb = event->rb;
-		event->rcu_batches = get_state_synchronize_rcu();
-		event->rcu_pending = 1;
-
 		spin_lock_irqsave(&old_rb->event_lock, flags);
 		list_del_rcu(&event->rb_entry);
 		spin_unlock_irqrestore(&old_rb->event_lock, flags);
-	}
 
-	if (event->rcu_pending && rb) {
-		cond_synchronize_rcu(event->rcu_batches);
-		event->rcu_pending = 0;
+		event->rcu_batches = get_state_synchronize_rcu();
+		event->rcu_pending = 1;
 	}
 
 	if (rb) {
+		if (event->rcu_pending) {
+			cond_synchronize_rcu(event->rcu_batches);
+			event->rcu_pending = 0;
+		}
+
 		spin_lock_irqsave(&rb->event_lock, flags);
 		list_add_rcu(&event->rb_entry, &rb->event_list);
 		spin_unlock_irqrestore(&rb->event_lock, flags);

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

#21 lore raw
In reply to Peter Zijlstra
Quoted 6 lines · Peter Zijlstra

This is a derived work of the cpu hotplug lock rework I did in 2013 which never really went anywhere because Linus didn't like it.

This applies those same optimizations to the percpu-rwsem. Seeing how we did all the work it seemed a waste to not use it at all.

So I *still* don't like it.

We literally have one single percpu-rwsem IN THE WHOLE KERNEL TREE.

One.

Not "a couple". Not "hundreds". ONE.

And that single use that you are optimizing for isn't even explained. It's not really even clear that that thing is needed: fork() already takes the mmap_sem for writing, and I'm not at all convinced that the uprobes code couldn't just use mmap_sem for every mm it traverses, rather than use this global percpu lock that we use absolutely nowhere else.

So I'm not convinced that the right thing to do is to try to optimize that lock at ALL. I'd rather get rid of it entirely.

Is there some new use that I don't know about? Have people really looked at that uprobes code deeply? OF COURSE global locks will have problems, I'm not at all convinced that "let's make that global lock really complicated and clever" is the proper solution.

For example, the write lock is only ever taken when registering an uprobe. Not exactly likely to be timing-critical. Is there some reason why we couldn't get rid of the magical per-cpu rwsem entirely, and replace it with something truly simple like a hashed rmsem, where the readers (fork), will just read-lock one hashed rwsem, and the writer will just write-lock every hashed rwsem.

The hash might be "pick the currently executing CPU at the time of fork(), modulo 16". That one doesn't even need to disable preemption, since it doesn't actually matter that the CPU stays constant, it's just a trivial heurisitic to avoid cacheline pingpongs under very heavy fork() load.

So we'd have

    #define NR_UPROBE_RWSEM 16

    // One cacheline per rwsem.
    struct cache_line_aligned_rw_semaphore uprobe_rwsem[NR_UPROBE_RWSEM];

and fork() would basically do

    int idx = raw_smp_processor_id() & (NR_UPROBE_RWSEM-1);
    struct rw_sem *usem = &uprobe_rwsem[idx].rwsem;
     ..
    down_read(usem);
    ... do the dup_mmap() here ..
    up_read(usem);

and the uprobe register thing could just do

    for (i = 0; i < NR_UPROBE_RWSEM; i++)
        down_write(&uprobe_rwsem[i].rwsem);
    ...
    for (i = 0; ... unlock them all ..)

which doesn't look any slower than what we do now (I suspect the fork() part would be faster), and is much simpler, and would get rid of the percpu-rwsem entirely.

Hmm?

Linus

author maintainer
#22 lore raw
Quoted 1 line · Linus Torvalds

Is there some new use that I don't know about?

TJ wants to use it in cgroups.

lkml.kernel.org/r/[email protected]

maintainer
#23 lore raw
In reply to Linus Torvalds

Hello, Linus.

On Tue, May 26, 2015 at 11:12:04AM -0700, Linus Torvalds wrote:
...

Quoted 4 lines

Is there some new use that I don't know about? Have people really looked at that uprobes code deeply? OF COURSE global locks will have problems, I'm not at all convinced that "let's make that global lock really complicated and clever" is the proper solution.

I've posted a patchset to make threadgroup rwsem cgroup-specific (it is used only by cgroups) and replace the per-threadgroup rwsems with a global percpu_rwsem. This is primarily to allow either succeeding or failing migrations of multiple tasks atomically. We already try to do multiple task migrations in some corner cases and the unified hierarchy will make wider use of it. Currently, the implementation is broken as there's no easy way of reverting changes if the operation fails in the middle.

Given that task migrations aren't particularly high frequency operations, a global percpu_rwsem is likely to be workable while being less tedious and slightly less costly for the usual fork/exit paths.

That said, from cgroup's pov, write-locking per-threadgroup rwsems of all target tasks works too although we'd need an outer mutex to serialize down-write attempts to avoid locking order issues.

Thanks.

Signature · 1 line
tejun
maintainer
#24 lore raw
In reply to Linus Torvalds
Quoted 8 lines · Linus Torvalds
Quoted 6 lines · Peter Zijlstra

This is a derived work of the cpu hotplug lock rework I did in 2013 which never really went anywhere because Linus didn't like it.

This applies those same optimizations to the percpu-rwsem. Seeing how we did all the work it seemed a waste to not use it at all.

Quoted 7 lines

So I *still* don't like it.

We literally have one single percpu-rwsem IN THE WHOLE KERNEL TREE.

One.

Not "a couple". Not "hundreds". ONE.

Instead of dropping percpu-rwsem, I was thinking we could instead look for opportunities to convert new users, for instance shinkers, where the write lock is also taken just for register and unregister purposes, similar to uprobes.

Thanks,
Davidlohr

#25 lore raw
Quoted 5 lines · Davidlohr Bueso

Instead of dropping percpu-rwsem, I was thinking we could instead look for opportunities to convert new users, for instance shinkers, where the write lock is also taken just for register and unregister purposes, similar to uprobes.

So if there really are useful use cases for this, I don't object to the patch. It seems to just improve on a currently very low-usage locking primitive.

And it's not like I conceptually mind the notion of a percpu rwsem, I just hate seeing specialty locking that isn't really worth it.

Because as it is, with the current single use, I don't think it's even worth improving on.

I _would_ ask that people who are looking at this also look at our "lglock" thing. It's pretty much *exactly* the same thing, except for spinlocks, and that one too has exactly two users (the documentation states that the only user is stop_machine, but in fact file locking does too).

Because that is another example of a complete failure of a locking primitive that was just too specialized to be worth it.

Linus

#26 lore raw
Quoted 14 lines · Linus Torvalds
Quoted 5 lines · Davidlohr Bueso

Instead of dropping percpu-rwsem, I was thinking we could instead look for opportunities to convert new users, for instance shinkers, where the write lock is also taken just for register and unregister purposes, similar to uprobes.

So if there really are useful use cases for this, I don't object to the patch. It seems to just improve on a currently very low-usage locking primitive.

And it's not like I conceptually mind the notion of a percpu rwsem, I just hate seeing specialty locking that isn't really worth it.

Quoted 9 lines

Because as it is, with the current single use, I don't think it's even worth improving on.

I _would_ ask that people who are looking at this also look at our "lglock" thing. It's pretty much *exactly* the same thing, except for spinlocks, and that one too has exactly two users (the documentation states that the only user is stop_machine, but in fact file locking does too).

not sure where this would be missing:
Documentation/locking/lglock.txt
"Users: currently only the VFS and stop_machine related code"

I atleast did not find any other users as of 3.18 and in 4.0-rc5 this still seems valid.

thx!
hofrat

#27 lore raw
In reply to Linus Torvalds
Quoted 2 lines · Linus Torvalds

Because that is another example of a complete failure of a locking primitive that was just too specialized to be worth it.

<notices stale include in fs/file_table.c and removes it>

FWIW, I hadn't really looked into stop_machine uses, but fs/locks.c one is really not all that great - there we have a large trashcan of a list (every file_lock on the system) and the only use of that list is /proc/locks output generation. Sure, additions take this CPU's spinlock. And removals take pretty much a random one - losing the timeslice and regaining it on a different CPU is quite likely with the uses there.

Why do we need a global lock there, anyway? Why not hold only one for the chain currently being traversed? Sure, we'll need to get and drop them in ->next() that way; so what?

#28 lore raw
Quoted 11 lines · Al Viro

FWIW, I hadn't really looked into stop_machine uses, but fs/locks.c one is really not all that great - there we have a large trashcan of a list (every file_lock on the system) and the only use of that list is /proc/locks output generation. Sure, additions take this CPU's spinlock. And removals take pretty much a random one - losing the timeslice and regaining it on a different CPU is quite likely with the uses there.

Why do we need a global lock there, anyway? Why not hold only one for the chain currently being traversed? Sure, we'll need to get and drop them in ->next() that way; so what?

And note that fs/seq_file.c:seq_hlist_next_percpu() has no other users.

And given that locks_delete_global_locks() takes the random lock anyway, perhaps the hashed lists/locking makes no sense, I dunno.

Oleg.

#29 lore raw
Quoted 9 lines · Oleg Nesterov
Quoted 8 lines · Al Viro

FWIW, I hadn't really looked into stop_machine uses, but fs/locks.c one is really not all that great - there we have a large trashcan of a list (every file_lock on the system) and the only use of that list is /proc/locks output generation. Sure, additions take this CPU's spinlock. And removals take pretty much a random one - losing the timeslice and regaining it on a different CPU is quite likely with the uses there.

Quoted 8 lines
Quoted 3 lines

Why do we need a global lock there, anyway? Why not hold only one for the chain currently being traversed? Sure, we'll need to get and drop them in ->next() that way; so what?

And note that fs/seq_file.c:seq_hlist_next_percpu() has no other users.

And given that locks_delete_global_locks() takes the random lock anyway, perhaps the hashed lists/locking makes no sense, I dunno.

It's not about making life easier for /proc/locks; it's about not screwing those who add/remove file_lock... And no, that "random lock" isn't held when modifying the (per-cpu) lists - it protects the list hanging off each element of the global list, and /proc/locks scans those lists, so rather than taking/dropping it in each ->show(), it's taken once in ->start()...

#30 lore raw
Quoted 15 lines · Al Viro
Quoted 13 lines · Oleg Nesterov
Quoted 11 lines · Al Viro

FWIW, I hadn't really looked into stop_machine uses, but fs/locks.c one is really not all that great - there we have a large trashcan of a list (every file_lock on the system) and the only use of that list is /proc/locks output generation. Sure, additions take this CPU's spinlock. And removals take pretty much a random one - losing the timeslice and regaining it on a different CPU is quite likely with the uses there.

Why do we need a global lock there, anyway? Why not hold only one for the chain currently being traversed? Sure, we'll need to get and drop them in ->next() that way; so what?

Quoted 7 lines
Quoted 4 lines

And note that fs/seq_file.c:seq_hlist_next_percpu() has no other users.

And given that locks_delete_global_locks() takes the random lock anyway, perhaps the hashed lists/locking makes no sense, I dunno.

It's not about making life easier for /proc/locks; it's about not screwing those who add/remove file_lock...

I meant, seq_hlist_next_percpu() could be "static" in fs/locks.c.

Quoted 4 lines

And no, that "random lock" isn't held
when modifying the (per-cpu) lists - it protects the list hanging off each element of the global list, and /proc/locks scans those lists, so rather than taking/dropping it in each ->show(), it's taken once in ->start()...

Sure, I understand. I meant that (perhaps) something like

	struct {
		spinlock_t lock;
		struct list_head *head
	} file_lock_list[];


	locks_insert_global_locks(fl)
	{
		int idx = fl->idx = hash(fl);
		spin_lock(&file_lock_list[idx].lock);
		hlist_add_head(...);
		spin_unlock(...);
	}

seq_hlist_next_percpu() could scan file_lock_list[] and unlock/lock ->lock when it changes the index.

But please forget, this is really minor. Just I think that file_lock_list is not actually "per-cpu", exactly because every locks_delete_global_locks() needs lg_local_lock_cpu(fl->fl_link_cpu) as you pointed out.

Oleg.

author maintainer
#31 lore raw
In reply to Davidlohr Bueso
Quoted 4 lines · Davidlohr Bueso

Instead of dropping percpu-rwsem, I was thinking we could instead look for opportunities to convert new users, for instance shinkers, where the write lock is also taken just for register and unregister purposes, similar to uprobes.

Another 'obvious' one would be the cpu hotplug lock. That's basically the same locking primitive with just a wee little difference.

#32 lore raw
In reply to Linus Torvalds
Quoted 6 lines · Linus Torvalds
Quoted 4 lines · Peter Zijlstra

This is a derived work of the cpu hotplug lock rework I did in 2013 which never really went anywhere because Linus didn't like it.

Quoted 8 lines
Quoted 2 lines

This applies those same optimizations to the percpu-rwsem. Seeing how we did all the work it seemed a waste to not use it at all.

So I *still* don't like it.

We literally have one single percpu-rwsem IN THE WHOLE KERNEL TREE.

One.

Well. IIRC Tejun is going to turn signal_struct->group_rwsem into percpu-rwsem.

And it can have more users. Say, __sb_start_write/etc does something similar, and last time I checked this code it looked buggy to me.

Quoted 4 lines

And that single use that you are optimizing for isn't even explained. It's not really even clear that that thing is needed: fork() already takes the mmap_sem for writing, and I'm not at all convinced that the uprobes code couldn't just use mmap_sem for every mm it traverses,

I don't think we can do this... At least I do not see how.

register_for_each_vma() would need to lock all ->mmap_sem's in system to avoid the race with fork().

Oleg.

#33 lore raw
Quoted 6 lines · Oleg Nesterov
Quoted 4 lines · Linus Torvalds

We literally have one single percpu-rwsem IN THE WHOLE KERNEL TREE.

Quoted 7 lines
Quoted 1 line

One.

Well. IIRC Tejun is going to turn signal_struct->group_rwsem into percpu-rwsem.

And it can have more users. Say, __sb_start_write/etc does something similar, and last time I checked this code it looked buggy to me.

I have found my old email, see below. Perhaps this code was changed since 2013 when I sent this email, I didn't verify... but in any case this logic doesn't look simple, imo it would be nice to rely on the generic helpers from kernel/locking.

Oleg.

------------------------------------------------------------------------------
When I look at __sb_start_write/etc I am not sure this locking is correct. OK, __sb_start_write() does:

	percpu_counter_inc();

	mb();

	if (sb->s_writers.frozen)
		abort_and_retry;

freeze_super() does STORE + mb + LOAD in reverse order so either __sb_start_write() must see SB_FREEZE_WRITE or freeze_super() must see the change in ->s_writers.counter. This is correct.

Still I am not sure sb_wait_write() can trust percpu_counter_sum(), because it can also see _other_ changes.

To simplify the discussion, suppose that percpu_counter doesn't have lock/count/batch/whatever and inc/dec/sum only uses "__percpu *counters". Lets denote sb->s_writers.counter[level] as CTR[cpu].

Suppose that some thread did __sb_start_write() on CPU_1 and sleeps "forever". CTR[0] == 0, CTR_[1] == 1, freezer_super() should block.

Now:

1. freeze_super() sets SB_FREEZE_WRITE, does mb(), and starts sb_wait_write()->percpu_counter_sum().

2. __percpu_counter_sum() does for_each_online_cpu(), reads CTR[0] == 0. ret = 0.

3. Another thread comes, calls __sb_start_write() on CPU_0, increments CTR[0].

Then it notices sb->s_writers.frozen >= level and starts __sb_end_write() before retry.

Then it migrates to CPU_1. And decrements CTR[1] before __percpu_counter_sum() reads it.

So CTR[0] == 1, CTR[1] == 0. Everything is fine except sb_wait_write() has already read CTR[0].

4. __percpu_counter_sum() continues, reads CTR[1] == 0 and returns ret == 0.

sb_wait_write() returns while it should not?

#34 lore raw
In reply to Oleg Nesterov

Sorry for noise, forgot to mention...

Quoted 2 lines · Oleg Nesterov

And it can have more users.

I can be wrong and I am obviously biased... but it seems to me that even rcu_sync alone can have more users in future. This interface looks natural and simple.

Oleg.

maintainer
#35 lore raw
In reply to Peter Zijlstra

While at it, I'll go ahead and add this triviality.

From: Davidlohr Bueso <[email protected]>
Date: Tue, 26 May 2015 12:51:15 -0700
Subject: [PATCH] doc,locking: Move percpu-rwsem doc into proper subdir

Signed-off-by Davidlohr Bueso <[email protected]>
---
 Documentation/locking/percpu-rw-semaphore.txt | 27 +++++++++++++++++++++++++++
 Documentation/percpu-rw-semaphore.txt         | 27 ---------------------------
 2 files changed, 27 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/locking/percpu-rw-semaphore.txt
 delete mode 100644 Documentation/percpu-rw-semaphore.txt
Documentation/locking/percpu-rw-semaphore.txt new file +27 -0
new file mode 100644
index 0000000..7d3c824
@@ -0,0 +1,27 @@
+Percpu rw semaphores
+--------------------
+
+Percpu rw semaphores is a new read-write semaphore design that is
+optimized for locking for reading.
+
+The problem with traditional read-write semaphores is that when multiple
+cores take the lock for reading, the cache line containing the semaphore
+is bouncing between L1 caches of the cores, causing performance
+degradation.
+
+Locking for reading is very fast, it uses RCU and it avoids any atomic
+instruction in the lock and unlock path. On the other hand, locking for
+writing is very expensive, it calls synchronize_rcu() that can take
+hundreds of milliseconds.
+
+The lock is declared with "struct percpu_rw_semaphore" type.
+The lock is initialized percpu_init_rwsem, it returns 0 on success and
+-ENOMEM on allocation failure.
+The lock must be freed with percpu_free_rwsem to avoid memory leak.
+
+The lock is locked for read with percpu_down_read, percpu_up_read and
+for write with percpu_down_write, percpu_up_write.
+
+The idea of using RCU for optimized rw-lock was introduced by
+Eric Dumazet <[email protected]>.
+The code was written by Mikulas Patocka <[email protected]>
Documentation/percpu-rw-semaphore.txt deleted +0 -27
deleted file mode 100644
index 7d3c824..0000000
@@ -1,27 +0,0 @@
-Percpu rw semaphores
---------------------
-
-Percpu rw semaphores is a new read-write semaphore design that is
-optimized for locking for reading.
-
-The problem with traditional read-write semaphores is that when multiple
-cores take the lock for reading, the cache line containing the semaphore
-is bouncing between L1 caches of the cores, causing performance
-degradation.
-
-Locking for reading is very fast, it uses RCU and it avoids any atomic
-instruction in the lock and unlock path. On the other hand, locking for
-writing is very expensive, it calls synchronize_rcu() that can take
-hundreds of milliseconds.
-
-The lock is declared with "struct percpu_rw_semaphore" type.
-The lock is initialized percpu_init_rwsem, it returns 0 on success and
--ENOMEM on allocation failure.
-The lock must be freed with percpu_free_rwsem to avoid memory leak.
-
-The lock is locked for read with percpu_down_read, percpu_up_read and
-for write with percpu_down_write, percpu_up_write.
-
-The idea of using RCU for optimized rw-lock was introduced by
-Eric Dumazet <[email protected]>.
-The code was written by Mikulas Patocka <[email protected]>
Signature · 1 line
2.1.4