linux-next: build failure after merge of the rcu tree

[Stephen Rothwell]

Hi Paul,

After merging the rcu tree, today's linux-next build (arm
multi_v7_defconfig) failed like this:

kernel/sched/core.c: In function 'do_task_dead':
kernel/sched/core.c:3385:2: error: implicit declaration of function 'smp_mb__before_spinlock' [-Werror=implicit-function-declaration]
smp_mb__before_spinlock();
^
cc1: some warnings being treated as errors

Caused by commit

4a6fc6107e90 ("sched: Replace spin_unlock_wait() with lock/unlock pair")

Interacting with commit

a9668cd6ee28 ("locking: Remove smp_mb__before_spinlock()")

from the tip tree.

I applied this patch for now, but I assume something better is required.

From: Stephen Rothwell <s...@canb.auug.org.au>
Date: Fri, 11 Aug 2017 14:32:10 +1000
Subject: [PATCH] sched: temporary hack for locking: Remove smp_mb__before_spinlock()

Signed-off-by: Stephen Rothwell <s...@canb.auug.org.au>
---
kernel/sched/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2bd00feaea15..a4f4ba2e3be6 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3382,7 +3382,7 @@ void __noreturn do_task_dead(void)
* To avoid it, we have to wait for releasing tsk->pi_lock which
* is held by try_to_wake_up()
*/
- smp_mb__before_spinlock();
+ smp_mb();
raw_spin_lock_irq(&current->pi_lock);
raw_spin_unlock_irq(&current->pi_lock);

--
Cheers,
Stephen Rothwell

[Paul E. McKenney]

On Fri, Aug 11, 2017 at 02:43:52PM +1000, Stephen Rothwell wrote:
> Hi Paul,
>
> After merging the rcu tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
>
> kernel/sched/core.c: In function 'do_task_dead':
> kernel/sched/core.c:3385:2: error: implicit declaration of function 'smp_mb__before_spinlock' [-Werror=implicit-function-declaration]
> smp_mb__before_spinlock();
> ^
> cc1: some warnings being treated as errors
>
> Caused by commit
>
> 4a6fc6107e90 ("sched: Replace spin_unlock_wait() with lock/unlock pair")
>
> Interacting with commit
>
> a9668cd6ee28 ("locking: Remove smp_mb__before_spinlock()")
>
> from the tip tree.
>
> I applied this patch for now, but I assume something better is required.

Looks like I need to rebase my patch on top of a9668cd6ee28, and
than put an smp_mb__after_spinlock() between the lock and the unlock.

Peter, any objections to that approach? Other suggestions?

Thanx, Paul

[Peter Zijlstra]

On Thu, Aug 10, 2017 at 09:54:53PM -0700, Paul E. McKenney wrote:
> On Fri, Aug 11, 2017 at 02:43:52PM +1000, Stephen Rothwell wrote:
>
> Looks like I need to rebase my patch on top of a9668cd6ee28, and
> than put an smp_mb__after_spinlock() between the lock and the unlock.
>
> Peter, any objections to that approach? Other suggestions?

Hurm.. I'll have to try and understand that comment there again it
seems.

[Paul E. McKenney]

My reasoning is as follows:

1. The critical section is empty, so any prior references
would be ordered only against later critical sections.

2. A full barrier within the critical section will order those
prior references against later critical sections just
as easily as would one prior to the critical section.

Does that make sense, I should I have stayed away from the keyboard
at this early hour? ;-)

Thanx, Paul

[Peter Zijlstra]

On Fri, Aug 11, 2017 at 11:14:34AM +0200, Peter Zijlstra wrote:

OK, so per commit b5740f4b2cb3 ("sched: Fix ancient race in do_exit()")
the race is with try_to_wake_up():

down_read()
p->state = TASK_UNINTERRUPTIBLE;

try_to_wake_up(p)
spin_lock(p->pi_lock);
/* sees TASK_UNINTERRUPTIBLE */
ttwu_remote()
/* check stuff, no need to schedule() */
p->state = TASK_RUNNING


p->state = TASK_DEAD

p->state = TASK_RUNNING /* whoops! */
spin_unlock(p->pi_lock);

__schedule(false);
BUG();




So given that, I think that:

spin_lock(&current->pi_lock);
spin_unlock(&current->pi_lock);

current->state = TASK_DEAD;

is sufficient. I don't see a need for an additional smp_mb here.

Either the concurrent ttwu is finished and we must observe its RUNNING
store, or it will observe our RUNNING store.

[Peter Zijlstra]

So I think we can do away with 2 because our prior and later stores have
an address dependency (they are to the same variable) and thus must be
ordered already.

Basically:

CPU0 CPU1

p->state = TASK_UNINTERRUPTIBLE;
try_to_wake_up(p)

p->state = TASK_RUNNING
spin_lock(&p->pi_lock);
spin_unlock(&p->pi_lock);
p->state = TASK_DEAD

Now, the ttwu(p) NO-OPs unless it sees (UN)INTERRUPTIBLE, so either
RUNNING or DEAD are fine. However if it sees (UN)INTERRUPTIBLE it will
do another (competing) RUNNING store which must not overwrite DEAD.

[Paul E. McKenney]

Makes sense to me! Please see below for the updated commit.

Thanx, Paul

------------------------------------------------------------------------

commit 23a9b748a3d27f67cdb078fcb891a920285e75d9
Author: Paul E. McKenney <pau...@linux.vnet.ibm.com>
Date: Thu Jun 29 12:08:26 2017 -0700

sched: Replace spin_unlock_wait() with lock/unlock pair

There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair. This commit therefore replaces the spin_unlock_wait() call in
do_task_dead() with spin_lock() followed immediately by spin_unlock().
This should be safe from a performance perspective because the lock is
this tasks ->pi_lock, and this is called only after the task exits.

Signed-off-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>
Cc: Ingo Molnar <mi...@redhat.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Will Deacon <will....@arm.com>
Cc: Alan Stern <st...@rowland.harvard.edu>
Cc: Andrea Parri <parri....@gmail.com>
Cc: Linus Torvalds <torv...@linux-foundation.org>
[ paulmck: Drop smp_mb() based on Peter Zijlstra's analysis:
http://lkml.kernel.org/r/20170811144150....@hirez.programming.kicks-ass.net ]

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 17c667b427b4..5d22323ae099 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3352,8 +3352,8 @@ void __noreturn do_task_dead(void)

* To avoid it, we have to wait for releasing tsk->pi_lock which
* is held by try_to_wake_up()
*/

- smp_mb();
- raw_spin_unlock_wait(&current->pi_lock);
+ raw_spin_lock_irq(&current->pi_lock);
+ raw_spin_unlock_irq(&current->pi_lock);

/* Causes final put_task_struct in finish_task_switch(): */
__set_current_state(TASK_DEAD);