[RFC v1 0/3] Add simple work framework

[RFC v1 0/3] Add simple work framework

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Hi,

here is my next attempt to fix the crash I see on my system.

I spend considerable time reading up on memory barriers and etc.  Many
thanks to the authors of the excellent documentation
(memory-barriers.txt and Is Parallel Programming hard, And, If So,
What Can You Do About It?)!

After reading it I am pretty sure I got it wrong.

The framework is inspired by the irq_work.c infrastructure. Maybe it
could be merged somehow but I don't know if that is a clever idea.

cheers,
daniel

Daniel Wagner (2):
  work-simple: Simple work queue implemenation
  thermal: Defer thermal wakups to threads

Steven Rostedt (1):
  x86/mce: Defer mce wakeups to threads for PREEMPT_RT

 arch/x86/kernel/cpu/mcheck/mce.c       |  57 +++++++++---
 drivers/thermal/x86_pkg_temp_thermal.c |  48 +++++++++-
 include/linux/work-simple.h            |  35 +++++++
 kernel/sched/Makefile                  |   1 +
 kernel/sched/work-simple.c             | 165 +++++++++++++++++++++++++++++++++
 5 files changed, 292 insertions(+), 14 deletions(-)
 create mode 100644 include/linux/work-simple.h
 create mode 100644 kernel/sched/work-simple.c

-- 
1.9.3

[RFC v1 1/3] work-simple: Simple work queue implemenation

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

Provides a framework for enqueuing callbacks from irq context
PREEMPT_RT_FULL safe. The callbacks are executed in kthread context.

Bases on wait-simple.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/work-simple.h |  35 ++++++++++
 kernel/sched/Makefile       |   1 +
 kernel/sched/work-simple.c  | 165 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+)
 create mode 100644 include/linux/work-simple.h
 create mode 100644 kernel/sched/work-simple.c

diff --git a/include/linux/work-simple.h b/include/linux/work-simple.h
new file mode 100644
index 0000000..4cf169b
--- /dev/null
+++ b/include/linux/work-simple.h
@@ -0,0 +1,35 @@
+#ifndef _LINUX_SWORK_H
+#define _LINUX_SWORK_H
+
+#include <linux/list.h>
+#include <linux/atomic.h>
+
+/*
+ * An event can be in one of three states:
+ *
+ * free		0: free to be used
+ * pending	1: queued, pending callback
+ */
+
+#define SWORK_EVENT_PENDING	1UL
+
+struct swork_event {
+	unsigned long flags;
+	struct list_head list;
+	void (*func)(struct swork_event *);
+};
+
+static inline void init_swork_event(struct swork_event *event,
+					void (*func)(struct swork_event *))
+{
+	event->flags = 0;
+	INIT_LIST_HEAD(&event->list);
+	event->func = func;
+}
+
+void swork_queue(struct swork_event *event);
+
+int swork_get(void);
+void swork_put(void);
+
+#endif /* _LINUX_SWORK_H */
diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
index b14a512..539e287 100644
--- a/kernel/sched/Makefile
+++ b/kernel/sched/Makefile
@@ -14,6 +14,7 @@ endif
 obj-y += core.o proc.o clock.o cputime.o
 obj-y += idle_task.o fair.o rt.o deadline.o stop_task.o
 obj-y += wait.o wait-simple.o completion.o
+obj-y += work-simple.o
 obj-$(CONFIG_SMP) += cpupri.o cpudeadline.o
 obj-$(CONFIG_SCHED_AUTOGROUP) += auto_group.o
 obj-$(CONFIG_SCHEDSTATS) += stats.o
diff --git a/kernel/sched/work-simple.c b/kernel/sched/work-simple.c
new file mode 100644
index 0000000..2a8b2ca
--- /dev/null
+++ b/kernel/sched/work-simple.c
@@ -0,0 +1,165 @@
+/*
+ * Copyright (C) 2014 BMW Car IT GmbH, Daniel Wagner <daniel.wagner@bmw-carit.de>
+ *
+ * Provides a framework for enqueuing callbacks from irq context
+ * PREEMPT_RT_FULL safe. The callbacks are executed in kthread context.
+ */
+
+#include <linux/wait-simple.h>
+#include <linux/work-simple.h>
+#include <linux/kthread.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+/* protect swork_data */
+static DEFINE_MUTEX(worker_mutex);
+/* there is only one work thread available */
+static struct sworker *worker;
+
+struct sworker {
+	/* all pending work_events */
+	struct list_head events;
+	struct swait_head wq;
+	/* protects the events list */
+	raw_spinlock_t lock;
+	/* kthread */
+	struct task_struct *task;
+	/* number of sworker users */
+	int refs;
+};
+
+static bool swork_readable(struct sworker *worker)
+{
+	bool r;
+
+	if (kthread_should_stop())
+		return true;
+
+	raw_spin_lock(&worker->lock);
+	r = !list_empty(&worker->events);
+	raw_spin_unlock(&worker->lock);
+
+	return r;
+}
+
+static int swork_kthread(void *arg)
+{
+	struct sworker *sw = arg;
+	struct swork_event *ev;
+
+	pr_info("swork_kthread enter\n");
+
+	for (;;) {
+		swait_event_interruptible(sw->wq,
+					swork_readable(sw));
+		if (kthread_should_stop())
+			break;
+
+		raw_spin_lock(&sw->lock);
+		while (!list_empty(&sw->events)) {
+			ev = list_first_entry(&sw->events,
+					struct swork_event, list);
+			list_del_init(&ev->list);
+
+			raw_spin_unlock(&sw->lock);
+
+			ev->func(ev);
+			xchg(&ev->flags, 0);
+
+			raw_spin_lock(&sw->lock);
+		}
+		raw_spin_unlock(&sw->lock);
+
+	}
+
+	pr_info("swork_kthread exit\n");
+	return 0;
+}
+
+static struct sworker *swork_create(void)
+{
+	struct sworker *sw;
+
+	sw = kzalloc(sizeof(*sw), GFP_KERNEL);
+	if (!sw)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&sw->events);
+	raw_spin_lock_init(&sw->lock);
+	init_swait_head(&sw->wq);
+
+	sw->task = kthread_run(swork_kthread, sw, "swork_thread");
+	if (IS_ERR(sw->task)) {
+		kfree(sw);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	return sw;
+}
+
+static void swork_destroy(struct sworker *sw)
+{
+	struct swork_event *e, *tmp;
+
+	if (sw->task)
+		kthread_stop(sw->task);
+
+	list_for_each_entry_safe(e, tmp, &sw->events, list) {
+		list_del(&e->list);
+		e->func(e);
+	}
+
+	kfree(sw);
+}
+
+
+void swork_queue(struct swork_event *ev)
+{
+	if (cmpxchg(&ev->flags, 0, SWORK_EVENT_PENDING) != 0)
+		return;
+
+	raw_spin_lock(&worker->lock);
+	list_add(&worker->events, &ev->list);
+	raw_spin_unlock(&worker->lock);
+
+	swait_wake(&worker->wq);
+}
+EXPORT_SYMBOL_GPL(swork_queue);
+
+int swork_get(void)
+{
+	struct sworker *sw;
+
+	mutex_lock(&worker_mutex);
+	if (!worker) {
+		sw = swork_create();
+		if (IS_ERR(sw)) {
+			mutex_unlock(&worker_mutex);
+			return -ENOMEM;
+		}
+
+		worker = sw;
+	}
+
+	worker->refs++;
+	mutex_unlock(&worker_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(swork_get);
+
+void swork_put(void)
+{
+	mutex_lock(&worker_mutex);
+
+	worker->refs--;
+	if (worker->refs > 0)
+		goto out;
+
+	swork_destroy(worker);
+	worker = NULL;
+
+out:
+	mutex_unlock(&worker_mutex);
+}
+EXPORT_SYMBOL_GPL(swork_put);
-- 
1.9.3

[RFC v1 2/3] x86/mce: Defer mce wakeups to threads for PREEMPT_RT

[Daniel Wagner]

From: Steven Rostedt <rostedt@goodmis.org>

We had a customer report a lockup on a 3.0-rt kernel that had the
following backtrace:

[ffff88107fca3e80] rt_spin_lock_slowlock at ffffffff81499113
[ffff88107fca3f40] rt_spin_lock at ffffffff81499a56
[ffff88107fca3f50] __wake_up at ffffffff81043379
[ffff88107fca3f80] mce_notify_irq at ffffffff81017328
[ffff88107fca3f90] intel_threshold_interrupt at ffffffff81019508
[ffff88107fca3fa0] smp_threshold_interrupt at ffffffff81019fc1
[ffff88107fca3fb0] threshold_interrupt at ffffffff814a1853

It actually bugged because the lock was taken by the same owner that
already had that lock. What happened was the thread that was setting
itself on a wait queue had the lock when an MCE triggered. The MCE
interrupt does a wake up on its wait list and grabs the same lock.

NOTE: THIS IS NOT A BUG ON MAINLINE

Sorry for yelling, but as I Cc'd mainline maintainers I want them to
know that this is an PREEMPT_RT bug only. I only Cc'd them for advice.

On PREEMPT_RT the wait queue locks are converted from normal
"spin_locks" into an rt_mutex (see the rt_spin_lock_slowlock above).
These are not to be taken by hard interrupt context. This usually isn't
a problem as most all interrupts in PREEMPT_RT are converted into
schedulable threads. Unfortunately that's not the case with the MCE irq.

As wait queue locks are notorious for long hold times, we can not
convert them to raw_spin_locks without causing issues with -rt. But
Thomas has created a "simple-wait" structure that uses raw spin locks
which may have been a good fit.

Unfortunately, wait queues are not the only issue, as the mce_notify_irq
also does a schedule_work(), which grabs the workqueue spin locks that
have the exact same issue.

Thus, this patch I'm proposing is to move the actual work of the MCE
interrupt into a helper thread that gets woken up on the MCE interrupt
and does the work in a schedulable context.

NOTE: THIS PATCH ONLY CHANGES THE BEHAVIOR WHEN PREEMPT_RT IS SET

Oops, sorry for yelling again, but I want to stress that I keep the same
behavior of mainline when PREEMPT_RT is not set. Thus, this only changes
the MCE behavior when PREEMPT_RT is configured.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
[bigeasy@linutronix: make mce_notify_work() a proper prototype, use
		     kthread_run()]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
[wagi: use work-simple framework to defer work to a kthread]
Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 57 +++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 6c9174b..a58e6b4 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -42,6 +42,7 @@
 #include <linux/irq_work.h>
 #include <linux/export.h>
 #include <linux/jiffies.h>
+#include <linux/work-simple.h>
 
 #include <asm/processor.h>
 #include <asm/mce.h>
@@ -1355,6 +1356,47 @@ static void mce_do_trigger(struct work_struct *work)
 
 static DECLARE_WORK(mce_trigger_work, mce_do_trigger);
 
+static void __mce_notify_work(struct swork_event *event)
+{
+	/* Not more than two messages every minute */
+	static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
+
+	/* wake processes polling /dev/mcelog */
+	wake_up_interruptible(&mce_chrdev_wait);
+
+	/*
+	 * There is no risk of missing notifications because
+	 * work_pending is always cleared before the function is
+	 * executed.
+	 */
+	if (mce_helper[0] && !work_pending(&mce_trigger_work))
+		schedule_work(&mce_trigger_work);
+
+	if (__ratelimit(&ratelimit))
+		pr_info(HW_ERR "Machine check events logged\n");
+}
+
+#ifdef CONFIG_PREEMPT_RT_FULL
+struct swork_event notify_work;
+
+static int mce_notify_work_init(void)
+{
+	init_swork_event(&notify_work, __mce_notify_work);
+	return swork_get();
+}
+
+static void mce_notify_work(void)
+{
+	swork_queue(&notify_work);
+}
+#else
+static void mce_notify_work(void)
+{
+	__mce_notify_work(NULL);
+}
+static inline int mce_notify_work_init(void) { return 0; }
+#endif
+
 /*
  * Notify the user(s) about new machine check events.
  * Can be called from interrupt context, but not from machine check/NMI
@@ -1362,19 +1404,8 @@ static DECLARE_WORK(mce_trigger_work, mce_do_trigger);
  */
 int mce_notify_irq(void)
 {
-	/* Not more than two messages every minute */
-	static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
-
 	if (test_and_clear_bit(0, &mce_need_notify)) {
-		/* wake processes polling /dev/mcelog */
-		wake_up_interruptible(&mce_chrdev_wait);
-
-		if (mce_helper[0])
-			schedule_work(&mce_trigger_work);
-
-		if (__ratelimit(&ratelimit))
-			pr_info(HW_ERR "Machine check events logged\n");

[RFC v1 3/3] thermal: Defer thermal wakups to threads

[Daniel Wagner]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

On RT the spin lock in pkg_temp_thermal_platfrom_thermal_notify will
call schedule while we run in irq context.

[<ffffffff816850ac>] dump_stack+0x4e/0x8f
[<ffffffff81680f7d>] __schedule_bug+0xa6/0xb4
[<ffffffff816896b4>] __schedule+0x5b4/0x700
[<ffffffff8168982a>] schedule+0x2a/0x90
[<ffffffff8168a8b5>] rt_spin_lock_slowlock+0xe5/0x2d0
[<ffffffff8168afd5>] rt_spin_lock+0x25/0x30
[<ffffffffa03a7b75>] pkg_temp_thermal_platform_thermal_notify+0x45/0x134 [x86_pkg_temp_thermal]
[<ffffffff8103d4db>] ? therm_throt_process+0x1b/0x160
[<ffffffff8103d831>] intel_thermal_interrupt+0x211/0x250
[<ffffffff8103d8c1>] smp_thermal_interrupt+0x21/0x40
[<ffffffff8169415d>] thermal_interrupt+0x6d/0x80

Let's defer the work to a kthread.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/thermal/x86_pkg_temp_thermal.c | 48 ++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
index 081fd7e..a54ec65 100644
--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -29,6 +29,7 @@
 #include <linux/pm.h>
 #include <linux/thermal.h>
 #include <linux/debugfs.h>
+#include <linux/work-simple.h>
 #include <asm/cpu_device_id.h>
 #include <asm/mce.h>
 
@@ -352,7 +353,7 @@ static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
 	}
 }
 
-static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
+static void platform_thermal_notify_work(struct swork_event *event)
 {
 	unsigned long flags;
 	int cpu = smp_processor_id();
@@ -369,7 +370,7 @@ static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
 			pkg_work_scheduled[phy_id]) {
 		disable_pkg_thres_interrupt();
 		spin_unlock_irqrestore(&pkg_work_lock, flags);
-		return -EINVAL;
+		return;
 	}
 	pkg_work_scheduled[phy_id] = 1;
 	spin_unlock_irqrestore(&pkg_work_lock, flags);
@@ -378,9 +379,48 @@ static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
 	schedule_delayed_work_on(cpu,
 				&per_cpu(pkg_temp_thermal_threshold_work, cpu),
 				msecs_to_jiffies(notify_delay_ms));
+}
+
+#ifdef CONFIG_PREEMPT_RT_FULL
+static struct swork_event notify_work;
+
+static int thermal_notify_work_init(void)
+{
+	int err;
+
+	err = swork_get();
+	if (!err)
+		return err;
+
+	init_swork_event(&notify_work, platform_thermal_notify_work);
 	return 0;
 }
 
+static void thermal_notify_work_cleanup(void)
+{
+	swork_put();
+}
+
+static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
+{
+	swork_queue(&notify_work);
+	return 0;
+}
+
+#else  /* !CONFIG_PREEMPT_RT_FULL */
+
+static int thermal_notify_work_init(void) { return 0; }
+
+static int thermal_notify_work_cleanup(void) {  }
+
+static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
+{
+	platform_thermal_notify_work(NULL);
+
+	return 0;
+}
+#endif /* CONFIG_PREEMPT_RT_FULL */
+
 static int find_siblings_cpu(int cpu)
 {
 	int i;
@@ -594,6 +634,9 @@ static int __init pkg_temp_thermal_init(void)
 	for_each_online_cpu(i)
 		if (get_core_online(i))
 			goto err_ret;
+	if (!thermal_notify_work_init())
+		goto err_ret;
+
 	register_hotcpu_notifier(&pkg_temp_thermal_notifier);
 	put_online_cpus();
 
@@ -619,6 +662,7 @@ static void __exit pkg_temp_thermal_exit(void)
 
 	get_online_cpus();
 	unregister_hotcpu_notifier(&pkg_temp_thermal_notifier);
+	thermal_notify_work_cleanup();
 	mutex_lock(&phy_dev_list_mutex);
 	list_for_each_entry_safe(phdev, n, &phy_dev_list, list) {
 		/* Retore old MSR value for package thermal interrupt */
-- 
1.9.3

Re: [RFC v1 1/3] work-simple: Simple work queue implemenation

[Sebastian Andrzej Siewior]

* Daniel Wagner | 2014-07-11 15:26:11 [+0200]:

>diff --git a/kernel/sched/work-simple.c b/kernel/sched/work-simple.c
>--- /dev/null
>+++ b/kernel/sched/work-simple.c
…
>+static int swork_kthread(void *arg)
>+{
>+	struct sworker *sw = arg;
>+	struct swork_event *ev;
>+
>+	pr_info("swork_kthread enter\n");
>+
>+	for (;;) {
>+		swait_event_interruptible(sw->wq,
>+					swork_readable(sw));
>+		if (kthread_should_stop())
>+			break;
>+
>+		raw_spin_lock(&sw->lock);

why not the _irq() suffix?

>+		while (!list_empty(&sw->events)) {
>+			ev = list_first_entry(&sw->events,
>+					struct swork_event, list);
>+			list_del_init(&ev->list);
>+
>+			raw_spin_unlock(&sw->lock);
>+
>+			ev->func(ev);
>+			xchg(&ev->flags, 0);

I've been looking at this for a while and this won't work in longterm.
Why do you bother using xchg if you don't look at the return value?
Also, you need to clear that flag _before_ invoking ->func() because
while that function is beeing executed someone might do another
queue_work() and expects that that work item invoked again. Atleast this
is how queue_work() behaves and I would want the same here.

But don't worry. I fix this up to me needs so there is nothing you need
to worry about. The remaining part is simple. Thanks.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 1/3] work-simple: Simple work queue implemenation

[Daniel Wagner]

Hi Sebastian,

On 02/13/2015 01:09 PM, Sebastian Andrzej Siewior wrote:
> * Daniel Wagner | 2014-07-11 15:26:11 [+0200]:
> 
>> diff --git a/kernel/sched/work-simple.c b/kernel/sched/work-simple.c
>> --- /dev/null
>> +++ b/kernel/sched/work-simple.c
> …
>> +static int swork_kthread(void *arg)
>> +{
>> +	struct sworker *sw = arg;
>> +	struct swork_event *ev;
>> +
>> +	pr_info("swork_kthread enter\n");
>> +
>> +	for (;;) {
>> +		swait_event_interruptible(sw->wq,
>> +					swork_readable(sw));
>> +		if (kthread_should_stop())
>> +			break;
>> +
>> +		raw_spin_lock(&sw->lock);
> 
> why not the _irq() suffix?

Indeed. That should be with a _irq suffix.

>> +		while (!list_empty(&sw->events)) {
>> +			ev = list_first_entry(&sw->events,
>> +					struct swork_event, list);
>> +			list_del_init(&ev->list);
>> +
>> +			raw_spin_unlock(&sw->lock);
>> +
>> +			ev->func(ev);
>> +			xchg(&ev->flags, 0);
> 
> I've been looking at this for a while and this won't work in longterm.
> Why do you bother using xchg if you don't look at the return value?

Forgot to mention in the commit message (sorry about that), that I was
following what I saw in irq_work.c:__irq_work_run(). Looking at it again
(and reading up a lot on this topic) I agree that doesn't make sense.

> Also, you need to clear that flag _before_ invoking ->func() because
> while that function is beeing executed someone might do another
> queue_work() and expects that that work item invoked again. Atleast this
> is how queue_work() behaves and I would want the same here.

You are right. I tried to simplify what I saw in irq_work and only using
a PENDING flag instead of a PENDING and BUSY flag. Obviously, my version
is broken in that regard. I guess the BUSY flag is necessary.

> But don't worry. I fix this up to me needs so there is nothing you need
> to worry about. The remaining part is simple. Thanks.

Thanks a lot!

cheers,
daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html