Skip to contentSkip to conversation

rcu: remove the context test in rcu_read_unlock_special()

PATCH In review Lai Jiangshan 2 participants 4 messages Mar 29, 2012 – Apr 2, 2012

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rcu: remove the context test in rcu_read_unlock_special()

Because of commit#26861f and #10f39bb1b2, the context test in rcu_read_unlock_special() are not needed.

commit#26861f and #10f39bb1b2 ensure ->rcu_read_unlock_special is zero after rcu_read_unlock(), which implies:

if ->rcu_read_unlock_special is non-zero, the out-most RCU C.S. can be interrupted or can be preempted, such context is safe to call rcu_read_unlock_special().

---

kernel/rcutiny_plugin.h +0 -6
index 22ecea0..22e2ef1 100644
@@ -571,12 +571,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 	if (special & RCU_READ_UNLOCK_NEED_QS)
 		rcu_preempt_cpu_qs();
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
kernel/rcutree_plugin.h +0 -6
index c023464..2bd3689 100644
@@ -338,12 +338,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 		rcu_preempt_qs(smp_processor_id());
 	}
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
kernel/softirq.c +1 -5
index 671f959..7074f97 100644
@@ -318,12 +318,8 @@ static inline void invoke_softirq(void)
 #else
 		do_softirq();
 #endif
-	} else {
-		__local_bh_disable((unsigned long)__builtin_return_address(0),
-				SOFTIRQ_OFFSET);
+	} else
 		wakeup_softirqd();
-		__local_bh_enable(SOFTIRQ_OFFSET);
-	}
 }
 
 /*
#1 lore raw
Quoted 9 lines · Lai Jiangshan

Because of commit#26861f and #10f39bb1b2, the context test in rcu_read_unlock_special() are not needed.

commit#26861f and #10f39bb1b2 ensure ->rcu_read_unlock_special is zero after rcu_read_unlock(), which implies:

if ->rcu_read_unlock_special is non-zero, the out-most RCU C.S. can be interrupted or can be preempted, such context is safe to call rcu_read_unlock_special().

In theory, I am very much in favor of this, but in practice we need to put a WARN_ON_ONCE() in the "if" statement to be removed for a few years first. If there are never any warnings (or, more likely, once such warnings have been fixed for some time), then this patch below would be eminently sensible.

Thanx, Paul

Quoted 55 lines

---

kernel/rcutiny_plugin.h +0 -6
index 22ecea0..22e2ef1 100644
@@ -571,12 +571,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 	if (special & RCU_READ_UNLOCK_NEED_QS)
 		rcu_preempt_cpu_qs();
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
kernel/rcutree_plugin.h +0 -6
index c023464..2bd3689 100644
@@ -338,12 +338,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 		rcu_preempt_qs(smp_processor_id());
 	}
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
kernel/softirq.c +1 -5
index 671f959..7074f97 100644
@@ -318,12 +318,8 @@ static inline void invoke_softirq(void)
 #else
 		do_softirq();
 #endif
-	} else {
-		__local_bh_disable((unsigned long)__builtin_return_address(0),
-				SOFTIRQ_OFFSET);
+	} else
 		wakeup_softirqd();
-		__local_bh_enable(SOFTIRQ_OFFSET);
-	}
 }
 
 /*
#2 lore raw
Quoted 11 lines · Paul E. McKenney
Quoted 9 lines · Lai Jiangshan

Because of commit#26861f and #10f39bb1b2, the context test in rcu_read_unlock_special() are not needed.

commit#26861f and #10f39bb1b2 ensure ->rcu_read_unlock_special is zero after rcu_read_unlock(), which implies:

if ->rcu_read_unlock_special is non-zero, the out-most RCU C.S. can be interrupted or can be preempted, such context is safe to call rcu_read_unlock_special().

Quoted 6 lines

In theory, I am very much in favor of this, but in practice we need to put a WARN_ON_ONCE() in the "if" statement to be removed for a few years first. If there are never any warnings (or, more likely, once such warnings have been fixed for some time), then this patch below would be eminently sensible.

Add BUG_ON() instead, how about it?
Thanks,
Lai

kernel/rcutiny_plugin.h +0 -6
index 22ecea0..22e2ef1 100644
@@ -571,12 +571,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 	if (special & RCU_READ_UNLOCK_NEED_QS)
 		rcu_preempt_cpu_qs();
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
kernel/rcutree_plugin.h +6 -7
index c023464..e90593e 100644
@@ -210,6 +210,7 @@ static void rcu_preempt_note_context_switch(int cpu)
 		raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	} else if (t->rcu_read_lock_nesting < 0 &&
 		   t->rcu_read_unlock_special) {
+		WARN_ON_ONCE(t->rcu_read_lock_nesting != INT_MIN);
 
 		/*
 		 * Complete exit from RCU read-side critical section on
@@ -239,7 +240,10 @@ static void rcu_preempt_note_context_switch(int cpu)
  */
 void __rcu_read_lock(void)
 {
-	current->rcu_read_lock_nesting++;
+	struct task_struct *t = current;
+
+	BUG_ON(!t->rcu_read_lock_nesting && t->rcu_read_unlock_special);
+	t->rcu_read_lock_nesting++;
 	barrier();  /* needed if we ever invoke rcu_read_lock in rcutree.c */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_lock);
@@ -338,12 +342,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 		rcu_preempt_qs(smp_processor_id());
 	}
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
@@ -439,6 +437,7 @@ void __rcu_read_unlock(void)
 			rcu_read_unlock_special(t);
 		barrier();  /* ->rcu_read_unlock_special load before assign */
 		t->rcu_read_lock_nesting = 0;
+		BUG_ON(t->rcu_read_unlock_special);
 	}
 #ifdef CONFIG_PROVE_LOCKING
 	{
kernel/softirq.c +1 -5
index 671f959..7074f97 100644
@@ -318,12 +318,8 @@ static inline void invoke_softirq(void)
 #else
 		do_softirq();
 #endif
-	} else {
-		__local_bh_disable((unsigned long)__builtin_return_address(0),
-				SOFTIRQ_OFFSET);
+	} else
 		wakeup_softirqd();
-		__local_bh_enable(SOFTIRQ_OFFSET);
-	}
 }
 
 /*
#3 lore raw
Quoted 18 lines · Lai Jiangshan
Quoted 11 lines · Paul E. McKenney
Quoted 9 lines · Lai Jiangshan

Because of commit#26861f and #10f39bb1b2, the context test in rcu_read_unlock_special() are not needed.

commit#26861f and #10f39bb1b2 ensure ->rcu_read_unlock_special is zero after rcu_read_unlock(), which implies:

if ->rcu_read_unlock_special is non-zero, the out-most RCU C.S. can be interrupted or can be preempted, such context is safe to call rcu_read_unlock_special().

Quoted 5 lines

In theory, I am very much in favor of this, but in practice we need to put a WARN_ON_ONCE() in the "if" statement to be removed for a few years first. If there are never any warnings (or, more likely, once such warnings have been fixed for some time), then this patch below would be eminently sensible.

Quoted 3 lines

Add BUG_ON() instead, how about it?
Thanks,
Lai

We need a two-step process: (1) Add a WARN_ON() in the existing "if" statement, and after a year or so of no WARN_ON()s being triggered (2) eliminate the "if" and in_serving_softirq().

Thanx, Paul

Quoted 81 lines
kernel/rcutiny_plugin.h +0 -6
index 22ecea0..22e2ef1 100644
@@ -571,12 +571,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 	if (special & RCU_READ_UNLOCK_NEED_QS)
 		rcu_preempt_cpu_qs();
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
kernel/rcutree_plugin.h +6 -7
index c023464..e90593e 100644
@@ -210,6 +210,7 @@ static void rcu_preempt_note_context_switch(int cpu)
 		raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	} else if (t->rcu_read_lock_nesting < 0 &&
 		   t->rcu_read_unlock_special) {
+		WARN_ON_ONCE(t->rcu_read_lock_nesting != INT_MIN);
 
 		/*
 		 * Complete exit from RCU read-side critical section on
@@ -239,7 +240,10 @@ static void rcu_preempt_note_context_switch(int cpu)
  */
 void __rcu_read_lock(void)
 {
-	current->rcu_read_lock_nesting++;
+	struct task_struct *t = current;
+
+	BUG_ON(!t->rcu_read_lock_nesting && t->rcu_read_unlock_special);
+	t->rcu_read_lock_nesting++;
 	barrier();  /* needed if we ever invoke rcu_read_lock in rcutree.c */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_lock);
@@ -338,12 +342,6 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 		rcu_preempt_qs(smp_processor_id());
 	}
 
-	/* Hardware IRQ handlers cannot block. */
-	if (in_irq() || in_serving_softirq()) {
-		local_irq_restore(flags);
-		return;
-	}
-
 	/* Clean up if blocked during RCU read-side critical section. */
 	if (special & RCU_READ_UNLOCK_BLOCKED) {
 		t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BLOCKED;
@@ -439,6 +437,7 @@ void __rcu_read_unlock(void)
 			rcu_read_unlock_special(t);
 		barrier();  /* ->rcu_read_unlock_special load before assign */
 		t->rcu_read_lock_nesting = 0;
+		BUG_ON(t->rcu_read_unlock_special);
 	}
 #ifdef CONFIG_PROVE_LOCKING
 	{
kernel/softirq.c +1 -5
index 671f959..7074f97 100644
@@ -318,12 +318,8 @@ static inline void invoke_softirq(void)
 #else
 		do_softirq();
 #endif
-	} else {
-		__local_bh_disable((unsigned long)__builtin_return_address(0),
-				SOFTIRQ_OFFSET);
+	} else
 		wakeup_softirqd();
-		__local_bh_enable(SOFTIRQ_OFFSET);
-	}
 }
 
 /*