using lockdep to validate rcu usage

lockdep annotate rcu_read_{,un}lock{,_bh} in order to catch imbalanced usage.

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/lockdep.h  |    7 +++++++
 include/linux/rcupdate.h |   12 ++++++++++++
 kernel/rcupdate.c        |    8 ++++++++
 3 files changed, 27 insertions(+)

Index: linux-2.6/include/linux/lockdep.h
===================================================================

include/linux/lockdep.h +7 -0

@@ -252,6 +252,13 @@ extern void lockdep_init_map(struct lock
 			     struct lock_class_key *key, int subclass);
 
 /*
+ * To initialize a lockdep_map statically use this macro.
+ * Note that _name must not be NULL.
+ */
+#define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
+	{ .name = (_name), .key = (void *)(_key), }
+
+/*
  * Reinitialize a lock key - for cases where there is special locking or
  * special initialization of locks so that the validator gets the scope
  * of dependencies wrong: they are either too broad (they need a class-split)

Index: linux-2.6/include/linux/rcupdate.h
===================================================================

include/linux/rcupdate.h +14 -0

@@ -41,6 +41,7 @@
 #include <linux/percpu.h>
 #include <linux/cpumask.h>
 #include <linux/seqlock.h>
+#include <linux/lockdep.h>
 
 /**
  * struct rcu_head - callback structure for use with RCU

@@ -133,6 +134,15 @@ static inline void rcu_bh_qsctr_inc(int 
 extern int rcu_pending(int cpu);
 extern int rcu_needs_cpu(int cpu);
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+extern struct lockdep_map rcu_lock_map;
+# define rcu_read_acquire()	lock_acquire(&rcu_lock_map, 0, 0, 2, 1, _THIS_IP_)
+# define rcu_read_release()	lock_release(&rcu_lock_map, 1, _THIS_IP_)
+#else
+# define rcu_read_acquire()	do { } while (0)
+# define rcu_read_release()	do { } while (0)
+#endif
+
 /**
  * rcu_read_lock - mark the beginning of an RCU read-side critical section.
  *

@@ -166,6 +176,7 @@ extern int rcu_needs_cpu(int cpu);
 	do { \
 		preempt_disable(); \
 		__acquire(RCU); \
+		rcu_read_acquire(); \
 	} while(0)
 
 /**

@@ -175,6 +186,7 @@ extern int rcu_needs_cpu(int cpu);
  */
 #define rcu_read_unlock() \
 	do { \
+		rcu_read_release(); \
 		__release(RCU); \
 		preempt_enable(); \
 	} while(0)

@@ -216,6 +218,7 @@ extern struct lockdep_map rcu_lock_map;
 	do { \
 		local_bh_disable(); \
 		__acquire(RCU_BH); \
+		rcu_read_acquire(); \
 	} while(0)
 
 /*

@@ -225,6 +228,7 @@ extern struct lockdep_map rcu_lock_map;
  */
 #define rcu_read_unlock_bh() \
 	do { \
+		rcu_read_release(); \
 		__release(RCU_BH); \
 		local_bh_enable(); \
 	} while(0)

Index: linux-2.6/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +8 -1

@@ -48,6 +48,14 @@
 #include <linux/cpu.h>
 #include <linux/mutex.h>
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static struct lock_class_key rcu_lock_key;
+struct lockdep_map rcu_lock_map =
+	STATIC_LOCKDEP_MAP_INIT("rcu_read_lock", &rcu_lock_key);
+
+EXPORT_SYMBOL_GPL(rcu_lock_map);
+#endif
+
 /* Definition for rcupdate control block. */
 static struct rcu_ctrlblk rcu_ctrlblk = {
 	.cur = -300,
 
--

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

Quoted 17 lines · Peter Zijlstra

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Quoted 4 lines

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

OK, then it goes beyond RCU... We need to come up with something that can be used to synchronize with IRQ handlers (quite often in driver code one needs to be sure that current invocation of IRQ handler completed before doing something). And once we have it splinlock + RCU users can just use that method.

Quoted 17 lines · Peter Zijlstra

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Quoted 3 lines

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

OK, then it goes beyond RCU... We need to come up with something that can be used to synchronize with IRQ handlers (quite often in driver code one needs to be sure that current invocation of IRQ handler completed before doing something). And once we have it splinlock + RCU users can just use that method.

Quoted 22 lines · Dmitry Torokhov

Quoted 20 lines · Peter Zijlstra

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

Quoted 5 lines

OK, then it goes beyond RCU... We need to come up with something that can be used to synchronize with IRQ handlers (quite often in driver code one needs to be sure that current invocation of IRQ handler completed before doing something). And once we have it splinlock + RCU users can just use that method.

But Peter's approach would not cause a problem here -- you wouldn't be doing an rcu_dereference from within the IRQ handler in this case, right?

That said, we will need something to handle threaded interrupts, since synchronize_sched() only waits for hardirq, NMI, SMI, etc., and not threaded IRQs.

Thanx, Paul

Quoted 29 lines · Paul E. McKenney

Quoted 27 lines · Dmitry Torokhov

Quoted 17 lines · Peter Zijlstra

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Quoted 3 lines

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

OK, then it goes beyond RCU... We need to come up with something that can be used to synchronize with IRQ handlers (quite often in driver code one needs to be sure that current invocation of IRQ handler completed before doing something). And once we have it splinlock + RCU users can just use that method.

Quoted 2 lines

But Peter's approach would not cause a problem here -- you wouldn't be doing an rcu_dereference from within the IRQ handler in this case, right?

Yes I do. Along with list_for_each_rcu().

Quoted 6 lines

That said, we will need something to handle threaded interrupts, since synchronize_sched() only waits for hardirq, NMI, SMI, etc., and not threaded IRQs.

Thanx, Paul

Signature · 1 line

Dmitry

Quoted 30 lines · Dmitry Torokhov

Quoted 29 lines · Paul E. McKenney

Quoted 27 lines · Dmitry Torokhov

Quoted 17 lines · Peter Zijlstra

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Quoted 3 lines

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

OK, then it goes beyond RCU... We need to come up with something that can be used to synchronize with IRQ handlers (quite often in driver code one needs to be sure that current invocation of IRQ handler completed before doing something). And once we have it splinlock + RCU users can just use that method.

Quoted 4 lines

Quoted 2 lines

But Peter's approach would not cause a problem here -- you wouldn't be doing an rcu_dereference from within the IRQ handler in this case, right?

Yes I do. Along with list_for_each_rcu().

OK, in that case it does indeed need to be handled.

Quoted 34 lines · Paul E. McKenney

Quoted 33 lines · Dmitry Torokhov

Quoted 31 lines · Paul E. McKenney

Quoted 22 lines · Dmitry Torokhov

Quoted 17 lines · Peter Zijlstra

Quoted 6 lines · On Wed, 19 Sep 2007 10:17:25 -0400 "Dmitry Torokhov"…

Hi Peter,

Quoted 2 lines · Peter Zijlstra

Warn when rcu_dereference() is not used in combination with rcu_read_lock()

Quoted 7 lines

According to Paul it is fine to use RCU primitives (when accompanied with proper comments) when the read-size critical section is guarded by spin_lock_irqsave()/spin_lock_irqsrestore() instead of rcu_read_lock()/rcu_read_unlock() and writers synchronize with synchronize_sched(), not synchronize_rcu(). Your patch will trigger warnign on such valid usages.

Quoted 3 lines

Sounds fragile to begin with. But you're right in that that is valid for Linux as you know it. However in -rt most/all spinlocks are converted to sleeping locks. In that case sync_sched() is not enough.

Quoted 5 lines

OK, then it goes beyond RCU... We need to come up with something that can be used to synchronize with IRQ handlers (quite often in driver code one needs to be sure that current invocation of IRQ handler completed before doing something). And once we have it splinlock + RCU users can just use that method.

But Peter's approach would not cause a problem here -- you wouldn't be doing an rcu_dereference from within the IRQ handler in this case, right?

Quoted 3 lines

Quoted 1 line

Yes I do. Along with list_for_each_rcu().

OK, in that case it does indeed need to be handled.

PS to previous -- any problem with inserting rcu_read_lock() and rcu_read_unlock() around the portion of the IRQ handler that has these accesses?

Quoted 4 lines · Paul E. McKenney

PS to previous -- any problem with inserting rcu_read_lock() and rcu_read_unlock() around the portion of the IRQ handler that has these accesses?

I guess I could but it is an extra lock that needs to be managed and given the fact that it is not really needed (other to make a newly developed tool happy) I am hestsant to do that.

Quoted 7 lines · On Wed, 19 Sep 2007 14:49:56 -0400 "Dmitry Torokhov"…

Quoted 4 lines · Paul E. McKenney

PS to previous -- any problem with inserting rcu_read_lock() and rcu_read_unlock() around the portion of the IRQ handler that has these accesses?

Quoted 3 lines

I guess I could but it is an extra lock that needs to be managed and given the fact that it is not really needed (other to make a newly developed tool happy) I am hestsant to do that.

As is, these sites are a bug in -rt and we'll need to fix them anyway.

As for the code you pointed me to, the i8042 driver, it seems to play way to funny tricks for a simple 'slow' driver.

If you replace the spin_lock() + sync_sched(), with rcu_read_lock() + rcu_call() it should work again without adding an extra lock.

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · On Wed, 19 Sep 2007 14:49:56 -0400 "Dmitry Torokhov"…

Quoted 4 lines · Paul E. McKenney

PS to previous -- any problem with inserting rcu_read_lock() and rcu_read_unlock() around the portion of the IRQ handler that has these accesses?

Quoted 8 lines

Quoted 3 lines

I guess I could but it is an extra lock that needs to be managed and given the fact that it is not really needed (other to make a newly developed tool happy) I am hestsant to do that.

As is, these sites are a bug in -rt and we'll need to fix them anyway.

As for the code you pointed me to, the i8042 driver, it seems to play way to funny tricks for a simple 'slow' driver.

Even "slow" driver should try not to slow down the rest of the system if it can help it. I am sorry if the thing it does do not quite fit in with the changes you are proposing but it does not make the exeisting code invalid.

Quoted 1 line

Quoted 3 lines

If you replace the spin_lock() + sync_sched(), with rcu_read_lock() + rcu_call() it should work again without adding an extra lock.

Except that I need spin_lock_irq for other reasons. I could take the same lock in write-side code and not use RCU at all but using RCU allows opening/closing input devices without slowing down interrupt handlers so why not use it?

Quoted 26 lines · On Wed, 19 Sep 2007 15:49:24 -0400 "Dmitry Torokhov"…

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · On Wed, 19 Sep 2007 14:49:56 -0400 "Dmitry Torokhov"…

Quoted 4 lines · Paul E. McKenney

PS to previous -- any problem with inserting rcu_read_lock() and rcu_read_unlock() around the portion of the IRQ handler that has these accesses?

Quoted 8 lines

Quoted 3 lines

I guess I could but it is an extra lock that needs to be managed and given the fact that it is not really needed (other to make a newly developed tool happy) I am hestsant to do that.

As is, these sites are a bug in -rt and we'll need to fix them anyway.

As for the code you pointed me to, the i8042 driver, it seems to play way to funny tricks for a simple 'slow' driver.

Even "slow" driver should try not to slow down the rest of the system if it can help it. I am sorry if the thing it does do not quite fit in with the changes you are proposing but it does not make the exeisting code invalid.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

If you replace the spin_lock() + sync_sched(), with rcu_read_lock() + rcu_call() it should work again without adding an extra lock.

Except that I need spin_lock_irq for other reasons. I could take the same lock in write-side code and not use RCU at all but using RCU allows opening/closing input devices without slowing down interrupt handlers so why not use it?

If the IRQ handler does rcu_read_lock(),unlock() and the i8042_stop() function does sync_rcu() instead of _sched(), it should be good again. It will not affect anything else than the task that calls _stop(). And even there the only change is that the sleep might be a tad longer.

I find it curious that a driver that is 'low performant' and does not suffer lock contention pioneers locking schemes. I agree with optimizing, but this is not the place to push the envelope.

Quoted 4 lines

If the IRQ handler does rcu_read_lock(),unlock() and the i8042_stop() function does sync_rcu() instead of _sched(), it should be good again. It will not affect anything else than the task that calls _stop(). And even there the only change is that the sleep might be a tad longer.

And the IRQ handler needs to do some extra job... Anyway, it looks -rt breaks synchronize_sched() and needs to have it fixed:

"/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
* This means that all preempt_disable code sequences, including NMI and
* hardware-interrupt handlers, in progress on entry will have completed
* before this primitive returns."

Quoted 3 lines

I find it curious that a driver that is 'low performant' and does not suffer lock contention pioneers locking schemes. I agree with optimizing, but this is not the place to push the envelope.

Please realize that evey microsecond wasted on a 'low performant' driver is taken from high performers and if we can help it why shouldn't we?

Quoted 8 lines · On Wed, 19 Sep 2007 16:41:04 -0400 "Dmitry Torokhov"…

Quoted 4 lines

If the IRQ handler does rcu_read_lock(),unlock() and the i8042_stop() function does sync_rcu() instead of _sched(), it should be good again. It will not affect anything else than the task that calls _stop(). And even there the only change is that the sleep might be a tad longer.

And the IRQ handler needs to do some extra job... Anyway, it looks -rt breaks synchronize_sched() and needs to have it fixed:

Quoted 7 lines

"/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
* This means that all preempt_disable code sequences, including NMI and
* hardware-interrupt handlers, in progress on entry will have completed
* before this primitive returns."

That still does as it says in -rt. Its just that the interrupt handler will be preemptible so the guarantees it gives are useless.

Quoted 8 lines

Quoted 4 lines

I find it curious that a driver that is 'low performant' and does not suffer lock contention pioneers locking schemes. I agree with optimizing, but this is not the place to push the envelope.

Please realize that evey microsecond wasted on a 'low performant' driver is taken from high performers and if we can help it why shouldn't we?

sure, but the cache eviction caused by running the driver will have more impact than the added rcu_read_{,un}lock() calls.

Quoted 19 lines · Peter Zijlstra

Quoted 15 lines · On Wed, 19 Sep 2007 16:41:04 -0400 "Dmitry Torokhov"…

Quoted 4 lines

If the IRQ handler does rcu_read_lock(),unlock() and the i8042_stop() function does sync_rcu() instead of _sched(), it should be good again. It will not affect anything else than the task that calls _stop(). And even there the only change is that the sleep might be a tad longer.

And the IRQ handler needs to do some extra job... Anyway, it looks -rt breaks synchronize_sched() and needs to have it fixed:

"/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
* This means that all preempt_disable code sequences, including NMI and
* hardware-interrupt handlers, in progress on entry will have completed
* before this primitive returns."

Quoted 2 lines

That still does as it says in -rt. Its just that the interrupt handler will be preemptible so the guarantees it gives are useless.

Please note "... including NMI and hardware-interrupt handlers ..."

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

I find it curious that a driver that is 'low performant' and does not suffer lock contention pioneers locking schemes. I agree with optimizing, but this is not the place to push the envelope.

Please realize that evey microsecond wasted on a 'low performant' driver is taken from high performers and if we can help it why shouldn't we?

Quoted 2 lines

sure, but the cache eviction caused by running the driver will have more impact than the added rcu_read_{,un}lock() calls.

Are you saying that adding rcu_read_{,un}lock() will help with cache eviction? How?

Quoted 20 lines · On Wed, 19 Sep 2007 17:29:09 -0400 "Dmitry Torokhov"…

Quoted 19 lines · Peter Zijlstra

Quoted 15 lines · On Wed, 19 Sep 2007 16:41:04 -0400 "Dmitry Torokhov"…

Quoted 4 lines

If the IRQ handler does rcu_read_lock(),unlock() and the i8042_stop() function does sync_rcu() instead of _sched(), it should be good again. It will not affect anything else than the task that calls _stop(). And even there the only change is that the sleep might be a tad longer.

And the IRQ handler needs to do some extra job... Anyway, it looks -rt breaks synchronize_sched() and needs to have it fixed:

"/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
* This means that all preempt_disable code sequences, including NMI and
* hardware-interrupt handlers, in progress on entry will have completed
* before this primitive returns."

Quoted 4 lines

Quoted 2 lines

That still does as it says in -rt. Its just that the interrupt handler will be preemptible so the guarantees it gives are useless.

Please note "... including NMI and hardware-interrupt handlers ..."

-rt doesn't run interrupt handlers in hardware irq context anymore.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

I find it curious that a driver that is 'low performant' and does not suffer lock contention pioneers locking schemes. I agree with optimizing, but this is not the place to push the envelope.

Please realize that evey microsecond wasted on a 'low performant' driver is taken from high performers and if we can help it why shouldn't we?

Quoted 5 lines

Quoted 2 lines

sure, but the cache eviction caused by running the driver will have more impact than the added rcu_read_{,un}lock() calls.

Are you saying that adding rcu_read_{,un}lock() will help with cache eviction? How?

No, I'm saying that its noise compared to the cache eviction overhead it causes for others.

Quoted 23 lines · Peter Zijlstra

Quoted 20 lines · On Wed, 19 Sep 2007 17:29:09 -0400 "Dmitry Torokhov"…

Quoted 19 lines · Peter Zijlstra

Quoted 15 lines · On Wed, 19 Sep 2007 16:41:04 -0400 "Dmitry Torokhov"…

Quoted 4 lines

If the IRQ handler does rcu_read_lock(),unlock() and the i8042_stop() function does sync_rcu() instead of _sched(), it should be good again. It will not affect anything else than the task that calls _stop(). And even there the only change is that the sleep might be a tad longer.

And the IRQ handler needs to do some extra job... Anyway, it looks -rt breaks synchronize_sched() and needs to have it fixed:

"/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
* This means that all preempt_disable code sequences, including NMI and
* hardware-interrupt handlers, in progress on entry will have completed
* before this primitive returns."

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

That still does as it says in -rt. Its just that the interrupt handler will be preemptible so the guarantees it gives are useless.

Please note "... including NMI and hardware-interrupt handlers ..."

-rt doesn't run interrupt handlers in hardware irq context anymore.

OK, then what is the purpose of synchronize_sched() in -rt?

You really need to provide users with a replacement. There are several drivers that use it and for example r8169 is not what you'd call a 'low performer'.

I guess I can switch i8042 to use synchronize_irq(). That still works in -rt, doesn't it? That still leaves atkbd...

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

I find it curious that a driver that is 'low performant' and does not suffer lock contention pioneers locking schemes. I agree with optimizing, but this is not the place to push the envelope.

Please realize that evey microsecond wasted on a 'low performant' driver is taken from high performers and if we can help it why shouldn't we?

sure, but the cache eviction caused by running the driver will have more impact than the added rcu_read_{,un}lock() calls.

Quoted 5 lines

Quoted 2 lines

Are you saying that adding rcu_read_{,un}lock() will help with cache eviction? How?

No, I'm saying that its noise compared to the cache eviction overhead it causes for others.

What about udelay(10)? It is probably also a noise but we shoudl not go and sprinkle it through drivers, should we? ;)

Quoted 2 lines · Dmitry Torokhov

OK, then what is the purpose of synchronize_sched() in -rt?

To wait for all preempt-disable, irq-disable, hard-irq, and SMI/NMI code sequences to complete.

Quoted 4 lines

You really need to provide users with a replacement. There are several drivers that use it and for example r8169 is not what you'd call a 'low performer'.

I did look at making a synchronize_all_irq() some time back, and all the approaches I came up with at the time were busted.

But I just took another look, and I think I see a way to handle it. Either that, or I simply forgot the way in which this approach is broken...

I will stare at is some more.

Quoted 2 lines

I guess I can switch i8042 to use synchronize_irq(). That still works in -rt, doesn't it? That still leaves atkbd...

Yep, looks that way to me. The only difference that I can see is that in -rt, concurrent synchronize_irq() calls on the same descriptor mean that the guy that gets there second has to wait for the next interrupt to happen.

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines · Dmitry Torokhov

OK, then what is the purpose of synchronize_sched() in -rt?

To wait for all preempt-disable, irq-disable, hard-irq, and SMI/NMI code sequences to complete.

OK, so what spin_lock_irq[save]? Does is disable IRQs in -rt or not anymore? If IRQs are disabled it appears that I can continue using synchronize_sched().

Quoted 8 lines

Quoted 4 lines

You really need to provide users with a replacement. There are several drivers that use it and for example r8169 is not what you'd call a 'low performer'.

I did look at making a synchronize_all_irq() some time back, and all the approaches I came up with at the time were busted.

Quoted 6 lines

But I just took another look, and I think I see a way to handle it. Either that, or I simply forgot the way in which this approach is broken...

I will stare at is some more.

Thank you.

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · On Wed, 19 Sep 2007 14:49:56 -0400 "Dmitry Torokhov"…

Quoted 4 lines · Paul E. McKenney

PS to previous -- any problem with inserting rcu_read_lock() and rcu_read_unlock() around the portion of the IRQ handler that has these accesses?

Quoted 8 lines

Quoted 3 lines

I guess I could but it is an extra lock that needs to be managed and given the fact that it is not really needed (other to make a newly developed tool happy) I am hestsant to do that.

As is, these sites are a bug in -rt and we'll need to fix them anyway.

As for the code you pointed me to, the i8042 driver, it seems to play way to funny tricks for a simple 'slow' driver.

Even "slow" driver should try not to slow down the rest of the system if it can help it. I am sorry if the thing it does do not quite fit in with the changes you are proposing but it does not make the exeisting code invalid.

Quoted 2 lines

If you replace the spin_lock() + sync_sched(), with rcu_read_lock() + rcu_call() it should work again without adding an extra lock.

Except that I need spin_lock_irq for other reasons. I could take the same lock in write-side code and not use RCU at all but using RCU allows opening/closing input devices without slowing down interrupt handlers so why not use it?

This patch set uses lockdep to validate rcu usage.

It annotates rcu_read_{,un}lock{,_bh}() to catch imbalances. And further uses that information to establish a proper context for rcu_dereference().

It also separates implicit from explicit preempt_disable() usage, in order to separate rcu_dereference() from the locking model.

A kernel (2.6.23-rc4-mm1) with these patches boots but does have some funnies - I suspect it calls printf from places it doesn't like.

The first patch should be safe to apply, the rest is RFC.