kernel/kthread.c: need spin_lock_irq() for 'worker' before main looping, since it can "WARN_ON(worker->task)".

PATCH In review Chen Gang 3 participants 10 messages Jun 19, 2013 – Jun 20, 2013

Share & tools

Chen Gang

author

2013-06-19 04:04 UTC #0 lore raw

PATCH 1/1 kernel/kthread.c: need spin_lock_irq() for 'worker' before main looping, since it can "WARN_ON(worker->task)".

Since "WARN_ON(worker->task)", we can not assume that 'worker->task' will be NULL before set 'current' to it.

So need let 'worker' lock protected too, just like it already lock protected all time in main looping.

Signed-off-by Chen Gang <[email protected]>

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

kernel/kthread.c +2 -0

index 760e86d..8d572b8 100644
@@ -511,8 +511,10 @@ int kthread_worker_fn(void *worker_ptr)
 	struct kthread_worker *worker = worker_ptr;
 	struct kthread_work *work;
 
+	spin_lock_irq(&worker->lock);
 	WARN_ON(worker->task);
 	worker->task = current;
+	spin_unlock_irq(&worker->lock);
 repeat:
 	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */

Signature · 1 line

1.7.7.6

Tejun Heo

2013-06-19 08:41 UTC #1 lore raw

Quoted 6 lines · Chen Gang

Since "WARN_ON(worker->task)", we can not assume that 'worker->task' will be NULL before set 'current' to it.
So need let 'worker' lock protected too, just like it already lock protected all time in main looping.

That synchronization is the kthread_worker user's responsibility. The locking around worker->task = NULL is to prevent the worker task being destroyed while insert_kthread_work() is trying to wake it up. It has nothing to do with the user trying to attach multiple tasks to the same kthread_worker. Plus, putting locking around WARN_ON() is pointless. It doesn't really fix anything. It just makes WARN_ON() trigger *slightly* more reliably.

Thanks.

Signature · 1 line

tejun

Chen Gang

author

2013-06-19 10:18 UTC #2 lore raw

Quoted 7 lines · Tejun Heo

Quoted 6 lines · Chen Gang
Quoted 6 lines
Since "WARN_ON(worker->task)", we can not assume that 'worker->task' will be NULL before set 'current' to it.
So need let 'worker' lock protected too, just like it already lock protected all time in main looping.

Quoted 7 lines

That synchronization is the kthread_worker user's responsibility. The locking around worker->task = NULL is to prevent the worker task being destroyed while insert_kthread_work() is trying to wake it up. It has nothing to do with the user trying to attach multiple tasks to the same kthread_worker. Plus, putting locking around WARN_ON() is pointless. It doesn't really fix anything. It just makes WARN_ON() trigger *slightly* more reliably.

Hmm... can 'worker->task' has chance to be not NULL before set 'current' to it ?

why do we use WARN_ON(worker->task) ?

I guess it still has chance to let "worker->task != NULL", or it should be BUG_ON(worker->task) instead of.

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Tejun Heo

2013-06-19 15:52 UTC #3 lore raw

Quoted 2 lines · Chen Gang

Hmm... can 'worker->task' has chance to be not NULL before set 'current' to it ?

Yes, if the caller screws up and try to attach more than one workers to the kthread_worker, which has some possibility of happening as kthread_worker allows both attaching and detaching a worker.

Quoted 1 line

why do we use WARN_ON(worker->task) ?

To detect bugs on the caller side.

Quoted 2 lines

I guess it still has chance to let "worker->task != NULL", or it should be BUG_ON(worker->task) instead of.

What difference does that make?

Signature · 1 line

tejun

Chen Gang

author

2013-06-20 01:54 UTC #4 lore raw

Quoted 7 lines · Tejun Heo

Quoted 2 lines · Chen Gang
Quoted 2 lines
Hmm... can 'worker->task' has chance to be not NULL before set 'current' to it ?
Yes, if the caller screws up and try to attach more than one workers to the kthread_worker, which has some possibility of happening as kthread_worker allows both attaching and detaching a worker.

If we detect the bugs, and still want to use WARN_ON() to report warning and continue running, we need be sure of keeping the related things no touch (at least not lead to worse).

If we can not be sure of keeping the related things no touch:
if it is a kernel bug, better use BUG_ON() instead of,
if it is a user mode bug, better to return failure with error code and
print related information.

Quoted 3 lines

Quoted 1 line
Quoted 1 line
why do we use WARN_ON(worker->task) ?
To detect bugs on the caller side.

OK, thanks.

Quoted 3 lines

Quoted 2 lines
Quoted 2 lines
I guess it still has chance to let "worker->task != NULL", or it should be BUG_ON(worker->task) instead of.
What difference does that make?

BUG_ON() will stop current working flow and report kernel bug in details.

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Thomas Gleixner

2013-06-20 07:02 UTC #5 lore raw

Quoted 9 lines · Chen Gang

Quoted 7 lines · Tejun Heo
Quoted 2 lines · Chen Gang
Quoted 2 lines
Hmm... can 'worker->task' has chance to be not NULL before set 'current' to it ?
Yes, if the caller screws up and try to attach more than one workers to the kthread_worker, which has some possibility of happening as kthread_worker allows both attaching and detaching a worker.

Quoted 8 lines

If we detect the bugs, and still want to use WARN_ON() to report warning and continue running, we need be sure of keeping the related things no touch (at least not lead to worse).
If we can not be sure of keeping the related things no touch:
if it is a kernel bug, better use BUG_ON() instead of,
if it is a user mode bug, better to return failure with error code and
print related information.

Wrong. BUG_ON() is only for cases where the kernel CANNOT continue at all. WARN_ON() prints the very same information, but allows to continue.

Quoted 1 line

BUG_ON() will stop current working flow and report kernel bug in details.

There is no reason to crash the machine completely. The kernel can continue and the WARN_ON reports the bug with the same details.

Thanks,

tglx

Chen Gang

author

2013-06-20 07:38 UTC #6 lore raw

Quoted 15 lines · Thomas Gleixner

Quoted 13 lines · Chen Gang
Quoted 13 lines · Tejun Heo
Quoted 7 lines
Quoted 7 lines · Chen Gang
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Hmm... can 'worker->task' has chance to be not NULL before set 'current' to it ?
Yes, if the caller screws up and try to attach more than one workers to the kthread_worker, which has some possibility of happening as kthread_worker allows both attaching and detaching a worker.
If we detect the bugs, and still want to use WARN_ON() to report warning and continue running, we need be sure of keeping the related things no touch (at least not lead to worse).

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
If we can not be sure of keeping the related things no touch:
if it is a kernel bug, better use BUG_ON() instead of,
if it is a user mode bug, better to return failure with error code and
print related information.
Wrong. BUG_ON() is only for cases where the kernel CANNOT continue at all. WARN_ON() prints the very same information, but allows to continue.

In fact, BUG_ON() and WARN_ON() has various implementations in different architectures, and also can be configured by user.

Even some of 'crazy users' (e.g. randconfig), can make BUG_ON() and WARN_ON() 'empty' (include/asm-generic/bug.h).

In my experience (mainly for servers), when find a kernel bug, it will stop and report bug, that will let coredump analysing (or KDB trap) much easier.

Quoted 3 lines

Quoted 1 line
Quoted 1 line
BUG_ON() will stop current working flow and report kernel bug in details.
There is no reason to crash the machine completely. The kernel can continue and the WARN_ON reports the bug with the same details.

If so (we still prefer to use WARN_ON), we'd better to let it in lock protected.

At least when we still have to continue, try not to lead things worse.

It will provide much help for coredump analysing (or KDB trap).

In fact, for coredump analysers, for every real world coredump, they have to assume the system has already continued blindly, and then die.

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Thomas Gleixner

2013-06-20 08:29 UTC #7 lore raw

Quoted 20 lines · Chen Gang

Quoted 19 lines · Thomas Gleixner
Quoted 17 lines · Chen Gang
Quoted 17 lines · Tejun Heo
Quoted 7 lines
Quoted 7 lines · Chen Gang
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Hmm... can 'worker->task' has chance to be not NULL before set 'current' to it ?
Yes, if the caller screws up and try to attach more than one workers to the kthread_worker, which has some possibility of happening as kthread_worker allows both attaching and detaching a worker.
If we detect the bugs, and still want to use WARN_ON() to report warning and continue running, we need be sure of keeping the related things no touch (at least not lead to worse).
If we can not be sure of keeping the related things no touch:
if it is a kernel bug, better use BUG_ON() instead of,
if it is a user mode bug, better to return failure with error code and
print related information.

Quoted 7 lines

Quoted 4 lines
Wrong. BUG_ON() is only for cases where the kernel CANNOT continue at all. WARN_ON() prints the very same information, but allows to continue.
In fact, BUG_ON() and WARN_ON() has various implementations in different architectures, and also can be configured by user.

And how is that relevant?

Quoted 2 lines

Even some of 'crazy users' (e.g. randconfig), can make BUG_ON() and WARN_ON() 'empty' (include/asm-generic/bug.h).

That does not matter at all.

Quoted 3 lines

In my experience (mainly for servers), when find a kernel bug, it will stop and report bug, that will let coredump analysing (or KDB trap) much easier.

And your core dump will help you in what way? The code which misbehaved is not longer executing. The problem is detected after the fact and therefor your coredump will just tell you that worker->task is not NULL.

Quoted 3 lines

Quoted 3 lines
Quoted 1 line
Quoted 1 line
BUG_ON() will stop current working flow and report kernel bug in details.
There is no reason to crash the machine completely. The kernel can continue and the WARN_ON reports the bug with the same details.

Linus said about BUG_ON():

Adding BUG_ON()'s just makes things much much much worse. There is *never* a reason to add a BUG_ON().

BUG_ON() makes it almost impossible to debug something, because you just killed the machine. So using BUG_ON() for "please notice this" is stupid as hell, because the most common end result is: "Oh, the machine just hung with no messages".

And he is right about that.

Quoted 2 lines

If so (we still prefer to use WARN_ON), we'd better to let it in lock protected.

No, because the lock is not protecting anything in that case. If some other code misbehaves and sets worker->task, then the lock does not prevent this and taking the lock is not making the WARN_ON any more reliable. So why the heck should we take it?

Quoted 1 line

At least when we still have to continue, try not to lead things worse.

And what's going to be better if we take the lock? Nothing, because the lock CANNOT protect the check.

Quoted 4 lines

It will provide much help for coredump analysing (or KDB trap).
In fact, for coredump analysers, for every real world coredump, they have to assume the system has already continued blindly, and then die.

Core dump analysers cannot analyse dynamic race conditions and neither can KDB.

So what do you gain from crashing the kernel? Exactly NOTHING.

Thanks,

tglx

Chen Gang

author

2013-06-20 09:37 UTC #8 lore raw

Quoted 22 lines · Thomas Gleixner

Quoted 20 lines · Chen Gang
Quoted 20 lines · Thomas Gleixner
Quoted 19 lines
Quoted 19 lines · Chen Gang
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines · Tejun Heo
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Chen Gang
Hmm... can 'worker->task' has chance to be not NULL before set 'current'
to it ?
Yes, if the caller screws up and try to attach more than one workers
to the kthread_worker, which has some possibility of happening as
kthread_worker allows both attaching and detaching a worker.
If we detect the bugs, and still want to use WARN_ON() to report warning and continue running, we need be sure of keeping the related things no touch (at least not lead to worse).
If we can not be sure of keeping the related things no touch:
if it is a kernel bug, better use BUG_ON() instead of,
if it is a user mode bug, better to return failure with error code and
print related information.

Quoted 9 lines

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Wrong. BUG_ON() is only for cases where the kernel CANNOT continue at all. WARN_ON() prints the very same information, but allows to continue.
In fact, BUG_ON() and WARN_ON() has various implementations in different architectures, and also can be configured by user.
And how is that relevant?

I only want to say 'Wrong. BUG_ON() is only for cases where ...." is not quite precious.

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
Even some of 'crazy users' (e.g. randconfig), can make BUG_ON() and WARN_ON() 'empty' (include/asm-generic/bug.h).
That does not matter at all.

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
In my experience (mainly for servers), when find a kernel bug, it will stop and report bug, that will let coredump analysing (or KDB trap) much easier.
And your core dump will help you in what way? The code which misbehaved is not longer executing. The problem is detected after the fact and therefor your coredump will just tell you that worker->task is not NULL.

In this case, if generate a coredump, it will provide much help to analyze the issues.

Normally, this coredump is not belongs to complex coredump.

I met a complex KDB trap (at least for me, it is complex, maybe easy for others):
When a driver is quiting, it releases the dma buffers firstly, then
immediately tell the hardware to stop dma usage.
After the hardware writes 'a little waste data' to the released
buffer, the driver quite successfully.
Then the driver restart again, and work normally.
After 'a long period', the system finds random issues (sometimes for
ext3, sometimes for mm, block, or anywhere...).

I spend almost 1 month to find the root cause (from 2008-12 to 2009-01).

So if it generates a coredump (or KDB trap) when find bug, in most cases, it is not a quite complex coredump.

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
BUG_ON() will stop current working flow and report kernel bug in details.
There is no reason to crash the machine completely. The kernel can continue and the WARN_ON reports the bug with the same details.
Linus said about BUG_ON():
Adding BUG_ON()'s just makes things much much much worse. There is *never* a reason to add a BUG_ON().

Quoted 7 lines

BUG_ON() makes it almost impossible to debug something, because you just killed the machine. So using BUG_ON() for "please notice this" is stupid as hell, because the most common end result is: "Oh, the machine just hung with no messages".
And he is right about that.

Why we provide BUG_ON(), and many sub-systems also use it, at last ?

and Why we integrated KDB and KGDB at last ?

Quoted 7 lines

Quoted 2 lines
Quoted 2 lines
If so (we still prefer to use WARN_ON), we'd better to let it in lock protected.
No, because the lock is not protecting anything in that case. If some other code misbehaves and sets worker->task, then the lock does not prevent this and taking the lock is not making the WARN_ON any more reliable. So why the heck should we take it?

For writing code, if 'worker->task' is not NULL, we can assume, it need lock protected.

Quoted 11 lines

Quoted 1 line
Quoted 1 line
At least when we still have to continue, try not to lead things worse.
And what's going to be better if we take the lock? Nothing, because the lock CANNOT protect the check.
Quoted 4 lines
Quoted 4 lines
It will provide much help for coredump analysing (or KDB trap).
In fact, for coredump analysers, for every real world coredump, they have to assume the system has already continued blindly, and then die.
Core dump analysers cannot analyse dynamic race conditions and neither can KDB.

Yes, they can.

At least for me, I have at least 20 successful experiences (2009 - 2010) for coredump, dead lock, memory leak, and busy looping under user mode system services (the related code about 400K).

For kernel, I also have some successful experience for coredump.

Quoted 1 line

So what do you gain from crashing the kernel? Exactly NOTHING.

So, the coredump (or KDB trap) is really useful for some guys (at least for me).

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Thomas Gleixner

2013-06-19 08:44 UTC #9 lore raw

In reply to Chen Gang

Quoted 3 lines · Chen Gang

Since "WARN_ON(worker->task)", we can not assume that 'worker->task' will be NULL before set 'current' to it.

It better is NULL and all that WARN_ON does is to verify that.

Quoted 2 lines

So need let 'worker' lock protected too, just like it already lock protected all time in main looping.

No. That's pointless. This happens when the new worker starts up and there is nothing which can modify worker->task at this point.

Thanks,

tglx