[PATCH] core: workqueue: BUG_ON on workqueue recursion

[Simon Kagstrom]

When the workqueue is flushed from workqueue context (recursively), the
system enters a strange state where things at random (dependent on the
global workqueue) start misbehaving. For example, for us the console and
logins locks up while the web server continues running.

Since the system becomes unstable, change this to a BUG_ON instead.

Signed-off-by: Simon Kagstrom <simon.k...@netinsight.net>
---
kernel/workqueue.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index dee4865..e617d29 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -482,7 +482,7 @@ static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
int active = 0;
struct wq_barrier barr;

- WARN_ON(cwq->thread == current);
+ BUG_ON(cwq->thread == current);

spin_lock_irq(&cwq->lock);
if (!list_empty(&cwq->worklist) || cwq->current_work != NULL) {
--
1.6.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Oleg Nesterov]

On 02/03, Simon Kagstrom wrote:
>
> When the workqueue is flushed from workqueue context (recursively), the
> system enters a strange state where things at random (dependent on the
> global workqueue) start misbehaving. For example, for us the console and
> logins locks up while the web server continues running.
>
> Since the system becomes unstable, change this to a BUG_ON instead.

I agree with this patch. We are going to deadlock anyway, if the
condition is true the caller is cwq->current_work, this means
flush_cpu_workqueue() will insert the barrier and hang.

However,

> @@ -482,7 +482,7 @@ static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
> int active = 0;
> struct wq_barrier barr;
>
> - WARN_ON(cwq->thread == current);
> + BUG_ON(cwq->thread == current);

Another option is change the code to do

if (WARN_ON(cwq->thread == current))
return;

This gives the kernel chance to survive after the warning.

What do you think?

Oleg.

[Lai Jiangshan]

Simon Kagstrom wrote:
> When the workqueue is flushed from workqueue context (recursively), the
> system enters a strange state where things at random (dependent on the
> global workqueue) start misbehaving. For example, for us the console and
> logins locks up while the web server continues running.
>
> Since the system becomes unstable, change this to a BUG_ON instead.

For design view, we should disallow this recursion when using workqueue.

I like BUG_ON. But it is not a fatal end usually when it happens,
most developers would like to let system go on.

Acked-by: Lai Jiangshan <la...@cn.fujitsu.com>

>
> Signed-off-by: Simon Kagstrom <simon.k...@netinsight.net>
> ---
> kernel/workqueue.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index dee4865..e617d29 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -482,7 +482,7 @@ static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
> int active = 0;
> struct wq_barrier barr;
>
> - WARN_ON(cwq->thread == current);
> + BUG_ON(cwq->thread == current);
>
> spin_lock_irq(&cwq->lock);
> if (!list_empty(&cwq->worklist) || cwq->current_work != NULL) {

--

[Tejun Heo]

Hello,

On 02/04/2010 04:43 AM, Oleg Nesterov wrote:
> On 02/03, Simon Kagstrom wrote:
>>
>> When the workqueue is flushed from workqueue context (recursively), the
>> system enters a strange state where things at random (dependent on the
>> global workqueue) start misbehaving. For example, for us the console and
>> logins locks up while the web server continues running.
>>
>> Since the system becomes unstable, change this to a BUG_ON instead.
>
> I agree with this patch. We are going to deadlock anyway, if the
> condition is true the caller is cwq->current_work, this means
> flush_cpu_workqueue() will insert the barrier and hang.
>
> However,
>
>> @@ -482,7 +482,7 @@ static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
>> int active = 0;
>> struct wq_barrier barr;
>>
>> - WARN_ON(cwq->thread == current);
>> + BUG_ON(cwq->thread == current);
>
> Another option is change the code to do
>
> if (WARN_ON(cwq->thread == current))
> return;
>
> This gives the kernel chance to survive after the warning.
>
> What do you think?

Yeah, I like this one better too. Even solely for debugging,
WARN_ON() is better as often users don't have reliable ways to gather
kernel log after a BUG_ON().

Thanks.

--
tejun

[Simon Kagstrom]

When the workqueue is flushed from workqueue context (recursively), the
system enters a strange state where things at random (dependent on the
global workqueue) start misbehaving. For example, for us the console and
logins locks up while the web server continues running.

The system becomes unstable since the workqueue barrier locks the
workqueue. This patch instead returns if the workqueue is flushed
recursively, which keeps the workqueue alive but warns.

Signed-off-by: Simon Kagstrom <simon.k...@netinsight.net>
---

ChangeLog:
* Instead of BUG_ON, warn and return on recursive calls as suggested
by Oleg Nesterov and Tejun Hao

kernel/workqueue.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index dee4865..49f8fa7 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -482,7 +482,8 @@ static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)

int active = 0;
struct wq_barrier barr;

- WARN_ON(cwq->thread == current);

+ if (WARN_ON(cwq->thread == current))
+ return 1;

spin_lock_irq(&cwq->lock);
if (!list_empty(&cwq->worklist) || cwq->current_work != NULL) {
--
1.6.0.4

--

[Oleg Nesterov]

On 02/04, Simon Kagstrom wrote:
>
> When the workqueue is flushed from workqueue context (recursively), the
> system enters a strange state where things at random (dependent on the
> global workqueue) start misbehaving. For example, for us the console and
> logins locks up while the web server continues running.
>
> The system becomes unstable since the workqueue barrier locks the
> workqueue. This patch instead returns if the workqueue is flushed
> recursively, which keeps the workqueue alive but warns.
>
> Signed-off-by: Simon Kagstrom <simon.k...@netinsight.net>

Acked-by: Oleg Nesterov <ol...@redhat.com>

[Tejun Heo]

On 02/04/2010 05:02 PM, Simon Kagstrom wrote:
> When the workqueue is flushed from workqueue context (recursively), the
> system enters a strange state where things at random (dependent on the
> global workqueue) start misbehaving. For example, for us the console and
> logins locks up while the web server continues running.
>
> The system becomes unstable since the workqueue barrier locks the
> workqueue. This patch instead returns if the workqueue is flushed
> recursively, which keeps the workqueue alive but warns.
>
> Signed-off-by: Simon Kagstrom <simon.k...@netinsight.net>

applied to wq tree. Will push out when the merge window opens.

Thanks.

--
tejun