Re: [PATCH] lib/klist: Remove mb() before wake_up_process

chi wu
2022-06-15T03:31:11Z

Greg KH <gregkh@linuxfoundation.org> 于2022年6月14日周二 22:58写道：
>
> On Tue, Jun 14, 2022 at 10:44:43PM +0800, wuchi wrote:
> > Function wake_up_process always executes a general memory barrier,
> > so remove the mb() before it.
>
> Really? On all systems? I do not see that, where does it happen?
>
As I understand it, it is on all systems. Please help correct the
mistake, thanks.

1. Follow Documentation/memory-barriers.txt line 2128 ~ 2278,
especially line 2187 ~ 2202 snippet:
A general memory barrier is executed by wake_up() if it wakes
something up.
If it doesn't wake anything up then a memory barrier may or may not be
executed; you must not rely on it. The barrier occurs before
the task state
is accessed, in particular, it sits between the STORE to
indicate the event
and the STORE to set TASK_RUNNING:
CPU 1 (Sleeper) CPU 2 (Waker)
=============================== ===============================
set_current_state(); STORE
event_indicated
smp_store_mb(); wake_up();
STORE current->state ...
<general barrier>
<general barrier>
[*1-1*]
LOAD event_indicated if
((LOAD task->state) & TASK_NORMAL)

STORE task->state
where "task" is the thread being woken up and it equals CPU
1's "current".

2. Follow code wake_up_process in kernel/sched/core.c
wake_up_process
try_to_wake_up
....
raw_spin_lock_irqsave [*2-1*]
smp_mb__after_spinlock [*2-2*]
....

[*2-1*] and [*2-2*] will match [*1-1*], though smp_mb__after_spinlock
does nothing on most architectures,
but the architectures implement ACQUIRE with an smp_mb() after the
LL/SC loop, Following include/linux/spinlock.h
line 172 ~ 178.

3. Following lib/klist.c
klist_release and klist_remove conform to model "SLEEP AND WAKE-UP
FUNCTIONS" in Documentation/memory-barriers.txt,
so we do as the patch show.

> > Signed-off-by: wuchi <wuchi.zero@gmail.com>
>
> We need a "real name" for commits.
>
> How did you test this patch?

Sorry, I didn't. Just found the mb before wake_up_process could be
remove when reading the code,
Maybe you can view this as a question I asked.

thanks for your time