Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1260655 > unrolled thread

[PATCH 0/4] scheduler ordering bits

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] scheduler ordering bits Peter Zijlstra <peterz@infradead.org> - 2015-11-02 15:00 +0100
    [PATCH 2/4] sched: Document Program-Order guarantees Peter Zijlstra <peterz@infradead.org> - 2015-11-02 15:00 +0100
      Re: [PATCH 2/4] sched: Document Program-Order guarantees Paul Turner <pjt@google.com> - 2015-11-02 21:30 +0100
        Re: [PATCH 2/4] sched: Document Program-Order guarantees Peter Zijlstra <peterz@infradead.org> - 2015-11-02 21:40 +0100
          Re: [PATCH 2/4] sched: Document Program-Order guarantees Paul Turner <pjt@google.com> - 2015-11-02 23:10 +0100
            Re: [PATCH 2/4] sched: Document Program-Order guarantees Peter Zijlstra <peterz@infradead.org> - 2015-11-02 23:20 +0100
    [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-02 15:00 +0100
      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-02 15:00 +0100
        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-02 18:50 +0100
          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-03 02:20 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-03 02:30 +0100
      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-02 18:50 +0100
      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-02 19:10 +0100
        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-02 19:40 +0100
          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-02 20:20 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-02 21:00 +0100
              Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-02 21:30 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-02 23:00 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-03 03:00 +0100
              Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-03 20:50 +0100
                Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-04 05:00 +0100
                  Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-04 05:50 +0100
                    Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-04 14:10 +0100
        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() David Howells <dhowells@redhat.com> - 2015-11-02 21:40 +0100
          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-02 21:50 +0100
          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-02 22:20 +0100
      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Oleg Nesterov <oleg@redhat.com> - 2015-11-03 18:10 +0100
        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-03 19:30 +0100
        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Boqun Feng <boqun.feng@gmail.com> - 2015-11-11 10:50 +0100
          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Boqun Feng <boqun.feng@gmail.com> - 2015-11-11 11:40 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Oleg Nesterov <oleg@redhat.com> - 2015-11-11 20:00 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-12 15:00 +0100
          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-11 13:20 +0100
            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Oleg Nesterov <oleg@redhat.com> - 2015-11-11 19:50 +0100
              Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-11 22:30 +0100
              Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Boqun Feng <boqun.feng@gmail.com> - 2015-11-12 08:20 +0100
                Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-12 11:30 +0100
                Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Oleg Nesterov <oleg@redhat.com> - 2015-11-12 15:10 +0100
                  Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Boqun Feng <boqun.feng@gmail.com> - 2015-11-12 15:50 +0100
                    Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-12 16:50 +0100
                      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-12 23:00 +0100
                  Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-12 15:50 +0100
                    Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-12 16:00 +0100
                      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-12 16:10 +0100
                        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-12 16:10 +0100
                          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-12 16:30 +0100
                            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-12 22:30 +0100
                  Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Boqun Feng <boqun.feng@gmail.com> - 2015-11-12 16:20 +0100
                    Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Oleg Nesterov <oleg@redhat.com> - 2015-11-12 18:50 +0100
                      Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Peter Zijlstra <peterz@infradead.org> - 2015-11-12 19:10 +0100
                        Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Oleg Nesterov <oleg@redhat.com> - 2015-11-12 19:40 +0100
                          Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-12 20:00 +0100
                            Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-12 22:40 +0100
                              Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-13 00:50 +0100
                Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-12 19:30 +0100
                  Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Will Deacon <will.deacon@arm.com> - 2015-11-12 23:10 +0100
    [PATCH 1/4] sched: Better document the try_to_wake_up() barriers Peter Zijlstra <peterz@infradead.org> - 2015-11-02 15:00 +0100
    [PATCH 3/4] sched: Fix a race in try_to_wake_up() vs schedule() Peter Zijlstra <peterz@infradead.org> - 2015-11-02 15:00 +0100

Page 3 of 3 — ← Prev page 1 2 [3]

#1268293 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 07:02:51AM -0800, Paul E. McKenney wrote:
> On Thu, Nov 12, 2015 at 10:49:02PM +0800, Boqun Feng wrote:
> > On Thu, Nov 12, 2015 at 06:40:04AM -0800, Paul E. McKenney wrote:
> > [snip]
> > > 
> > > I cannot resist suggesting that any lock that interacts with
> > > spin_unlock_wait() must have all relevant acquisitions followed by
> > > smp_mb__after_unlock_lock().
> > > 
> > 
> > But
> > 
> > 1.	This would expand the purpose of smp_mb__after_unlock_lock(),
> > 	right? smp_mb__after_unlock_lock() is for making UNLOCK-LOCK
> > 	pair global transitive rather than guaranteeing no operations
> > 	can be reorder before the STORE part of LOCK/ACQUIRE.
> 
> Indeed it would.  Which might be OK.
> 
> > 2.	If ARM64 has the same problem as PPC now,
> > 	smp_mb__after_unlock_lock() can't help, as it's a no-op on
> > 	ARM64.
> 
> Agreed, and that is why we need Will to weigh in.

I really don't want to implement smp_mb__after_unlock_lock, because we
don't need it based on its current definition and I think there's a better
way to fix spin_unlock_wait (see my other post).

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267999 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 04:00:58PM +0100, Oleg Nesterov wrote:
> On 11/12, Boqun Feng wrote:
> >
> > On Wed, Nov 11, 2015 at 08:39:53PM +0100, Oleg Nesterov wrote:
> > >
> > > 	object_t *object;
> > > 	spinlock_t lock;
> > >
> > > 	void update(void)
> > > 	{
> > > 		object_t *o;
> > >
> > > 		spin_lock(&lock);
> > > 		o = READ_ONCE(object);
> > > 		if (o) {
> > > 			BUG_ON(o->dead);
> > > 			do_something(o);
> > > 		}
> > > 		spin_unlock(&lock);
> > > 	}
> > >
> > > 	void destroy(void) // can be called only once, can't race with itself
> > > 	{
> > > 		object_t *o;
> > >
> > > 		o = object;
> > > 		object = NULL;
> > >
> > > 		/*
> > > 		 * pairs with lock/ACQUIRE. The next update() must see
> > > 		 * object == NULL after spin_lock();
> > > 		 */
> > > 		smp_mb();
> > >
> > > 		spin_unlock_wait(&lock);
> > >
> > > 		/*
> > > 		 * pairs with unlock/RELEASE. The previous update() has
> > > 		 * already passed BUG_ON(o->dead).
> > > 		 *
> > > 		 * (Yes, yes, in this particular case it is not needed,
> > > 		 *  we can rely on the control dependency).
> > > 		 */
> > > 		smp_mb();
> > >
> > > 		o->dead = true;
> > > 	}
> > >
> > > I believe the code above is correct and it needs the barriers on both sides.
> > >
> >
> > Hmm.. probably incorrect.. because the ACQUIRE semantics of spin_lock()
> > only guarantees that the memory operations following spin_lock() can't
> > be reorder before the *LOAD* part of spin_lock() not the *STORE* part,
> > i.e. the case below can happen(assuming the spin_lock() is implemented
> > as ll/sc loop)
> >
> > 	spin_lock(&lock):
> > 	  r1 = *lock; // LL, r1 == 0
> > 	o = READ_ONCE(object); // could be reordered here.
> > 	  *lock = 1; // SC
> >
> > This could happen because of the ACQUIRE semantics of spin_lock(), and
> > the current implementation of spin_lock() on PPC allows this happen.
> >
> > (Cc PPC maintainers for their opinions on this one)
> 
> In this case the code above is obviously wrong. And I do not understand
> how we can rely on spin_unlock_wait() then.
> 
> And afaics do_exit() is buggy too then, see below.
> 
> > I think it's OK for it as an ACQUIRE(with a proper barrier) or even just
> > a control dependency to pair with spin_unlock(), for example, the
> > following snippet in do_exit() is OK, except the smp_mb() is redundant,
> > unless I'm missing something subtle:
> >
> > 	/*
> > 	 * The setting of TASK_RUNNING by try_to_wake_up() may be delayed
> > 	 * when the following two conditions become true.
> > 	 *   - There is race condition of mmap_sem (It is acquired by
> > 	 *     exit_mm()), and
> > 	 *   - SMI occurs before setting TASK_RUNINNG.
> > 	 *     (or hypervisor of virtual machine switches to other guest)
> > 	 *  As a result, we may become TASK_RUNNING after becoming TASK_DEAD
> > 	 *
> > 	 * 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(&tsk->pi_lock);
> 
> Perhaps it is me who missed something. But I don't think we can remove
> this mb(). And at the same time it can't help on PPC if I understand
> your explanation above correctly.

I cannot resist suggesting that any lock that interacts with
spin_unlock_wait() must have all relevant acquisitions followed by
smp_mb__after_unlock_lock().

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268004 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 06:40:04AM -0800, Paul E. McKenney wrote:
> I cannot resist suggesting that any lock that interacts with
> spin_unlock_wait() must have all relevant acquisitions followed by
> smp_mb__after_unlock_lock().

Ha! that would certainly help here. But it would mean that argh64v8 also
needs to define that, even though that is already RCsc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268013 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 03:50:13PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 12, 2015 at 06:40:04AM -0800, Paul E. McKenney wrote:
> > I cannot resist suggesting that any lock that interacts with
> > spin_unlock_wait() must have all relevant acquisitions followed by
> > smp_mb__after_unlock_lock().
> 
> Ha! that would certainly help here. But it would mean that argh64v8 also
> needs to define that, even though that is already RCsc.

Maybe.  It could also be that arm64 avoids the need somehow, for example
via their RCsc behavior.  Their memory model is similar to PPC, but not
exactly the same.

Will?

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268019 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 07:01:49AM -0800, Paul E. McKenney wrote:
> On Thu, Nov 12, 2015 at 03:50:13PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 12, 2015 at 06:40:04AM -0800, Paul E. McKenney wrote:
> > > I cannot resist suggesting that any lock that interacts with
> > > spin_unlock_wait() must have all relevant acquisitions followed by
> > > smp_mb__after_unlock_lock().
> > 
> > Ha! that would certainly help here. But it would mean that argh64v8 also
> > needs to define that, even though that is already RCsc.
> 
> Maybe.  It could also be that arm64 avoids the need somehow, for example
> via their RCsc behavior.  Their memory model is similar to PPC, but not
> exactly the same.
> 
> Will?

So when I spoke to Will earlier today, we agreed that LDAXR+STXR is
susceptible to the same problem. The STXR will allow loads to pass up
over that store.

On v8.1, which is using LDADDA, this is not an issue, for as the ACQUIRE
is part of the LOAD, the Read-Modify-Write is indivisible as a whole,
and therefore a subsequent load has nothing to pass over.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268028 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 04:08:22PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 12, 2015 at 07:01:49AM -0800, Paul E. McKenney wrote:
> > On Thu, Nov 12, 2015 at 03:50:13PM +0100, Peter Zijlstra wrote:
> > > On Thu, Nov 12, 2015 at 06:40:04AM -0800, Paul E. McKenney wrote:
> > > > I cannot resist suggesting that any lock that interacts with
> > > > spin_unlock_wait() must have all relevant acquisitions followed by
> > > > smp_mb__after_unlock_lock().
> > > 
> > > Ha! that would certainly help here. But it would mean that argh64v8 also
> > > needs to define that, even though that is already RCsc.
> > 
> > Maybe.  It could also be that arm64 avoids the need somehow, for example
> > via their RCsc behavior.  Their memory model is similar to PPC, but not
> > exactly the same.
> > 
> > Will?
> 
> So when I spoke to Will earlier today, we agreed that LDAXR+STXR is
> susceptible to the same problem. The STXR will allow loads to pass up
> over that store.
> 
> On v8.1, which is using LDADDA, this is not an issue, for as the ACQUIRE
> is part of the LOAD, the Read-Modify-Write is indivisible as a whole,
> and therefore a subsequent load has nothing to pass over.

So one approach required for one level of hardware and another for the
next level.  I can relate to that all too well...  :-/

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268284 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

[sorry for the late reply, I'm away from my desk until Monday since I'm
 busy with family issues]

On Thu, Nov 12, 2015 at 07:20:42AM -0800, Paul E. McKenney wrote:
> On Thu, Nov 12, 2015 at 04:08:22PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 12, 2015 at 07:01:49AM -0800, Paul E. McKenney wrote:
> > > On Thu, Nov 12, 2015 at 03:50:13PM +0100, Peter Zijlstra wrote:
> > > > On Thu, Nov 12, 2015 at 06:40:04AM -0800, Paul E. McKenney wrote:
> > > > > I cannot resist suggesting that any lock that interacts with
> > > > > spin_unlock_wait() must have all relevant acquisitions followed by
> > > > > smp_mb__after_unlock_lock().
> > > > 
> > > > Ha! that would certainly help here. But it would mean that argh64v8 also
> > > > needs to define that, even though that is already RCsc.
> > > 
> > > Maybe.  It could also be that arm64 avoids the need somehow, for example
> > > via their RCsc behavior.  Their memory model is similar to PPC, but not
> > > exactly the same.
> > > 
> > > Will?
> > 
> > So when I spoke to Will earlier today, we agreed that LDAXR+STXR is
> > susceptible to the same problem. The STXR will allow loads to pass up
> > over that store.
> > 
> > On v8.1, which is using LDADDA, this is not an issue, for as the ACQUIRE
> > is part of the LOAD, the Read-Modify-Write is indivisible as a whole,
> > and therefore a subsequent load has nothing to pass over.
> 
> So one approach required for one level of hardware and another for the
> next level.  I can relate to that all too well...  :-/

Just to confirm, Peter's correct in that Boqun's litmus test is permitted
by the arm64 architecture when the ll/sc spinlock definitions are in use.

However, I don't think that strengthening smp_mb__after_unlock_lock is
the right way to solve this. I'll reply to the other part of the thread...

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268023 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 12, 2015 at 04:00:58PM +0100, Oleg Nesterov wrote:
> On 11/12, Boqun Feng wrote:
[snip]
> >
> > Hmm.. probably incorrect.. because the ACQUIRE semantics of spin_lock()
> > only guarantees that the memory operations following spin_lock() can't
> > be reorder before the *LOAD* part of spin_lock() not the *STORE* part,
> > i.e. the case below can happen(assuming the spin_lock() is implemented
> > as ll/sc loop)
> >
> > 	spin_lock(&lock):
> > 	  r1 = *lock; // LL, r1 == 0
> > 	o = READ_ONCE(object); // could be reordered here.
> > 	  *lock = 1; // SC
> >
> > This could happen because of the ACQUIRE semantics of spin_lock(), and
> > the current implementation of spin_lock() on PPC allows this happen.
> >
> > (Cc PPC maintainers for their opinions on this one)
> 
> In this case the code above is obviously wrong. And I do not understand
> how we can rely on spin_unlock_wait() then.
> 
> And afaics do_exit() is buggy too then, see below.
> 
> > I think it's OK for it as an ACQUIRE(with a proper barrier) or even just
> > a control dependency to pair with spin_unlock(), for example, the
> > following snippet in do_exit() is OK, except the smp_mb() is redundant,
> > unless I'm missing something subtle:
> >
> > 	/*
> > 	 * The setting of TASK_RUNNING by try_to_wake_up() may be delayed
> > 	 * when the following two conditions become true.
> > 	 *   - There is race condition of mmap_sem (It is acquired by
> > 	 *     exit_mm()), and
> > 	 *   - SMI occurs before setting TASK_RUNINNG.
> > 	 *     (or hypervisor of virtual machine switches to other guest)
> > 	 *  As a result, we may become TASK_RUNNING after becoming TASK_DEAD
> > 	 *
> > 	 * 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(&tsk->pi_lock);
> 
> Perhaps it is me who missed something. But I don't think we can remove
> this mb(). And at the same time it can't help on PPC if I understand

You are right, we need this smp_mb() to order the previous STORE of
->state with the LOAD of ->pi_lock. I missed that part because I saw all
the explicit STOREs of ->state in do_exit() are set_current_state()
which has a smp_mb() following the STOREs.

> your explanation above correctly.
> 
> To simplify, lets ignore exit_mm/down_read/etc. The exiting task does
> 
> 
> 	current->state = TASK_UNINTERRUPTIBLE;
> 	// without schedule() in between
> 	current->state = TASK_RUNNING;
> 
> 	smp_mb();
> 	spin_unlock_wait(pi_lock);
> 
> 	current->state = TASK_DEAD;
> 	schedule();
> 
> and we need to ensure that if we race with try_to_wake_up(TASK_UNINTERRUPTIBLE)
> it can't change TASK_DEAD back to RUNNING.
> 
> Without smp_mb() this can be reordered, spin_unlock_wait(pi_locked) can
> read the old "unlocked" state of pi_lock before we set UNINTERRUPTIBLE,
> so in fact we could have
> 
> 	current->state = TASK_UNINTERRUPTIBLE;
> 	
> 	spin_unlock_wait(pi_lock);
> 
> 	current->state = TASK_RUNNING;
> 
> 	current->state = TASK_DEAD;
> 
> and this can obviously race with ttwu() which can take pi_lock and see
> state == TASK_UNINTERRUPTIBLE after spin_unlock_wait().
> 

Yep, my mistake ;-)

> And, if I understand you correctly, this smp_mb() can't help on PPC.
> try_to_wake_up() can read task->state before it writes to *pi_lock.
> To me this doesn't really differ from the code above,
> 
> 	CPU 1 (do_exit)				CPU_2 (ttwu)
> 
> 						spin_lock(pi_lock):
> 						  r1 = *pi_lock; // r1 == 0;
> 	p->state = TASK_UNINTERRUPTIBLE;
> 						state = p->state;
> 	p->state = TASK_RUNNING;
> 	mb();
> 	spin_unlock_wait();
> 						*pi_lock = 1;
> 
> 	p->state = TASK_DEAD;
> 						if (state & TASK_UNINTERRUPTIBLE) // true
> 							p->state = RUNNING;
> 
> No?
> 

do_exit() is surely buggy if spin_lock() could work in this way.

> And smp_mb__before_spinlock() looks wrong too then.
> 

Maybe not? As smp_mb__before_spinlock() is used before a LOCK operation,
which has both LOAD part and STORE part unlike spin_unlock_wait()?

> Oleg.
> 

[toc] | [prev] | [next] | [standalone]

#1268140 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On 11/12, Boqun Feng wrote:
>
> On Thu, Nov 12, 2015 at 04:00:58PM +0100, Oleg Nesterov wrote:
> >
> > No?
> >
>
> do_exit() is surely buggy if spin_lock() could work in this way.

OK, good ;) so we need to fix spin_lock() on PPC ? Or add
mb__after_unlock_lock() but this leads to other questions... Or
change do_exit() to do lock() + unlock().

>
> > And smp_mb__before_spinlock() looks wrong too then.
> >
>
> Maybe not? As smp_mb__before_spinlock() is used before a LOCK operation,
> which has both LOAD part and STORE part unlike spin_unlock_wait()?

Maybe not.

But let me remind that the original purpose of this mb__before_spinlock()
was to ensure that "CONDITION = true" before ttwu() can not be reordered
with

	if (!(p->state & state))
		goto out;		// do not wakeup


inside try_to_wake_up(). Otherwise

	CONDITION = true;
	try_to_wake_up(p);

can race with "p" doing

	set_current_state(...);	// implies mb();
	if (CONDITION)
		return;
	schedule();

because try_to_wake_up() can read p->state before it sets CONDITION = 1
and then it won't wakeup "p" which has already checked this CONDITION.


Now. If try_to_wake_up() can read p->state before it writes to *pi_lock,
then how smp_mb__before_spinlock() == wmb() can help to serialize STORE
and LOAD?

It seems that PPC needs to define smp_mb__before_spinlock() as full mb(),
no?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268165 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 07:38:07PM +0100, Oleg Nesterov wrote:
> It seems that PPC needs to define smp_mb__before_spinlock() as full mb(),
> no?

It does:

arch/powerpc/include/asm/barrier.h:#define smp_mb__before_spinlock() smp_mb()

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268188 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On 11/12, Peter Zijlstra wrote:
>
> On Thu, Nov 12, 2015 at 07:38:07PM +0100, Oleg Nesterov wrote:
> > It seems that PPC needs to define smp_mb__before_spinlock() as full mb(),
> > no?
>
> It does:
>
> arch/powerpc/include/asm/barrier.h:#define smp_mb__before_spinlock() smp_mb()

Ah, indeed, thanks.

And given that it also defines smp_mb__after_unlock_lock() as smp_mb(),
I am starting to understand how it can help to avoid the races with
spin_unlock_wait() in (for example) do_exit().

But as Boqun has already mentioned, this means that mb__after_unlock_lock()
has the new meaning which should be documented.

Hmm. And 12d560f4 "Privatize smp_mb__after_unlock_lock()" should be reverted
then ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268208 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 08:33:02PM +0100, Oleg Nesterov wrote:
> On 11/12, Peter Zijlstra wrote:
> >
> > On Thu, Nov 12, 2015 at 07:38:07PM +0100, Oleg Nesterov wrote:
> > > It seems that PPC needs to define smp_mb__before_spinlock() as full mb(),
> > > no?
> >
> > It does:
> >
> > arch/powerpc/include/asm/barrier.h:#define smp_mb__before_spinlock() smp_mb()
> 
> Ah, indeed, thanks.
> 
> And given that it also defines smp_mb__after_unlock_lock() as smp_mb(),
> I am starting to understand how it can help to avoid the races with
> spin_unlock_wait() in (for example) do_exit().
> 
> But as Boqun has already mentioned, this means that mb__after_unlock_lock()
> has the new meaning which should be documented.
> 
> Hmm. And 12d560f4 "Privatize smp_mb__after_unlock_lock()" should be reverted
> then ;)

Surprisingly, this reverts cleanly against today's mainline, please see
the patch below.  Against my -rcu stack, not so much, but so it goes.  ;-)

							Thanx, Paul

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

commit eff0632b4181f91f2596d56f7c73194e1a869aff
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Thu Nov 12 10:54:23 2015 -0800

    Revert "rcu,locking: Privatize smp_mb__after_unlock_lock()"
    
    This reverts commit 12d560f4ea87030667438a169912380be00cea4b.
    
    The reason for this revert is that smp_mb__after_unlock_lock() might
    prove useful outside of RCU after all for interactions between
    the locking primitives and spin_unlock_wait().

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index aef9487303d0..d4501664d49f 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1855,10 +1855,16 @@ RELEASE are to the same lock variable, but only from the perspective of
 another CPU not holding that lock.  In short, a ACQUIRE followed by an
 RELEASE may -not- be assumed to be a full memory barrier.
 
-Similarly, the reverse case of a RELEASE followed by an ACQUIRE does
-not imply a full memory barrier.  Therefore, the CPU's execution of the
-critical sections corresponding to the RELEASE and the ACQUIRE can cross,
-so that:
+Similarly, the reverse case of a RELEASE followed by an ACQUIRE does not
+imply a full memory barrier.  If it is necessary for a RELEASE-ACQUIRE
+pair to produce a full barrier, the ACQUIRE can be followed by an
+smp_mb__after_unlock_lock() invocation.  This will produce a full barrier
+(including transitivity) if either (a) the RELEASE and the ACQUIRE are
+executed by the same CPU or task, or (b) the RELEASE and ACQUIRE act on
+the same variable.  The smp_mb__after_unlock_lock() primitive is free
+on many architectures.  Without smp_mb__after_unlock_lock(), the CPU's
+execution of the critical sections corresponding to the RELEASE and the
+ACQUIRE can cross, so that:
 
 	*A = a;
 	RELEASE M
@@ -1896,6 +1902,29 @@ the RELEASE would simply complete, thereby avoiding the deadlock.
 	a sleep-unlock race, but the locking primitive needs to resolve
 	such races properly in any case.
 
+With smp_mb__after_unlock_lock(), the two critical sections cannot overlap.
+For example, with the following code, the store to *A will always be
+seen by other CPUs before the store to *B:
+
+	*A = a;
+	RELEASE M
+	ACQUIRE N
+	smp_mb__after_unlock_lock();
+	*B = b;
+
+The operations will always occur in one of the following orders:
+
+	STORE *A, RELEASE, ACQUIRE, smp_mb__after_unlock_lock(), STORE *B
+	STORE *A, ACQUIRE, RELEASE, smp_mb__after_unlock_lock(), STORE *B
+	ACQUIRE, STORE *A, RELEASE, smp_mb__after_unlock_lock(), STORE *B
+
+If the RELEASE and ACQUIRE were instead both operating on the same lock
+variable, only the first of these alternatives can occur.  In addition,
+the more strongly ordered systems may rule out some of the above orders.
+But in any case, as noted earlier, the smp_mb__after_unlock_lock()
+ensures that the store to *A will always be seen as happening before
+the store to *B.
+
 Locks and semaphores may not provide any guarantee of ordering on UP compiled
 systems, and so cannot be counted on in such a situation to actually achieve
 anything at all - especially with respect to I/O accesses - unless combined
@@ -2126,6 +2155,40 @@ But it won't see any of:
 	*E, *F or *G following RELEASE Q
 
 
+However, if the following occurs:
+
+	CPU 1				CPU 2
+	===============================	===============================
+	WRITE_ONCE(*A, a);
+	ACQUIRE M		     [1]
+	WRITE_ONCE(*B, b);
+	WRITE_ONCE(*C, c);
+	RELEASE M	     [1]
+	WRITE_ONCE(*D, d);		WRITE_ONCE(*E, e);
+					ACQUIRE M		     [2]
+					smp_mb__after_unlock_lock();
+					WRITE_ONCE(*F, f);
+					WRITE_ONCE(*G, g);
+					RELEASE M	     [2]
+					WRITE_ONCE(*H, h);
+
+CPU 3 might see:
+
+	*E, ACQUIRE M [1], *C, *B, *A, RELEASE M [1],
+		ACQUIRE M [2], *H, *F, *G, RELEASE M [2], *D
+
+But assuming CPU 1 gets the lock first, CPU 3 won't see any of:
+
+	*B, *C, *D, *F, *G or *H preceding ACQUIRE M [1]
+	*A, *B or *C following RELEASE M [1]
+	*F, *G or *H preceding ACQUIRE M [2]
+	*A, *B, *C, *E, *F or *G following RELEASE M [2]
+
+Note that the smp_mb__after_unlock_lock() is critically important
+here: Without it CPU 3 might see some of the above orderings.
+Without smp_mb__after_unlock_lock(), the accesses are not guaranteed
+to be seen in order unless CPU 3 holds lock M.
+
 
 ACQUIRES VS I/O ACCESSES
 ------------------------
diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
index 523673d7583c..4dbe072eecbe 100644
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -28,6 +28,8 @@
 #include <asm/synch.h>
 #include <asm/ppc-opcode.h>
 
+#define smp_mb__after_unlock_lock()	smp_mb()  /* Full ordering for lock. */
+
 #ifdef CONFIG_PPC64
 /* use 0x800000yy when locked, where yy == CPU number */
 #ifdef __BIG_ENDIAN__
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 47dd0cebd204..ffcd053ca89a 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -130,6 +130,16 @@ do {								\
 #define smp_mb__before_spinlock()	smp_wmb()
 #endif
 
+/*
+ * Place this after a lock-acquisition primitive to guarantee that
+ * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
+ * if the UNLOCK and LOCK are executed by the same CPU or if the
+ * UNLOCK and LOCK operate on the same lock variable.
+ */
+#ifndef smp_mb__after_unlock_lock
+#define smp_mb__after_unlock_lock()	do { } while (0)
+#endif
+
 /**
  * raw_spin_unlock_wait - wait until the spinlock gets unlocked
  * @lock: the spinlock in question.
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 9fb4e238d4dc..8c6753d903ec 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -652,15 +652,3 @@ static inline void rcu_nocb_q_lengths(struct rcu_data *rdp, long *ql, long *qll)
 #endif /* #else #ifdef CONFIG_RCU_NOCB_CPU */
 }
 #endif /* #ifdef CONFIG_RCU_TRACE */
-
-/*
- * Place this after a lock-acquisition primitive to guarantee that
- * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
- * if the UNLOCK and LOCK are executed by the same CPU or if the
- * UNLOCK and LOCK operate on the same lock variable.
- */
-#ifdef CONFIG_PPC
-#define smp_mb__after_unlock_lock()	smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
-#define smp_mb__after_unlock_lock()	do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268287 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 10:59:06AM -0800, Paul E. McKenney wrote:
> On Thu, Nov 12, 2015 at 08:33:02PM +0100, Oleg Nesterov wrote:
> > On 11/12, Peter Zijlstra wrote:
> > >
> > > On Thu, Nov 12, 2015 at 07:38:07PM +0100, Oleg Nesterov wrote:
> > > > It seems that PPC needs to define smp_mb__before_spinlock() as full mb(),
> > > > no?
> > >
> > > It does:
> > >
> > > arch/powerpc/include/asm/barrier.h:#define smp_mb__before_spinlock() smp_mb()
> > 
> > Ah, indeed, thanks.
> > 
> > And given that it also defines smp_mb__after_unlock_lock() as smp_mb(),
> > I am starting to understand how it can help to avoid the races with
> > spin_unlock_wait() in (for example) do_exit().
> > 
> > But as Boqun has already mentioned, this means that mb__after_unlock_lock()
> > has the new meaning which should be documented.
> > 
> > Hmm. And 12d560f4 "Privatize smp_mb__after_unlock_lock()" should be reverted
> > then ;)
> 
> Surprisingly, this reverts cleanly against today's mainline, please see
> the patch below.  Against my -rcu stack, not so much, but so it goes.  ;-)

I think we ended up concluding that smp_mb__after_unlock_lock is indeed
required, but I don't think we should just resurrect the old definition,
which doesn't keep UNLOCK -> LOCK distinct from RELEASE -> ACQUIRE. I'm
still working on documenting the different types of transitivity that we
identified in that thread, but it's slow going.

Also, as far as spin_unlock_wait is concerned, it is neither a LOCK or
an UNLOCK and this barrier doesn't offer us anything. Sure, it might
work because PPC defines it as smp_mb(), but it doesn't help on arm64
and defining the macro is overkill for us in most places (i.e. RCU).

If we decide that the current usage of spin_unlock_wait is valid, then I
would much rather implement a version of it in the arm64 backend that
does something like:

 1:  ldrex r1, [&lock]
     if r1 indicates that lock is taken, branch back to 1b
     strex r1, [&lock]
     if store failed, branch back to 1b

i.e. we don't just test the lock, but we also write it back atomically
if we discover that it's free. That would then clear the exclusive monitor
on any cores in the process of taking the lock and restore the ordering
that we need.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268405 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 09:33:39PM +0000, Will Deacon wrote:
> On Thu, Nov 12, 2015 at 10:59:06AM -0800, Paul E. McKenney wrote:
> > On Thu, Nov 12, 2015 at 08:33:02PM +0100, Oleg Nesterov wrote:
> > > On 11/12, Peter Zijlstra wrote:
> > > >
> > > > On Thu, Nov 12, 2015 at 07:38:07PM +0100, Oleg Nesterov wrote:
> > > > > It seems that PPC needs to define smp_mb__before_spinlock() as full mb(),
> > > > > no?
> > > >
> > > > It does:
> > > >
> > > > arch/powerpc/include/asm/barrier.h:#define smp_mb__before_spinlock() smp_mb()
> > > 
> > > Ah, indeed, thanks.
> > > 
> > > And given that it also defines smp_mb__after_unlock_lock() as smp_mb(),
> > > I am starting to understand how it can help to avoid the races with
> > > spin_unlock_wait() in (for example) do_exit().
> > > 
> > > But as Boqun has already mentioned, this means that mb__after_unlock_lock()
> > > has the new meaning which should be documented.
> > > 
> > > Hmm. And 12d560f4 "Privatize smp_mb__after_unlock_lock()" should be reverted
> > > then ;)
> > 
> > Surprisingly, this reverts cleanly against today's mainline, please see
> > the patch below.  Against my -rcu stack, not so much, but so it goes.  ;-)
> 
> I think we ended up concluding that smp_mb__after_unlock_lock is indeed
> required, but I don't think we should just resurrect the old definition,
> which doesn't keep UNLOCK -> LOCK distinct from RELEASE -> ACQUIRE. I'm
> still working on documenting the different types of transitivity that we
> identified in that thread, but it's slow going.
> 
> Also, as far as spin_unlock_wait is concerned, it is neither a LOCK or
> an UNLOCK and this barrier doesn't offer us anything. Sure, it might
> work because PPC defines it as smp_mb(), but it doesn't help on arm64
> and defining the macro is overkill for us in most places (i.e. RCU).
> 
> If we decide that the current usage of spin_unlock_wait is valid, then I
> would much rather implement a version of it in the arm64 backend that
> does something like:
> 
>  1:  ldrex r1, [&lock]
>      if r1 indicates that lock is taken, branch back to 1b
>      strex r1, [&lock]
>      if store failed, branch back to 1b
> 
> i.e. we don't just test the lock, but we also write it back atomically
> if we discover that it's free. That would then clear the exclusive monitor
> on any cores in the process of taking the lock and restore the ordering
> that we need.

We could clearly do something similar in PowerPC, but I suspect that this
would hurt really badly on large systems, given that there are PowerPC
systems with more than a thousand hardware threads.  So one approach
is ARM makes spin_unlock_wait() do the write, similar to spin_lock();
spin_lock(), but PowerPC relies on smp_mb__after_unlock_lock().

Or does someone have a better proposal?

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268179 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Wed, Nov 11, 2015 at 11:14 PM, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hmm.. probably incorrect.. because the ACQUIRE semantics of spin_lock()
> only guarantees that the memory operations following spin_lock() can't
> be reorder before the *LOAD* part of spin_lock() not the *STORE* part,
> i.e. the case below can happen(assuming the spin_lock() is implemented
> as ll/sc loop)
>
>         spin_lock(&lock):
>           r1 = *lock; // LL, r1 == 0
>         o = READ_ONCE(object); // could be reordered here.
>           *lock = 1; // SC

It may be worth noting that at least in theory, not only reads may
pass the store. If the spin-lock is done as

        r1 = *lock   // read-acquire, r1 == 0
        *lock = 1    // SC

then even *writes* inside the locked region might pass up through the
"*lock = 1".

So other CPU's - that haven't taken the spinlock - could see the
modifications inside the critical region before they actually see the
lock itself change.

Now, the point of spin_unlock_wait() (and "spin_is_locked()") should
generally be that you have some external ordering guarantee that
guarantees that the lock has been taken. For example, for the IPC
semaphores, we do either one of:

 (a) get large lock, then - once you hold that lock - wait for each small lock

or

 (b) get small lock, then - once you hold that lock - check that the
largo lock is unlocked

and that's the case we should really worry about.  The other uses of
spin_unlock_wait() should have similar "I have other reasons to know
I've seen that the lock was taken, or will never be taken after this
because XYZ".

This is why powerpc has a memory barrier in "arch_spin_is_locked()".
Exactly so that the "check that the other lock is unlocked" is
guaranteed to be ordered wrt the store that gets the first lock.

It looks like ARM64 gets this wrong and is fundamentally buggy wrt
"spin_is_locked()" (and, as a result, "spin_unlock_wait()").

BUT! And this is a bug BUT:

It should be noted that that is purely an ARM64 bug. Not a bug in our
users. If you have a spinlock where the "get lock write" part of the
lock can be delayed, then you have to have a "arch_spin_is_locked()"
that has the proper memory barriers.

Of course, ARM still hides their architecture manuals in odd places,
so I can't double-check. But afaik, ARM64 store-conditional is "store
exclusive with release", and it has only release semantics, and ARM64
really does have the above bug.

On that note: can anybody point me to the latest ARM64 8.1
architecture manual in pdf form, without the "you have to register"
crap? I thought ARM released it, but all my googling just points to
the idiotic ARM service center that wants me to sign away something
just to see the docs. Which I don't do.

                                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268295 — Re: [PATCH 4/4] locking: Introduce smp_cond_acquire()

On Thu, Nov 12, 2015 at 10:21:39AM -0800, Linus Torvalds wrote:
> On Wed, Nov 11, 2015 at 11:14 PM, Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Hmm.. probably incorrect.. because the ACQUIRE semantics of spin_lock()
> > only guarantees that the memory operations following spin_lock() can't
> > be reorder before the *LOAD* part of spin_lock() not the *STORE* part,
> > i.e. the case below can happen(assuming the spin_lock() is implemented
> > as ll/sc loop)
> >
> >         spin_lock(&lock):
> >           r1 = *lock; // LL, r1 == 0
> >         o = READ_ONCE(object); // could be reordered here.
> >           *lock = 1; // SC
> 
> It may be worth noting that at least in theory, not only reads may
> pass the store. If the spin-lock is done as
> 
>         r1 = *lock   // read-acquire, r1 == 0
>         *lock = 1    // SC
> 
> then even *writes* inside the locked region might pass up through the
> "*lock = 1".

Right, but only if we didn't have the control dependency to branch back
in the case that the SC failed. In that case, the lock would be broken
because you'd dive into the critical section even if you failed to take
the lock.

> So other CPU's - that haven't taken the spinlock - could see the
> modifications inside the critical region before they actually see the
> lock itself change.
> 
> Now, the point of spin_unlock_wait() (and "spin_is_locked()") should
> generally be that you have some external ordering guarantee that
> guarantees that the lock has been taken. For example, for the IPC
> semaphores, we do either one of:
> 
>  (a) get large lock, then - once you hold that lock - wait for each small lock
> 
> or
> 
>  (b) get small lock, then - once you hold that lock - check that the
> largo lock is unlocked
> 
> and that's the case we should really worry about.  The other uses of
> spin_unlock_wait() should have similar "I have other reasons to know
> I've seen that the lock was taken, or will never be taken after this
> because XYZ".
> 
> This is why powerpc has a memory barrier in "arch_spin_is_locked()".
> Exactly so that the "check that the other lock is unlocked" is
> guaranteed to be ordered wrt the store that gets the first lock.
> 
> It looks like ARM64 gets this wrong and is fundamentally buggy wrt
> "spin_is_locked()" (and, as a result, "spin_unlock_wait()").

I don't see how a memory barrier would help us here, and Boqun's example
had smp_mb() either sides of the spin_unlock_wait(). What we actually need
is to make spin_unlock_wait more like a LOCK operation, so that it forces
parallel lockers to replay their LL/SC sequences.

I'll write a patch once I've heard more back from Paul about
smp_mb__after_unlock_lock().

> BUT! And this is a bug BUT:
> 
> It should be noted that that is purely an ARM64 bug. Not a bug in our
> users. If you have a spinlock where the "get lock write" part of the
> lock can be delayed, then you have to have a "arch_spin_is_locked()"
> that has the proper memory barriers.
> 
> Of course, ARM still hides their architecture manuals in odd places,
> so I can't double-check. But afaik, ARM64 store-conditional is "store
> exclusive with release", and it has only release semantics, and ARM64
> really does have the above bug.

The store-conditional in our spin_lock routine has no barrier semantics;
they are enforced by the load-exclusive-acquire that pairs with it.

> On that note: can anybody point me to the latest ARM64 8.1
> architecture manual in pdf form, without the "you have to register"
> crap? I thought ARM released it, but all my googling just points to
> the idiotic ARM service center that wants me to sign away something
> just to see the docs. Which I don't do.

We haven't yet released a document for the 8.1 instructions, but I can
certainly send you a copy when it's made available.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260659 — [PATCH 1/4] sched: Better document the try_to_wake_up() barriers

Explain how the control dependency and smp_rmb() end up providing
ACQUIRE semantics and pair with smp_store_release() in
finish_lock_switch().

Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/core.c  |    8 +++++++-
 kernel/sched/sched.h |    3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1947,7 +1947,13 @@ try_to_wake_up(struct task_struct *p, un
 	while (p->on_cpu)
 		cpu_relax();
 	/*
-	 * Pairs with the smp_wmb() in finish_lock_switch().
+	 * Combined with the control dependency above, we have an effective
+	 * smp_load_acquire() without the need for full barriers.
+	 *
+	 * Pairs with the smp_store_release() in finish_lock_switch().
+	 *
+	 * This ensures that tasks getting woken will be fully ordered against
+	 * their previous state and preserve Program Order.
 	 */
 	smp_rmb();
 
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1073,6 +1073,9 @@ static inline void finish_lock_switch(st
 	 * We must ensure this doesn't happen until the switch is completely
 	 * finished.
 	 *
+	 * In particular, the load of prev->state in finish_task_switch() must
+	 * happen before this.
+	 *
 	 * Pairs with the control dependency and rmb in try_to_wake_up().
 	 */
 	smp_store_release(&prev->on_cpu, 0);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260661 — [PATCH 3/4] sched: Fix a race in try_to_wake_up() vs schedule()

Oleg noticed that its possible to falsely observe p->on_cpu == 0 such
that we'll prematurely continue with the wakeup and effectively run p on
two CPUs at the same time.

Even though the overlap is very limited; the task is in the middle of
being scheduled out; it could still result in corruption of the
scheduler data structures.


        CPU0                            CPU1

        set_current_state(...)

        <preempt_schedule>
          context_switch(X, Y)
            prepare_lock_switch(Y)
              Y->on_cpu = 1;
            finish_lock_switch(X)
              store_release(X->on_cpu, 0);

                                        try_to_wake_up(X)
                                          LOCK(p->pi_lock);

                                          t = X->on_cpu; // 0

          context_switch(Y, X)
            prepare_lock_switch(X)
              X->on_cpu = 1;
            finish_lock_switch(Y)
              store_release(Y->on_cpu, 0);
        </preempt_schedule>

        schedule();
          deactivate_task(X);
          X->on_rq = 0;

                                          if (X->on_rq) // false

                                          if (t) while (X->on_cpu)
                                            cpu_relax();

          context_switch(X, ..)
            finish_lock_switch(X)
              store_release(X->on_cpu, 0);


Avoid the load of X->on_cpu being hoisted over the X->on_rq load.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/core.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2084,6 +2084,25 @@ try_to_wake_up(struct task_struct *p, un
 
 #ifdef CONFIG_SMP
 	/*
+	 * Ensure we load p->on_cpu _after_ p->on_rq, otherwise it would be
+	 * possible to, falsely, observe p->on_cpu == 0.
+	 *
+	 * One must be running (->on_cpu == 1) in order to remove oneself
+	 * from the runqueue.
+	 *
+	 *  [S] ->on_cpu = 1;	[L] ->on_rq
+	 *      UNLOCK rq->lock
+	 *			RMB
+	 *      LOCK   rq->lock
+	 *  [S] ->on_rq = 0;    [L] ->on_cpu
+	 *
+	 * Pairs with the full barrier implied in the UNLOCK+LOCK on rq->lock
+	 * from the consecutive calls to schedule(); the first switching to our
+	 * task, the second putting it to sleep.
+	 */
+	smp_rmb();
+
+	/*
 	 * If the owning (remote) cpu is still in the middle of schedule() with
 	 * this task as prev, wait until its done referencing the task.
 	 */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web