Skip to contentSkip to conversation

jrcu: fix broken rcu_barrier

PATCH In review Joe Korty 2 participants 6 messages Mar 14, 2011 – Mar 24, 2011

lore
Share & tools
lore

Joe Korty

author
#0 lore raw

PATCH 8/9 jrcu: fix broken rcu_barrier

jrcu: fix bug in rcu_barrier.

It is not equivalent to synchronize_rcu. But it can be implemented by a pair of synchronize_rcu's, so do so.

Thanks go to Paul E McKenney for finding this.

kernel/jrcu.c +8 -1
Index: b/kernel/jrcu.c
===================================================================
@@ -183,7 +183,7 @@ void rcu_note_might_resched(void)
 }
 EXPORT_SYMBOL(rcu_note_might_resched);
 
-void rcu_barrier(void)
+void synchronize_sched(void)
 {
 	struct rcu_synchronize rcu;
 
@@ -196,6 +196,13 @@ void rcu_barrier(void)
 	atomic_inc(&rcu_stats.nbarriers);
 
 }
+EXPORT_SYMBOL_GPL(synchronize_sched);
+
+void rcu_barrier(void)
+{
+	synchronize_sched();
+	synchronize_sched();
+}
 EXPORT_SYMBOL_GPL(rcu_barrier);
 
 void rcu_force_quiescent_state(void)
include/linux/jrcu.h +7 -6
Index: b/include/linux/jrcu.h
===================================================================
@@ -39,12 +39,13 @@ extern void rcu_barrier(void);
 #define rcu_barrier_sched			rcu_barrier
 #define rcu_barrier_bh				rcu_barrier
 
-#define synchronize_rcu				rcu_barrier
-#define synchronize_sched			rcu_barrier
-#define synchronize_sched_expedited		rcu_barrier
-#define synchronize_rcu_bh			rcu_barrier
-#define synchronize_rcu_expedited		rcu_barrier
-#define synchronize_rcu_bh_expedited		rcu_barrier
+extern void synchronize_sched(void);
+
+#define synchronize_rcu				synchronize_sched
+#define synchronize_sched_expedited		synchronize_sched
+#define synchronize_rcu_bh			synchronize_sched
+#define synchronize_rcu_expedited		synchronize_sched
+#define synchronize_rcu_bh_expedited		synchronize_sched
 
 #define rcu_init(cpu)				do { } while (0)
 #define rcu_init_sched()			do { } while (0)
#1 lore raw
Quoted 24 lines · Joe Korty

jrcu: fix bug in rcu_barrier.

It is not equivalent to synchronize_rcu. But it can be implemented by a pair of synchronize_rcu's, so do so.

Thanks go to Paul E McKenney for finding this.

Index: b/kernel/jrcu.c
===================================================================

kernel/jrcu.c +1 -1
@@ -183,7 +183,7 @@ void rcu_note_might_resched(void)
 }
 EXPORT_SYMBOL(rcu_note_might_resched);
 
-void rcu_barrier(void)
+void synchronize_sched(void)
 {
 	struct rcu_synchronize rcu;
 

kernel/jrcu.c void rcu_barrier(void) · line 205

Quoted 10 lines
kernel/jrcu.c +7 -0
@@ -196,6 +196,13 @@ void rcu_barrier(void)
 	atomic_inc(&rcu_stats.nbarriers);
 
 }
+EXPORT_SYMBOL_GPL(synchronize_sched);
+
+void rcu_barrier(void)
+{
+	synchronize_sched();
+	synchronize_sched();
+}

This is still broken. You need to wait until all pre-existing RCU callbacks on all CPUs have been invoked. Although this does guarantee that all callbacks on the current CPU have been invoked, if one of the other CPUs is busy, it might not yet have processed the callbacks from earlier grace periods. The usual way to take care of this is to post a callback on each CPU, then wait for them all to be invoked.

Thanx, Paul

Quoted 32 lines
kernel/jrcu.c +0 -0
 EXPORT_SYMBOL_GPL(rcu_barrier);
 
 void rcu_force_quiescent_state(void)

Index: b/include/linux/jrcu.h
===================================================================

include/linux/jrcu.h +7 -7
@@ -39,12 +39,13 @@ extern void rcu_barrier(void);
 #define rcu_barrier_sched			rcu_barrier
 #define rcu_barrier_bh				rcu_barrier
 
-#define synchronize_rcu				rcu_barrier
-#define synchronize_sched			rcu_barrier
-#define synchronize_sched_expedited		rcu_barrier
-#define synchronize_rcu_bh			rcu_barrier
-#define synchronize_rcu_expedited		rcu_barrier
-#define synchronize_rcu_bh_expedited		rcu_barrier
+extern void synchronize_sched(void);
+
+#define synchronize_rcu				synchronize_sched
+#define synchronize_sched_expedited		synchronize_sched
+#define synchronize_rcu_bh			synchronize_sched
+#define synchronize_rcu_expedited		synchronize_sched
+#define synchronize_rcu_bh_expedited		synchronize_sched
 
 #define rcu_init(cpu)				do { } while (0)
 #define rcu_init_sched()			do { } while (0)
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

author
#2 lore raw
Quoted 13 lines · Paul E. McKenney

kernel/jrcu.c void rcu_barrier(void) · line 205

Quoted 5 lines · Joe Korty
kernel/jrcu.c +5 -0
+void rcu_barrier(void)
+{
+	synchronize_sched();
+	synchronize_sched();
+}

This is still broken. You need to wait until all pre-existing RCU callbacks on all CPUs have been invoked. Although this does guarantee that all callbacks on the current CPU have been invoked, if one of the other CPUs is busy, it might not yet have processed the callbacks from earlier grace periods. The usual way to take care of this is to post a callback on each CPU, then wait for them all to be invoked.

Hi Paul,
JRCU (eventually) chains together all callbacks from all cpus into a single queue and then it expires that single queue. So the double synchronize_sched() should work as a rcu_barrier implementation.

Thanks,
Joe

#3 lore raw
Quoted 15 lines · Joe Korty
Quoted 13 lines · Paul E. McKenney

kernel/jrcu.c void rcu_barrier(void) · line 205

Quoted 5 lines · Joe Korty
kernel/jrcu.c +5 -0
+void rcu_barrier(void)
+{
+	synchronize_sched();
+	synchronize_sched();
+}

This is still broken. You need to wait until all pre-existing RCU callbacks on all CPUs have been invoked. Although this does guarantee that all callbacks on the current CPU have been invoked, if one of the other CPUs is busy, it might not yet have processed the callbacks from earlier grace periods. The usual way to take care of this is to post a callback on each CPU, then wait for them all to be invoked.

Quoted 5 lines

Hi Paul,
JRCU (eventually) chains together all callbacks from all cpus into a single queue and then it expires that single queue. So the double synchronize_sched() should work as a rcu_barrier implementation.

Hello, Joe,

So the following grace period is not permitted to start until all of the callbacks from the prior grace period have been invoked?

Or am I missing something subtle here?

Thanx, Paul

author
#4 lore raw
Quoted 22 lines · Paul E. McKenney
Quoted 15 lines · Joe Korty
Quoted 13 lines · Paul E. McKenney

kernel/jrcu.c void rcu_barrier(void) · line 205

Quoted 5 lines · Joe Korty
kernel/jrcu.c +5 -0
+void rcu_barrier(void)
+{
+	synchronize_sched();
+	synchronize_sched();
+}

This is still broken. You need to wait until all pre-existing RCU callbacks on all CPUs have been invoked. Although this does guarantee that all callbacks on the current CPU have been invoked, if one of the other CPUs is busy, it might not yet have processed the callbacks from earlier grace periods. The usual way to take care of this is to post a callback on each CPU, then wait for them all to be invoked.

Quoted 5 lines

Hi Paul,
JRCU (eventually) chains together all callbacks from all cpus into a single queue and then it expires that single queue. So the double synchronize_sched() should work as a rcu_barrier implementation.

Quoted 4 lines

Hello, Joe,

So the following grace period is not permitted to start until all of the callbacks from the prior grace period have been invoked?

Yes, it is all synchronized. Every cpu's current batch is closed off at the same time. We tolerate some fuzz in the timing, but that is not the same as being unsynchronized.

This happens as an automatic side effect of doing everything (except for call_rcu) from a single CPU.

Joe

#5 lore raw
Quoted 28 lines · Joe Korty
Quoted 22 lines · Paul E. McKenney
Quoted 15 lines · Joe Korty
Quoted 13 lines · Paul E. McKenney

kernel/jrcu.c void rcu_barrier(void) · line 205

Quoted 5 lines · Joe Korty
kernel/jrcu.c +5 -0
+void rcu_barrier(void)
+{
+	synchronize_sched();
+	synchronize_sched();
+}

This is still broken. You need to wait until all pre-existing RCU callbacks on all CPUs have been invoked. Although this does guarantee that all callbacks on the current CPU have been invoked, if one of the other CPUs is busy, it might not yet have processed the callbacks from earlier grace periods. The usual way to take care of this is to post a callback on each CPU, then wait for them all to be invoked.

Quoted 5 lines

Hi Paul,
JRCU (eventually) chains together all callbacks from all cpus into a single queue and then it expires that single queue. So the double synchronize_sched() should work as a rcu_barrier implementation.

Quoted 4 lines

Hello, Joe,

So the following grace period is not permitted to start until all of the callbacks from the prior grace period have been invoked?

Quoted 6 lines

Yes, it is all synchronized. Every cpu's current batch is closed off at the same time. We tolerate some fuzz in the timing, but that is not the same as being unsynchronized.

This happens as an automatic side effect of doing everything (except for call_rcu) from a single CPU.

Got it -- the rcu_delimit_batches() doesn't return until all callbacks have been invoked, and a new grace period cannot start until this function returns. I am not yet sure whether this is a good thing or a bad thing. Given your 50-millisecond grace periods, it is less of a bad thing than it would be for shorter grace periods, I suppose. ;-)

Thanx, Paul