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

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

[wuchi]

Function wake_up_process always executes a general memory barrier,
so remove the mb() before it.

Signed-off-by: wuchi <wuchi.zero@gmail.com>
---
 lib/klist.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/klist.c b/lib/klist.c
index 332a4fbf18ff..c44498e31d91 100644
--- a/lib/klist.c
+++ b/lib/klist.c
@@ -194,7 +194,6 @@ static void klist_release(struct kref *kref)
 
 		list_del(&waiter->list);
 		waiter->woken = 1;
-		mb();
 		wake_up_process(waiter->process);
 	}
 	spin_unlock(&klist_remove_lock);
-- 
2.20.1

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

[Greg KH]

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?

> Signed-off-by: wuchi <wuchi.zero@gmail.com>

We need a "real name" for commits.

How did you test this patch?

thanks,

greg k-h

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

[chi wu]

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

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

[Greg KH]

On Wed, Jun 15, 2022 at 11:30:51AM +0800, chi wu wrote:
> 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.

So as the documentation states, it might not be there, so if you have to
have a memory barrier, you must not rely on this function to provide it.

So unless you have testing proof otherwise, the code should be correct
as-is.

thanks,

greg k-h