Skip to contentSkip to conversation

Fixes for 3.12

PATCH In review Paul E. McKenney 3 participants 25 messages Aug 18, 2013 – Aug 19, 2013

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover Fixes for 3.12

Hello!

This series contains a number of miscellaneous fixes:

1. Switch to expedited grace periods during hibernation operations
(courtesy of Borislav Petkov).

2. Restrict above expediting to systems of 256 or fewer CPUs.

3. Make RCU debugobjects fixup actions leak callbacks rather than
risking corrupting the callback lists.

4. Propagate above callback leaking to debugobjects's callers.

5. Make call_rcu() leak callbacks when debugobjects indicates an error.

6. Switch to expedited grace periods during suspend/resume operations
(Courtesy of Bjørn Mork).

7. Avoid needless self-wakeups from the grace-period kthreads.

8. Make list_first_or_null_rcu() use list_entry_rcu() (courtesy of
Tejun Heo).

9. Fix irq_work_queue() build error for TREE_PREEMPT_RCU for some
configurations (courtesy of James Hogan).

10. Simplify _rcu_barrier() processing per Linus Torvalds feedback.

11. Avoid signed-overflow undefined behavior in time_after() and friends.

Thanx, Paul

 b/include/linux/debugobjects.h |    6 +-
 b/include/linux/jiffies.h      |    8 +--
 b/include/linux/rculist.h      |    5 +-
 b/init/Kconfig                 |    1 
 b/kernel/rcu.h                 |   10 ++--
 b/kernel/rcupdate.c            |  100 -----------------------------------------
 b/kernel/rcutree.c             |   62 ++++++++++++++++++++++---
 b/lib/debugobjects.c           |   20 +++++---
 8 files changed, 86 insertions(+), 126 deletions(-)
#1 lore raw

PATCH 1/11 rcu: Expedite grace periods during suspend/resume

From: Borislav Petkov <[email protected]>

CONFIG_RCU_FAST_NO_HZ can increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.

[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]

---
 kernel/rcutree.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
kernel/rcutree.c +18 -0
index 338f1d1..69e4792 100644
@@ -54,6 +54,7 @@
 #include <linux/stop_machine.h>
 #include <linux/random.h>
 #include <linux/ftrace_event.h>
+#include <linux/suspend.h>
 
 #include "rcutree.h"
 #include <trace/events/rcu.h>
@@ -3032,6 +3033,22 @@ static int rcu_cpu_notify(struct notifier_block *self,
 	return NOTIFY_OK;
 }
 
+static int rcu_pm_notify(struct notifier_block *self,
+			 unsigned long action, void *hcpu)
+{
+	switch (action) {
+	case PM_HIBERNATION_PREPARE:
+		rcu_expedited = 1;
+		break;
+	case PM_POST_RESTORE:
+		rcu_expedited = 0;
+		break;
+	default:
+		break;
+	}
+	return NOTIFY_OK;
+}
+
 /*
  * Spawn the kthread that handles this RCU flavor's grace periods.
  */
@@ -3273,6 +3290,7 @@ void __init rcu_init(void)
 	 * or the scheduler are operational.
 	 */
 	cpu_notifier(rcu_cpu_notify, 0);
+	pm_notifier(rcu_pm_notify, 0);
 	for_each_online_cpu(cpu)
 		rcu_cpu_notify(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
 }
Signature · 1 line
1.8.1.5
#2 lore raw

PATCH 2/11 rcu: Expedite during suspend and resume only on smallish systems

From: "Paul E. McKenney" <[email protected]>

Expedited grace periods are of dubious benefit on very large systems, so this commit restricts their automated use during suspend and resume to systems of 256 or fewer CPUs.

---
 kernel/rcutree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/rcutree.c +2 -1
index 69e4792..6ec33dc 100644
@@ -3038,7 +3038,8 @@ static int rcu_pm_notify(struct notifier_block *self,
 {
 	switch (action) {
 	case PM_HIBERNATION_PREPARE:
-		rcu_expedited = 1;
+		if (nr_cpu_ids <= 256) /* Expediting bad for large systems. */
+			rcu_expedited = 1;
 		break;
 	case PM_POST_RESTORE:
 		rcu_expedited = 0;
Signature · 1 line
1.8.1.5
maintainer
#3 lore raw

Re: [PATCH tip/core/rcu 02/11] rcu: Expedite during suspend and resume only on smallish systems

Quoted 7 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Expedited grace periods are of dubious benefit on very large systems, so this commit restricts their automated use during suspend and resume to systems of 256 or fewer CPUs.

This seems odd. If expedited grace periods don't help on large systems, shouldn't you just compile them out entirely and ignore rcu_expedited, rather than just in this one special case?

In any case, if this patch still makes sense, please squash it into the previous one.

- Josh Triplett

#4 lore raw

Re: [PATCH tip/core/rcu 02/11] rcu: Expedite during suspend and resume only on smallish systems

Quoted 7 lines · Josh Triplett
Quoted 6 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Expedited grace periods are of dubious benefit on very large systems, so this commit restricts their automated use during suspend and resume to systems of 256 or fewer CPUs.

Quoted 5 lines
Quoted 1 line

This seems odd. If expedited grace periods don't help on large systems, shouldn't you just compile them out entirely and ignore rcu_expedited, rather than just in this one special case?

Longer term, a bunch of stuff will need optimization for larger systems. Expedited RCU grace periods, rcu_barrier(), possibly even grace-period initialization and cleanup. Also smp_call_function(), along with other primitives that have broadcast semantics. These changes would allow this hack to be removed.

The most straightforward approach would be to provision helper kthreads, increasing the number of helper kthreads with the number of CPUs. But I would like to see actual problems due to the lack of scalability before adding more complexity to RCU. ;-)

Quoted 2 lines

In any case, if this patch still makes sense, please squash it into the previous one.

Fair enough, done!

Thanx, Paul

#5 lore raw

PATCH 3/11 rcu: Simplify debug-objects fixups

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The current debug-objects fixups are complex and heavyweight, and the fixups are not complete: Even with the fixups, RCU's callback lists can still be corrupted. This commit therefore strips the fixups down to their minimal form, eliminating two of the three.

It would be even better if (for example) call_rcu() simply leaked any problematic callbacks, but for that to happen, the debug-objects system would need to inform its caller of suspicious situations. This is the subject of a later commit in this series.

Cc Mathieu Desnoyers <[email protected]>
Cc Sedat Dilek <[email protected]>
Cc Davidlohr Bueso <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Linus Torvalds <[email protected]>
---
 kernel/rcupdate.c | 100 ------------------------------------------------------
 1 file changed, 100 deletions(-)
kernel/rcupdate.c +0 -100
index 14994d4..33eb462 100644
@@ -212,43 +212,6 @@ static inline void debug_rcu_head_free(struct rcu_head *head)
 }
 
 /*
- * fixup_init is called when:
- * - an active object is initialized
- */
-static int rcuhead_fixup_init(void *addr, enum debug_obj_state state)
-{
-	struct rcu_head *head = addr;
-
-	switch (state) {
-	case ODEBUG_STATE_ACTIVE:
-		/*
-		 * Ensure that queued callbacks are all executed.
-		 * If we detect that we are nested in a RCU read-side critical
-		 * section, we should simply fail, otherwise we would deadlock.
-		 * In !PREEMPT configurations, there is no way to tell if we are
-		 * in a RCU read-side critical section or not, so we never
-		 * attempt any fixup and just print a warning.
-		 */
-#ifndef CONFIG_PREEMPT
-		WARN_ON_ONCE(1);
-		return 0;
-#endif
-		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
-		    irqs_disabled()) {
-			WARN_ON_ONCE(1);
-			return 0;
-		}
-		rcu_barrier();
-		rcu_barrier_sched();
-		rcu_barrier_bh();
-		debug_object_init(head, &rcuhead_debug_descr);
-		return 1;
-	default:
-		return 0;
-	}
-}
-
-/*
  * fixup_activate is called when:
  * - an active object is activated
  * - an unknown object is activated (might be a statically initialized object)
@@ -268,69 +231,8 @@ static int rcuhead_fixup_activate(void *addr, enum debug_obj_state state)
 		debug_object_init(head, &rcuhead_debug_descr);
 		debug_object_activate(head, &rcuhead_debug_descr);
 		return 0;
-
-	case ODEBUG_STATE_ACTIVE:
-		/*
-		 * Ensure that queued callbacks are all executed.
-		 * If we detect that we are nested in a RCU read-side critical
-		 * section, we should simply fail, otherwise we would deadlock.
-		 * In !PREEMPT configurations, there is no way to tell if we are
-		 * in a RCU read-side critical section or not, so we never
-		 * attempt any fixup and just print a warning.
-		 */
-#ifndef CONFIG_PREEMPT
-		WARN_ON_ONCE(1);
-		return 0;
-#endif
-		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
-		    irqs_disabled()) {
-			WARN_ON_ONCE(1);
-			return 0;
-		}
-		rcu_barrier();
-		rcu_barrier_sched();
-		rcu_barrier_bh();
-		debug_object_activate(head, &rcuhead_debug_descr);
-		return 1;
 	default:
-		return 0;
-	}
-}
-
-/*
- * fixup_free is called when:
- * - an active object is freed
- */
-static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
-{
-	struct rcu_head *head = addr;
-
-	switch (state) {
-	case ODEBUG_STATE_ACTIVE:
-		/*
-		 * Ensure that queued callbacks are all executed.
-		 * If we detect that we are nested in a RCU read-side critical
-		 * section, we should simply fail, otherwise we would deadlock.
-		 * In !PREEMPT configurations, there is no way to tell if we are
-		 * in a RCU read-side critical section or not, so we never
-		 * attempt any fixup and just print a warning.
-		 */
-#ifndef CONFIG_PREEMPT
-		WARN_ON_ONCE(1);
-		return 0;
-#endif
-		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
-		    irqs_disabled()) {
-			WARN_ON_ONCE(1);
-			return 0;
-		}
-		rcu_barrier();
-		rcu_barrier_sched();
-		rcu_barrier_bh();
-		debug_object_free(head, &rcuhead_debug_descr);
 		return 1;
-	default:
-		return 0;
 	}
 }
 
@@ -369,9 +271,7 @@ EXPORT_SYMBOL_GPL(destroy_rcu_head_on_stack);
 
 struct debug_obj_descr rcuhead_debug_descr = {
 	.name = "rcu_head",
-	.fixup_init = rcuhead_fixup_init,
 	.fixup_activate = rcuhead_fixup_activate,
-	.fixup_free = rcuhead_fixup_free,
 };
 EXPORT_SYMBOL_GPL(rcuhead_debug_descr);
 #endif /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
Signature · 1 line
1.8.1.5
#6 lore raw

PATCH 4/11 debugobjects: Make debug_object_activate() return status

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

In order to better respond to things like duplicate invocations of call_rcu(), RCU needs to see the status of a call to debug_object_activate(). This would allow RCU to leak the callback in order to avoid adding freelist-reuse mischief to the duplicate invoations. This commit therefore makes debug_object_activate() return status, zero for success and -EINVAL for failure.

Cc Mathieu Desnoyers <[email protected]>
Cc Sedat Dilek <[email protected]>
Cc Davidlohr Bueso <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Linus Torvalds <[email protected]>
---
 include/linux/debugobjects.h |  6 +++---
 lib/debugobjects.c           | 20 ++++++++++++++------
 2 files changed, 17 insertions(+), 9 deletions(-)
include/linux/debugobjects.h +3 -3
index 0e5f578..98ffcbd 100644
@@ -63,7 +63,7 @@ struct debug_obj_descr {
 extern void debug_object_init      (void *addr, struct debug_obj_descr *descr);
 extern void
 debug_object_init_on_stack(void *addr, struct debug_obj_descr *descr);
-extern void debug_object_activate  (void *addr, struct debug_obj_descr *descr);
+extern int debug_object_activate  (void *addr, struct debug_obj_descr *descr);
 extern void debug_object_deactivate(void *addr, struct debug_obj_descr *descr);
 extern void debug_object_destroy   (void *addr, struct debug_obj_descr *descr);
 extern void debug_object_free      (void *addr, struct debug_obj_descr *descr);
@@ -85,8 +85,8 @@ static inline void
 debug_object_init      (void *addr, struct debug_obj_descr *descr) { }
 static inline void
 debug_object_init_on_stack(void *addr, struct debug_obj_descr *descr) { }
-static inline void
-debug_object_activate  (void *addr, struct debug_obj_descr *descr) { }
+static inline int
+debug_object_activate  (void *addr, struct debug_obj_descr *descr) { return 0; }
 static inline void
 debug_object_deactivate(void *addr, struct debug_obj_descr *descr) { }
 static inline void
lib/debugobjects.c +14 -6
index 37061ed..bf2c8b1 100644
@@ -381,19 +381,21 @@ void debug_object_init_on_stack(void *addr, struct debug_obj_descr *descr)
  * debug_object_activate - debug checks when an object is activated
  * @addr:	address of the object
  * @descr:	pointer to an object specific debug description structure
+ * Returns 0 for success, -EINVAL for check failed.
  */
-void debug_object_activate(void *addr, struct debug_obj_descr *descr)
+int debug_object_activate(void *addr, struct debug_obj_descr *descr)
 {
 	enum debug_obj_state state;
 	struct debug_bucket *db;
 	struct debug_obj *obj;
 	unsigned long flags;
+	int ret;
 	struct debug_obj o = { .object = addr,
 			       .state = ODEBUG_STATE_NOTAVAILABLE,
 			       .descr = descr };
 
 	if (!debug_objects_enabled)
-		return;
+		return 0;
 
 	db = get_bucket((unsigned long) addr);
 
@@ -405,23 +407,26 @@ void debug_object_activate(void *addr, struct debug_obj_descr *descr)
 		case ODEBUG_STATE_INIT:
 		case ODEBUG_STATE_INACTIVE:
 			obj->state = ODEBUG_STATE_ACTIVE;
+			ret = 0;
 			break;
 
 		case ODEBUG_STATE_ACTIVE:
 			debug_print_object(obj, "activate");
 			state = obj->state;
 			raw_spin_unlock_irqrestore(&db->lock, flags);
-			debug_object_fixup(descr->fixup_activate, addr, state);
-			return;
+			ret = debug_object_fixup(descr->fixup_activate, addr, state);
+			return ret ? -EINVAL : 0;
 
 		case ODEBUG_STATE_DESTROYED:
 			debug_print_object(obj, "activate");
+			ret = -EINVAL;
 			break;
 		default:
+			ret = 0;
 			break;
 		}
 		raw_spin_unlock_irqrestore(&db->lock, flags);
-		return;
+		return ret;
 	}
 
 	raw_spin_unlock_irqrestore(&db->lock, flags);
@@ -431,8 +436,11 @@ void debug_object_activate(void *addr, struct debug_obj_descr *descr)
 	 * true or not.
 	 */
 	if (debug_object_fixup(descr->fixup_activate, addr,
-			   ODEBUG_STATE_NOTAVAILABLE))
+			   ODEBUG_STATE_NOTAVAILABLE)) {
 		debug_print_object(&o, "activate");
+		return -EINVAL;
+	}
+	return 0;
 }
 
 /**
Signature · 1 line
1.8.1.5
#7 lore raw

PATCH 5/11 rcu: Make call_rcu() leak callbacks for debug-object errors

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

If someone does a duplicate call_rcu(), the worst thing the second call_rcu() could do would be to actually queue the callback the second time because doing so corrupts whatever list the callback was already queued on. This commit therefore makes __call_rcu() check the new return value from debug-objects and leak the callback upon error. This commit also substitutes rcu_leak_callback() for whatever callback function was previously in place in order to avoid freeing the callback out from under any readers that might still be referencing it.

These changes increase the probability that the debug-objects error messages will actually make it somewhere visible.

Cc Mathieu Desnoyers <[email protected]>
Cc Sedat Dilek <[email protected]>
Cc Davidlohr Bueso <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Linus Torvalds <[email protected]>
---
 kernel/rcu.h     | 10 +++++++---
 kernel/rcutree.c | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 4 deletions(-)
kernel/rcu.h +7 -3
index 0a90ccc..7713196 100644
@@ -67,12 +67,15 @@
 
 extern struct debug_obj_descr rcuhead_debug_descr;
 
-static inline void debug_rcu_head_queue(struct rcu_head *head)
+static inline int debug_rcu_head_queue(struct rcu_head *head)
 {
-	debug_object_activate(head, &rcuhead_debug_descr);
+	int r1;
+
+	r1 = debug_object_activate(head, &rcuhead_debug_descr);
 	debug_object_active_state(head, &rcuhead_debug_descr,
 				  STATE_RCU_HEAD_READY,
 				  STATE_RCU_HEAD_QUEUED);
+	return r1;
 }
 
 static inline void debug_rcu_head_unqueue(struct rcu_head *head)
@@ -83,8 +86,9 @@ static inline void debug_rcu_head_unqueue(struct rcu_head *head)
 	debug_object_deactivate(head, &rcuhead_debug_descr);
 }
 #else	/* !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
-static inline void debug_rcu_head_queue(struct rcu_head *head)
+static inline int debug_rcu_head_queue(struct rcu_head *head)
 {
+	return 0;
 }
 
 static inline void debug_rcu_head_unqueue(struct rcu_head *head)
kernel/rcutree.c +13 -1
index 6ec33dc..405f2303 100644
@@ -2305,6 +2305,13 @@ static void __call_rcu_core(struct rcu_state *rsp, struct rcu_data *rdp,
 }
 
 /*
+ * RCU callback function to leak a callback.
+ */
+static void rcu_leak_callback(struct rcu_head *rhp)
+{
+}
+
+/*
  * Helper function for call_rcu() and friends.  The cpu argument will
  * normally be -1, indicating "currently running CPU".  It may specify
  * a CPU only if that CPU is a no-CBs CPU.  Currently, only _rcu_barrier()
@@ -2318,7 +2325,12 @@ __call_rcu(struct rcu_head *head, void (*func)(struct rcu_head *rcu),
 	struct rcu_data *rdp;
 
 	WARN_ON_ONCE((unsigned long)head & 0x3); /* Misaligned rcu_head! */
-	debug_rcu_head_queue(head);
+	if (debug_rcu_head_queue(head)) {
+		/* Probable double call_rcu(), so leak the callback. */
+		ACCESS_ONCE(head->func) = rcu_leak_callback;
+		WARN_ONCE(1, "__call_rcu(): Leaked duplicate callback\n");
+		return;
+	}
 	head->func = func;
 	head->next = NULL;
 
Signature · 1 line
1.8.1.5
#8 lore raw

PATCH 6/11 rcu: Switch to exedited grace periods for suspend as well as hibernation

In reply to Paul E. McKenney

From: Bjørn Mork <[email protected]>

Commit 587ff2cf ("rcu: Expedite grace periods during suspend/resume") enabled expedited grace periods for hibernation, but not for suspend. The same issue applies to both cases, so this commit simply applies the same logic by adding additional cases to the switch statement.

Note that this commit also switches from PM_POST_RESTORE to the combination of PM_POST_HIBERNATION and PM_POST_SUSPEND. A separate patch from Borislav Petkov corrects the documentation to indicate that this is necessary.

---
 kernel/rcutree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/rcutree.c +3 -1
index 405f2303..9184056 100644
@@ -3050,10 +3050,12 @@ static int rcu_pm_notify(struct notifier_block *self,
 {
 	switch (action) {
 	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
 		if (nr_cpu_ids <= 256) /* Expediting bad for large systems. */
 			rcu_expedited = 1;
 		break;
-	case PM_POST_RESTORE:
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
 		rcu_expedited = 0;
 		break;
 	default:
Signature · 1 line
1.8.1.5
maintainer
#9 lore raw

Re: [PATCH tip/core/rcu 06/11] rcu: Switch to exedited grace periods for suspend as well as hibernation

Quoted 7 lines · Paul E. McKenney

From: Bjørn Mork <[email protected]>

Commit 587ff2cf ("rcu: Expedite grace periods during suspend/resume") enabled expedited grace periods for hibernation, but not for suspend. The same issue applies to both cases, so this commit simply applies the same logic by adding additional cases to the switch statement.

Quoted 7 lines

Note that this commit also switches from PM_POST_RESTORE to the combination of PM_POST_HIBERNATION and PM_POST_SUSPEND. A separate patch from Borislav Petkov corrects the documentation to indicate that this is necessary.

Please squash this together with the other two relevant patches in this series.

- Josh Triplett

#10 lore raw

Re: [PATCH tip/core/rcu 06/11] rcu: Switch to exedited grace periods for suspend as well as hibernation

Quoted 13 lines · Josh Triplett
Quoted 12 lines · Paul E. McKenney

From: Bjørn Mork <[email protected]>

Commit 587ff2cf ("rcu: Expedite grace periods during suspend/resume") enabled expedited grace periods for hibernation, but not for suspend. The same issue applies to both cases, so this commit simply applies the same logic by adding additional cases to the switch statement.

Note that this commit also switches from PM_POST_RESTORE to the combination of PM_POST_HIBERNATION and PM_POST_SUSPEND. A separate patch from Borislav Petkov corrects the documentation to indicate that this is necessary.

Quoted 5 lines
Quoted 2 lines

Please squash this together with the other two relevant patches in this series.

Done! (Of course, retaining both Borislav's and Bjørn's Signed-off-bys.)

Thanx, Paul

#11 lore raw

PATCH 7/11 rcu: Avoid redundant grace-period kthread wakeups

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

When setting up an in-the-future "advanced" grace period, the code needs to wake up the relevant grace-period kthread, which it currently does unconditionally. However, this results in needless wakeups in the case where the advanced grace period is being set up by the grace-period kthread itself, which is a non-uncommon situation. This commit therefore checks to see if the running thread is the grace-period kthread, and avoids doing the irq_work_queue()-mediated wakeup in that case.

---
 kernel/rcutree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
kernel/rcutree.c +5 -3
index 9184056..c6a064a 100644
@@ -1576,10 +1576,12 @@ rcu_start_gp_advanced(struct rcu_state *rsp, struct rcu_node *rnp,
 
 	/*
 	 * We can't do wakeups while holding the rnp->lock, as that
-	 * could cause possible deadlocks with the rq->lock. Deter
-	 * the wakeup to interrupt context.
+	 * could cause possible deadlocks with the rq->lock. Defer
+	 * the wakeup to interrupt context.  And don't bother waking
+	 * up the running kthread.
 	 */
-	irq_work_queue(&rsp->wakeup_work);
+	if (current != rsp->gp_kthread)
+		irq_work_queue(&rsp->wakeup_work);
 }
 
 /*
Signature · 1 line
1.8.1.5
#12 lore raw

PATCH 8/11 rculist: list_first_or_null_rcu() should use list_entry_rcu()

In reply to Paul E. McKenney

From: Tejun Heo <[email protected]>

list_first_or_null() should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in several ways.

* It compares __kernel @__ptr with __rcu @__next triggering the following sparse warning.

net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)

* It doesn't perform rcu_dereference*() and computes the entry address using container_of() directly from the __rcu pointer which is inconsitent with other rculist interface. As a result, all three in-kernel users - net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.

* While ->next dereference passes through list_next_rcu(), the compiler is still free to fetch ->next more than once and thus nullify the "__ptr != __next" condition check.

Fix it by making list_first_or_null_rcu() dereference ->next directly using ACCESS_ONCE() and then use list_entry_rcu() on it like other rculist accessors.

v2: Paul pointed out that the compiler may fetch the pointer more than
once nullifying the condition check. ACCESS_ONCE() added on ->next dereference.

v3: Restored () around macro param which was accidentally removed.
Spotted by Paul.

Cc Dipankar Sarma <[email protected]>
Cc "Paul E. McKenney" <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Li Zefan <[email protected]>
Cc Patrick McHardy <[email protected]>
---
 include/linux/rculist.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
include/linux/rculist.h +3 -2
index f4b1001..4106721 100644
@@ -267,8 +267,9 @@ static inline void list_splice_init_rcu(struct list_head *list,
  */
 #define list_first_or_null_rcu(ptr, type, member) \
 	({struct list_head *__ptr = (ptr); \
-	  struct list_head __rcu *__next = list_next_rcu(__ptr); \
-	  likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
+	  struct list_head *__next = ACCESS_ONCE(__ptr->next); \
+	  likely(__ptr != __next) ? \
+		list_entry_rcu(__next, type, member) : NULL; \
 	})
 
 /**
Signature · 1 line
1.8.1.5
#13 lore raw

PATCH 9/11 rcu: Select IRQ_WORK from TREE_PREEMPT_RCU

In reply to Paul E. McKenney

From: James Hogan <[email protected]>

TREE_RCU and TREE_PREEMPT_RCU both cause kernel/rcutree.c to be built, but only TREE_RCU selects IRQ_WORK, which can result in an undefined reference to irq_work_queue for some (random) configs:

kernel/built-in.o In function `rcu_start_gp_advanced':
kernel/rcutree.c:1564: undefined reference to `irq_work_queue'

Select IRQ_WORK from TREE_PREEMPT_RCU too to fix this.

Cc Steven Rostedt <[email protected]>
Cc Paul E. McKenney <[email protected]>
Cc Dipankar Sarma <[email protected]>
---
 init/Kconfig | 1 +
 1 file changed, 1 insertion(+)
init/Kconfig +1 -0
index 247084b..c08a549 100644
@@ -470,6 +470,7 @@ config TREE_RCU
 config TREE_PREEMPT_RCU
 	bool "Preemptible tree-based hierarchical RCU"
 	depends on PREEMPT
+	select IRQ_WORK
 	help
 	  This option selects the RCU implementation that is
 	  designed for very large SMP systems with hundreds or
Signature · 1 line
1.8.1.5
#14 lore raw

PATCH 10/11 rcu: Simplify _rcu_barrier() processing

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

This commit drops an unneeded ACCESS_ONCE() and simplifies an "our work is done" check in _rcu_barrier(). This applies feedback from Linus
(https://lkml.org/lkml/2013/7/26/777) that he gave to similar code in an unrelated patch.

---
 kernel/rcutree.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
kernel/rcutree.c +13 -2
index c6a064a..612aff1 100644
@@ -2817,9 +2817,20 @@ static void _rcu_barrier(struct rcu_state *rsp)
 	 * transition.  The "if" expression below therefore rounds the old
 	 * value up to the next even number and adds two before comparing.
 	 */
-	snap_done = ACCESS_ONCE(rsp->n_barrier_done);
+	snap_done = rsp->n_barrier_done;
 	_rcu_barrier_trace(rsp, "Check", -1, snap_done);
-	if (ULONG_CMP_GE(snap_done, ((snap + 1) & ~0x1) + 2)) {
+
+	/*
+	 * If the value in snap is odd, we needed to wait for the current
+	 * rcu_barrier() to complete, then wait for the next one, in other
+	 * words, we need the value of snap_done to be three larger than
+	 * the value of snap.  On the other hand, if the value in snap is
+	 * even, we only had to wait for the next rcu_barrier() to complete,
+	 * in other words, we need the value of snap_done to be only two
+	 * greater than the value of snap.  The "(snap + 3) & 0x1" computes
+	 * this for us (thank you, Linus!).
+	 */
+	if (ULONG_CMP_GE(snap_done, (snap + 3) & ~0x1)) {
 		_rcu_barrier_trace(rsp, "EarlyExit", -1, snap_done);
 		smp_mb(); /* caller's subsequent code after above check. */
 		mutex_unlock(&rsp->barrier_mutex);
Signature · 1 line
1.8.1.5
#15 lore raw

PATCH 11/11 jiffies: Avoid undefined behavior from signed overflow

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of time_after(), time_after_eq(), time_after64(), and time_after_eq64() to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.

Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be happy to act as a witness for its being committed to an insane asylum. (Although I have nothing against saturating arithmetic or signals in some cases, these things really should not be the default.)

Cc John Stultz <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Kevin Easton <[email protected]>

[ paulmck: Included time_after64() and time_after_eq64(), as suggested
by Eric Dumazet, also fixed commit message.]

---
 include/linux/jiffies.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
include/linux/jiffies.h +4 -4
index 97ba4e7..d235e88 100644
@@ -101,13 +101,13 @@ static inline u64 get_jiffies_64(void)
 #define time_after(a,b)		\
 	(typecheck(unsigned long, a) && \
 	 typecheck(unsigned long, b) && \
-	 ((long)(b) - (long)(a) < 0))
+	 ((long)((b) - (a)) < 0))
 #define time_before(a,b)	time_after(b,a)
 
 #define time_after_eq(a,b)	\
 	(typecheck(unsigned long, a) && \
 	 typecheck(unsigned long, b) && \
-	 ((long)(a) - (long)(b) >= 0))
+	 ((long)((a) - (b)) >= 0))
 #define time_before_eq(a,b)	time_after_eq(b,a)
 
 /*
@@ -130,13 +130,13 @@ static inline u64 get_jiffies_64(void)
 #define time_after64(a,b)	\
 	(typecheck(__u64, a) &&	\
 	 typecheck(__u64, b) && \
-	 ((__s64)(b) - (__s64)(a) < 0))
+	 ((__s64)((b) - (a)) < 0))
 #define time_before64(a,b)	time_after64(b,a)
 
 #define time_after_eq64(a,b)	\
 	(typecheck(__u64, a) && \
 	 typecheck(__u64, b) && \
-	 ((__s64)(a) - (__s64)(b) >= 0))
+	 ((__s64)((a) - (b)) >= 0))
 #define time_before_eq64(a,b)	time_after_eq64(b,a)
 
 #define time_in_range64(a, b, c) \
Signature · 1 line
1.8.1.5
maintainer Reviewed-by on v1 11/11
#16 lore raw

Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of time_after(), time_after_eq(), time_after64(), and time_after_eq64() to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.

Quoted 4 lines

Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be

s/free come/free to come/

Quoted 1 line

happy to act as a witness for its being committed to an insane asylum.

With the typo above fixed:

#17 lore raw

Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow

Quoted 12 lines · Josh Triplett
Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of time_after(), time_after_eq(), time_after64(), and time_after_eq64() to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.

Quoted 6 lines
Quoted 4 lines

Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be

s/free come/free to come/

Good catch, fixed!

Thanx, Paul

Quoted 5 lines
Quoted 1 line

happy to act as a witness for its being committed to an insane asylum.

With the typo above fixed:

maintainer
#18 lore raw

Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow

Quoted 13 lines · Paul E. McKenney
Quoted 12 lines · Josh Triplett
Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of time_after(), time_after_eq(), time_after64(), and time_after_eq64() to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be

s/free come/free to come/

Good catch, fixed!

Just realized when looking at this again that there's another typo:
"two-complement" should be "two's-complement".

Quoted 8 lines

Thanx, Paul

Quoted 5 lines
Quoted 1 line

happy to act as a witness for its being committed to an insane asylum.

With the typo above fixed:

#19 lore raw

Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow

Quoted 19 lines · Josh Triplett
Quoted 18 lines · Paul E. McKenney
Quoted 17 lines · Josh Triplett
Quoted 15 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of time_after(), time_after_eq(), time_after64(), and time_after_eq64() to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.

Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

s/free come/free to come/

Good catch, fixed!

Just realized when looking at this again that there's another typo:
"two-complement" should be "two's-complement".

OK, fixed that as well. ;-)

Thank you for all the reviews and comments!

Thanx, Paul

Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
Quoted 1 line

happy to act as a witness for its being committed to an insane asylum.

With the typo above fixed:

maintainer Reviewed-by on v1 1/11
#20 lore raw

Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume

In reply to Paul E. McKenney
Quoted 7 lines · Paul E. McKenney

From: Borislav Petkov <[email protected]>

CONFIG_RCU_FAST_NO_HZ can increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.

Quoted 5 lines

[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]

This patch still seems like a hack, and there *ought* to be a better general solution to avoid excessive grace-period latency. Nonetheless, in the absence of such a solution,

#21 lore raw

Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume

Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney

From: Borislav Petkov <[email protected]>

CONFIG_RCU_FAST_NO_HZ can increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.

[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]

Quoted 7 lines
Quoted 2 lines

This patch still seems like a hack, and there *ought* to be a better general solution to avoid excessive grace-period latency. Nonetheless, in the absence of such a solution,

Yeah, I'm not happy about it either but from quickly skimming over what context we're using rcu_expedited in, the basic requirement for a fix is for the pm core to be able to tell rcu not to stretch grace periods.

So simply setting a variable is much simpler than switching to calling all those *_expedited() rcu flavors from the pm notifier when going down.

Unless Paul has a better idea, of course.

The basic problem here is, however, that you need to temporarily reconfigure the inner workings of a subsystem because hardware is performing a power state transition. Unless someone teaches rcu about power state transitions... :-)

Signature · 5 lines
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
#22 lore raw

Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume

Quoted 28 lines · Borislav Petkov
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney

From: Borislav Petkov <[email protected]>

CONFIG_RCU_FAST_NO_HZ can increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.

[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]

Quoted 7 lines
Quoted 2 lines

This patch still seems like a hack, and there *ought* to be a better general solution to avoid excessive grace-period latency. Nonetheless, in the absence of such a solution,

Yeah, I'm not happy about it either but from quickly skimming over what context we're using rcu_expedited in, the basic requirement for a fix is for the pm core to be able to tell rcu not to stretch grace periods.

So simply setting a variable is much simpler than switching to calling all those *_expedited() rcu flavors from the pm notifier when going down.

Quoted 6 lines

Unless Paul has a better idea, of course.

The basic problem here is, however, that you need to temporarily reconfigure the inner workings of a subsystem because hardware is performing a power state transition. Unless someone teaches rcu about power state transitions... :-)

I would guess that once we have a few more subsystems that want RCU to temporarily expedite its grace periods, we will have enough information and experience to do something a bit more general. In the meantime, I am opting for something simple. ;-)

Thanx, Paul

#23 lore raw

Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume

Quoted 4 lines · Paul E. McKenney

I would guess that once we have a few more subsystems that want RCU to temporarily expedite its grace periods, we will have enough information and experience to do something a bit more general. In the meantime, I am opting for something simple. ;-)

Aah, the empirical approach! Very wise, master Jedi :-)

Signature · 5 lines
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
maintainer Reviewed-by on the v1 series
#24 lore raw
In reply to Paul E. McKenney
Quoted 24 lines · Paul E. McKenney

Hello!

This series contains a number of miscellaneous fixes:

1. Switch to expedited grace periods during hibernation operations
(courtesy of Borislav Petkov).

2. Restrict above expediting to systems of 256 or fewer CPUs.

3. Make RCU debugobjects fixup actions leak callbacks rather than
risking corrupting the callback lists.

4. Propagate above callback leaking to debugobjects's callers.

5. Make call_rcu() leak callbacks when debugobjects indicates an error.

6. Switch to expedited grace periods during suspend/resume operations
(Courtesy of Bjørn Mork).

7. Avoid needless self-wakeups from the grace-period kthreads.

8. Make list_first_or_null_rcu() use list_entry_rcu() (courtesy of
Tejun Heo).

Quoted 6 lines

9. Fix irq_work_queue() build error for TREE_PREEMPT_RCU for some
configurations (courtesy of James Hogan).

10. Simplify _rcu_barrier() processing per Linus Torvalds feedback.

11. Avoid signed-overflow undefined behavior in time_after() and friends.

Comments on 2, 6, and 11; for the rest: