Preview of fourth set of RCU changes for 3.3

The rcutorture test now can automatically exercise CPU hotplug and collect success statistics, which can be correlated with other rcutorture activity. This permits rcutorture to completely exercise RCU regardless of what sort of userspace and filesystem layout is in use. Unfortunately, rcutorture is happy to attempt to offline CPUs that cannot be offlined, for example, CPU 0 in both the x86 and ARM architectures. Although this allows rcutorture testing to proceed normally, it confounds attempts at error analysis due to the resulting flood of spurious CPU-hotplug errors.

Therefore, this commit creates a cpu_is_hotpluggable() function that allows rcutorture to avoid attempting to offline CPUs that are not hotpluggable, which in turn allows rcutorture to avoid reporting spurious CPU-hotplug errors. Note that this function is EXPORT_SYMBOL_GPL() to allow rcutorture to use it when compiled as a kernel module. This commit also includes modifications to rcutorture to use this new function.

drivers/base/cpu.c +1 -0

@@ -23,6 +23,7 @@ struct sysdev_class cpu_sysdev_class = {
 EXPORT_SYMBOL(cpu_sysdev_class);
 
 static DEFINE_PER_CPU(struct sys_device *, cpu_sys_devices);
+static DEFINE_PER_CPU(bool, is_hotpluggable);

Quoted 9 lines · Paul E. McKenney

The rcutorture test now can automatically exercise CPU hotplug and collect success statistics, which can be correlated with other rcutorture activity. This permits rcutorture to completely exercise RCU regardless of what sort of userspace and filesystem layout is in use. Unfortunately, rcutorture is happy to attempt to offline CPUs that cannot be offlined, for example, CPU 0 in both the x86 and ARM architectures. Although this allows rcutorture testing to proceed normally, it confounds attempts at error analysis due to the resulting flood of spurious CPU-hotplug errors.

Quoted 7 lines

Therefore, this commit creates a cpu_is_hotpluggable() function that allows rcutorture to avoid attempting to offline CPUs that are not hotpluggable, which in turn allows rcutorture to avoid reporting spurious CPU-hotplug errors. Note that this function is EXPORT_SYMBOL_GPL() to allow rcutorture to use it when compiled as a kernel module. This commit also includes modifications to rcutorture to use this new function.

I'd suggest writing this as a two-patch series: add the API, then use it.

I'd also suggest making the more general case for this API, beyond just rcutorture.

Quoted 7 lines

drivers/base/cpu.c +1 -0

@@ -23,6 +23,7 @@ struct sysdev_class cpu_sysdev_class = {
 EXPORT_SYMBOL(cpu_sysdev_class);
 
 static DEFINE_PER_CPU(struct sys_device *, cpu_sys_devices);
+static DEFINE_PER_CPU(bool, is_hotpluggable);

[...]

Quoted 4 lines

drivers/base/cpu.c +0 -0

@@ -224,8 +226,10 @@ int __cpuinit register_cpu(struct cpu *cpu, int num)
 
 	error = sysdev_register(&cpu->sysdev);
 

drivers/base/cpu.c int __cpuinit register_cpu(struct cpu *cpu, int num) · line 232

Quoted 5 lines

drivers/base/cpu.c +3 -1

-	if (!error && cpu->hotpluggable)
+	if (!error && cpu->hotpluggable) {
 		register_cpu_control(cpu);
+		per_cpu(is_hotpluggable, num) = 1;
+	}

This information already exists in the ->hotpluggable field of "struct cpu". I'd suggest an alternate approach (included as a patch below), which avoids the need to add a new per_cpu property redundant with ->hotpluggable.

Note that you can get to the patch below via "git am --scissors", or just by copy/pasting it.

---8<---
From: Josh Triplett <[email protected]>
Subject: [PATCH] driver-core/cpu: Expose hotpluggability to the rest of the kernel

When architectures register CPUs, they indicate whether the CPU allows hotplugging; notably, x86 and ARM don't allow hotplugging CPU 0. Userspace can easily query the hotpluggability of a CPU via sysfs; however, the kernel has no convenient way of accessing that property in an architecture-independent way. While the kernel can simply try it and see, some code needs to distinguish between "hotplug failed" and "hotplug has no hope of working on this CPU"; for example, rcutorture's CPU hotplug tests want to avoid drowning out real hotplug failures with expected failures.

Expose this property via a new cpu_is_hotpluggable function, so that the rest of the kernel can access it in an architecture-independent way.

Signed-off-by Josh Triplett <[email protected]>

---
 drivers/base/cpu.c  |    7 +++++++
 include/linux/cpu.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

drivers/base/cpu.c +7 -0

index 251acea..3991502 100644
@@ -247,6 +247,13 @@ struct sys_device *get_cpu_sysdev(unsigned cpu)
 }
 EXPORT_SYMBOL_GPL(get_cpu_sysdev);
 
+bool cpu_is_hotpluggable(unsigned cpu)
+{
+	struct sys_device *dev = get_cpu_sysdev(cpu);
+	return dev && container_of(dev, struct cpu, sysdev)->hotpluggable;
+}
+EXPORT_SYMBOL_GPL(cpu_is_hotpluggable);
+
 int __init cpu_dev_init(void)
 {
 	int err;

include/linux/cpu.h +1 -0

index 6cb60fd..305c263 100644
@@ -27,6 +27,7 @@ struct cpu {
 
 extern int register_cpu(struct cpu *cpu, int num);
 extern struct sys_device *get_cpu_sysdev(unsigned cpu);
+extern bool cpu_is_hotpluggable(unsigned cpu);
 
 extern int cpu_add_sysdev_attr(struct sysdev_attribute *attr);
 extern void cpu_remove_sysdev_attr(struct sysdev_attribute *attr);

Signature · 1 line

1.7.7.3

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

Signed-off-by Yong Zhang <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcutree_plugin.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +4 -1

index 8cd9efe..2020e8a 100644
@@ -401,8 +401,11 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 
 #ifdef CONFIG_RCU_BOOST
 		/* Unboost if we were boosted. */
-		if (rbmp)
+		if (rbmp) {
+			local_irq_save(flags);
 			rt_mutex_unlock(rbmp);
+			local_irq_restore(flags);
+		}
 #endif /* #ifdef CONFIG_RCU_BOOST */
 
 		/*

kernel/rcutree_plugin.h +1 -0

+	local_irq_restore(flags);
 
 	return rnp->exp_tasks != NULL || rnp->boost_tasks != NULL;
 }

Signature · 7 lines

1.7.3.2

--
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/

Quoted 35 lines · Paul E. McKenney

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

Signed-off-by Yong Zhang <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcutree_plugin.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +4 -1

index 8cd9efe..2020e8a 100644
@@ -401,8 +401,11 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 
 #ifdef CONFIG_RCU_BOOST
 		/* Unboost if we were boosted. */
-		if (rbmp)
+		if (rbmp) {
+			local_irq_save(flags);
 			rt_mutex_unlock(rbmp);
+			local_irq_restore(flags);
+		}
 #endif /* #ifdef CONFIG_RCU_BOOST */
 

kernel/rcutree_plugin.h static int rcu_boost(struct rcu_node *rnp) · line 1241

Quoted 9 lines

kernel/rcutree_plugin.h +1 -1

 		/*

@@ -1233,9 +1236,10 @@ static int rcu_boost(struct rcu_node *rnp)
 	lockdep_set_class_and_name(&mtx.wait_lock, &rcu_boost_class,
 				   "rcu_boost_mutex");
 	t->rcu_boost_mutex = &mtx;
-	raw_spin_unlock_irqrestore(&rnp->lock, flags);
+	raw_spin_unlock(&rnp->lock); /* rrupts remain disabled. */
 	rt_mutex_lock(&mtx);  /* Side effect: boosts task t's priority. */
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */

We permit rt_mutex_unlock() to be call with irq disabled, but rt_mutex_lock() is still not allowed. So this usage is not legal now.

Sounds we should hold this patch on until a more suitable way is found.

Thanks,
Yong

Quoted 12 lines

kernel/rcutree_plugin.h +1 -0

+	local_irq_restore(flags);
 
 	return rnp->exp_tasks != NULL || rnp->boost_tasks != NULL;
 }

Signature · 7 lines

1.7.3.2

--
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/

Signature · 1 line

Only stand for myself

Quoted 36 lines · Yong Zhang

Quoted 35 lines · Paul E. McKenney

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

Signed-off-by Yong Zhang <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcutree_plugin.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +4 -1

index 8cd9efe..2020e8a 100644
@@ -401,8 +401,11 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 
 #ifdef CONFIG_RCU_BOOST
 		/* Unboost if we were boosted. */
-		if (rbmp)
+		if (rbmp) {
+			local_irq_save(flags);
 			rt_mutex_unlock(rbmp);
+			local_irq_restore(flags);
+		}
 #endif /* #ifdef CONFIG_RCU_BOOST */
 

Quoted 13 lines

kernel/rcutree_plugin.h static int rcu_boost(struct rcu_node *rnp) · line 1241

Quoted 9 lines

kernel/rcutree_plugin.h +1 -1

 		/*

@@ -1233,9 +1236,10 @@ static int rcu_boost(struct rcu_node *rnp)
 	lockdep_set_class_and_name(&mtx.wait_lock, &rcu_boost_class,
 				   "rcu_boost_mutex");
 	t->rcu_boost_mutex = &mtx;
-	raw_spin_unlock_irqrestore(&rnp->lock, flags);
+	raw_spin_unlock(&rnp->lock); /* rrupts remain disabled. */
 	rt_mutex_lock(&mtx);  /* Side effect: boosts task t's priority. */
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */

We permit rt_mutex_unlock() to be call with irq disabled, but rt_mutex_lock() is still not allowed. So this usage is not legal now.

Even after commit #5342e269b has been applied?

Quoted 51 lines · Paul E. McKenney

Quoted 49 lines · Yong Zhang

Quoted 37 lines · Paul E. McKenney

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

Signed-off-by Yong Zhang <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcutree_plugin.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +4 -1

index 8cd9efe..2020e8a 100644
@@ -401,8 +401,11 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
 
 #ifdef CONFIG_RCU_BOOST
 		/* Unboost if we were boosted. */
-		if (rbmp)
+		if (rbmp) {
+			local_irq_save(flags);
 			rt_mutex_unlock(rbmp);
+			local_irq_restore(flags);
+		}
 #endif /* #ifdef CONFIG_RCU_BOOST */
 
 		/*

kernel/rcutree_plugin.h static int rcu_boost(struct rcu_node *rnp) · line 1241

Quoted 7 lines

kernel/rcutree_plugin.h +1 -1

@@ -1233,9 +1236,10 @@ static int rcu_boost(struct rcu_node *rnp)
 	lockdep_set_class_and_name(&mtx.wait_lock, &rcu_boost_class,
 				   "rcu_boost_mutex");
 	t->rcu_boost_mutex = &mtx;
-	raw_spin_unlock_irqrestore(&rnp->lock, flags);
+	raw_spin_unlock(&rnp->lock); /* rrupts remain disabled. */
 	rt_mutex_lock(&mtx);  /* Side effect: boosts task t's priority. */
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */

We permit rt_mutex_unlock() to be call with irq disabled, but rt_mutex_lock() is still not allowed. So this usage is not legal now.

Quoted 1 line

Even after commit #5342e269b has been applied?

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Maybe we could teach might_sleep() about this special case?

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Maybe we could teach might_sleep() about this special case?

Quoted 3 lines · Yong Zhang

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Why can't this mutex acquisition not block?

Quoted 2 lines

Maybe we could teach might_sleep() about this special case?

Sounds horrid.

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Yong Zhang

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Why can't this mutex acquisition not block?

It could block. The issue it's legal to call rt_mutex_lock() with irqs disabled and we don't want might_sleep() bite us on this

special case. When we are going to sleep, we re-enable irq in __rt_mutex_slowlock().

Quoted 4 lines

Quoted 2 lines

Maybe we could teach might_sleep() about this special case?

Sounds horrid.

Maybe, any alternative?

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Maybe we could teach might_sleep() about this special case?

Sounds horrid.

Maybe, any alternative?

Maybe someone explain this mess first?

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Maybe we could teach might_sleep() about this special case?

Sounds horrid.

Maybe, any alternative?

Maybe someone explain this mess first?

Me too, because this looks like a hack that's just like a lie. The first lie to be said gets you out of a bit of trouble, but then something else happens based on that lie, in which you need to make another bigger lie. This new lie affects more people and requires new more ingenious lies to control the chaos. But eventually the lies required to keep everything going become so overwhelming that it all blows up in your face and you end up looking like a jackass.

Why is rcu using rt_mutex_lock() in strange ways? It's lying about its use. And now this patch is the bigger lie to get around the issues of the first lie. Eventually this code will continue to expand largely based on these lies and will explode in our faces, and I feel sorry for the poor jackass that needs to fix it.

Perhaps the real answer is that we need to create an API for priority inheritance, that things like RCU could use. Attach a task that another task requires to finish something and boost the priority of that task. Maybe even completions could use such a thing?

Quoted 25 lines · Steven Rostedt

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Maybe we could teach might_sleep() about this special case?

Sounds horrid.

Maybe, any alternative?

Maybe someone explain this mess first?

Me too, because this looks like a hack that's just like a lie. The first lie to be said gets you out of a bit of trouble, but then something else happens based on that lie, in which you need to make another bigger lie. This new lie affects more people and requires new more ingenious lies to control the chaos. But eventually the lies required to keep everything going become so overwhelming that it all blows up in your face and you end up looking like a jackass.

Why is rcu using rt_mutex_lock() in strange ways? It's lying about its use. And now this patch is the bigger lie to get around the issues of the first lie. Eventually this code will continue to expand largely based on these lies and will explode in our faces, and I feel sorry for the poor jackass that needs to fix it.

Quoted 4 lines

Perhaps the real answer is that we need to create an API for priority inheritance, that things like RCU could use. Attach a task that another task requires to finish something and boost the priority of that task. Maybe even completions could use such a thing?

I would be OK with that -- that was in fact the approach I was taking when I was advised to use mutexes instead. ;-)

Quoted 4 lines

Perhaps the real answer is that we need to create an API for priority inheritance, that things like RCU could use. Attach a task that another task requires to finish something and boost the priority of that task. Maybe even completions could use such a thing?

I would be OK with that -- that was in fact the approach I was taking when I was advised to use mutexes instead. ;-)

Quoted 7 lines · Paul E. McKenney

Quoted 4 lines

Perhaps the real answer is that we need to create an API for priority inheritance, that things like RCU could use. Attach a task that another task requires to finish something and boost the priority of that task. Maybe even completions could use such a thing?

I would be OK with that -- that was in fact the approach I was taking when I was advised to use mutexes instead. ;-)

Maybe we should rethink it. Using the makeshift mutex looks to be a short term hack. But if we are starting to build on it, it will end up being a horrible design, based off of a hack.

A mutex is to provide mutual exclusion. If we start bastardizing it to do other things, it will become unmaintainable. I dare say that it's close to unmaintainable now ;)

If we create a new API to handle inheritance, then perhaps it could be used for other things like workqueues and completions (in -rt only).

Quoted 3 lines · Yong Zhang

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Why can't this mutex acquisition not block?

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Yong Zhang

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Why can't this mutex acquisition not block?

Gaah!! I see, this 5342e269 patch is revolting.. guys that's really vile don't do that!

I tried reading the RCU code but I gave up.. rcu_boost() does:

  rt_mutex_init_proxy_locked();
  raw_spin_unlock_irqrestore();
  rt_mutex_lock();
  rt_mutex_unlock();

vs rcu_read_unlock_special()'s RCU_READ_UNLOCK_BLOCKED branch:

  rt_mutex_unlock();

The latter looks to be unbalanced because I can't actually find a matching lock. Also, all of that is ran with IRQs enabled. So what's the problem?

The problem with the IRQs enabled is the following sequence:

	rcu_read_lock();
	/* do stuff */
	local_irq_save(flags);
	/* do more stuff */
	rcu_read_unlock();
	/* do even more stuff */
	local_irq_restore(flags);

This has been legal in the past, and might well be used in places that -rt does not exercise, hence the desire to explicitly legalize it.

Quoted 3 lines · Yong Zhang

Yeah, because we call might_sleep() in rt_mutex_lock() unconditionally. But in this case the 'BUG: sleeping function called from invalid context at *' is obviously false positive.

Why can't this mutex acquisition not block?

Quoted 1 line

Maybe we could teach might_sleep() about this special case?

Sounds horrid.

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

Quoted 9 lines · Paul E. McKenney

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

Quoted 3 lines

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

So what does this patch do? Make it not complain when you do the above?

How often does this pattern actually happen?

Can't be that often
otherwise we'd have had more complaints, no?

Quoted 10 lines · Peter Zijlstra

Quoted 9 lines · Paul E. McKenney

From: Yong Zhang <[email protected]>

RCU-lockdep will issue warnings given the following use pattern:

	rcu_read_lock();
	local_irq_disable();
	rcu_read_unlock();
	local_irq_enable();

Quoted 5 lines

Quoted 3 lines

However, this use pattern is legal except for the scheduler's runqueue and priority-inheritance locks (and any other locks that the scheduler might use during priority-inheritance operations).

So what does this patch do? Make it not complain when you do the above?

It suppose to not complain but it bring other complain :(

Quoted 1 line

How often does this pattern actually happen?

IIRC, we have just one which is cured by commit [a841796: signal: align __lock_task_sighand() irq disabling and RCU]

Quoted 2 lines

Can't be that often
otherwise we'd have had more complaints, no?

Yeah,

So that also means we don't dedicated lock_class_key for mtx.wait_lock.

Thanks,
Yong