Re: [PATCH v3 09/22] kthread: Allow to cancel kthread work

[Linus Torvalds <torvalds@linux-foundation.org>]

On Mon, Nov 23, 2015 at 2:58 PM, Tejun Heo <tj@kernel.org> wrote:
>
> And the timer can do (ignoring the multiple worker support, do we even
> need that?)
>
>         while (!trylock(worker)) {
>                 if (work->canceling)
>                         return;
>                 cpu_relax();
>         }

No no no!

People, you need to learn that code like the above is *not*
acceptable. It's busy-looping on a spinlock, and constantly trying to
*write* to the spinlock.

It will literally crater performance on a multi-socket SMP system if
it ever triggers. We're talking 10x slowdowns, and absolutely
unacceptable cache coherency traffic.

These kinds of loops absolutely *have* to have the read-only part. The
"cpu_relax()" above needs to be a loop that just tests the lock state
by *reading* it, so the cpu_relax() needs to be replaced with
something like

        while (spin_is_locked(lock)) cpu_relax();

instead (possibly just "spin_unlock_wait()" - but the explicit loop
might be worth it if you then want to check the "canceling" flag
independently of the lock state too).

In general, it's very dangerous to try to cook up your own locking
rules. People *always* get it wrong.

                      Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>