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 2 of 3 — ← Prev page 1 [2] 3  Next page →

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

On Tue, Nov 03, 2015 at 11:40:24AM -0800, Linus Torvalds wrote:
> On Mon, Nov 2, 2015 at 5:57 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:

[ . . . ]

> > I am in India and my Alpha Architecture Manual is in the USA.
> 
> I sent you a link to something that should work, and that has the section.

Thank you, and yes, it clearly states that read-to-write dependencies
are ordered.  Color me slow and stupid.

> > And they did post a clarification on the web:
> 
> So for alpha, you trust a random web posting by a unknown person that
> talks about some random problem in an application that we don't even
> know what it is.

In my defense, the Alpha architects pointed me at that web posting, but
yes, it appears to be pushing for overly conservative safety rather than
accuracy.  Please accept my apologies for my confusion.

							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]

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

On Tue, Nov 3, 2015 at 7:57 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> Thank you, and yes, it clearly states that read-to-write dependencies
> are ordered.

Well, I wouldn't say that it's exactly "clear".

The fact that they explicitly say "Note that the DP relation does not
directly impose a BEFORE (⇐) ordering between accesses u and v" makes
it all clear as mud.

They *then* go on to talk about how the DP relationship *together*
with the odd "is source of" ordering (which in turn is defined in
terms of BEFORE ordering) cannot have cycles.

I have no idea why they do it that way, but the reason seems to be
that they wanted to make "BEFORE" be purely about barriers and
accesses, and make the other orderings be described separately. So the
"BEFORE" ordering is used to define how memory must act, which is then
used as a basis for that storage definition and the "is source of"
thing.

But none of that seems to make much sense to a *user*.

The fact that they seem to equate "BEFORE" with "Processor Issue
Constraints" also makes me think that the whole logic was written by a
CPU designer, and part of why they document it that way is that the
CPU designer literally thought of "can I issue this access" as being
very different from "is there some inherent ordering that just results
from issues outside of my design".

I really don't know. That whole series of memory ordering rules makes
my head hurt.

But I do think the only thing that matters in the end is that they do
have that DP relationship between reads and subsequently dependent
writes, but  basically not for *any* other relationship.

So on alpha read-vs-read, write-vs-later-read, and write-vs-write all
have to have memory barriers, unless the accesses physically overlap.

                 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]

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

On Tue, Nov 03, 2015 at 08:43:22PM -0800, Linus Torvalds wrote:
> On Tue, Nov 3, 2015 at 7:57 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > Thank you, and yes, it clearly states that read-to-write dependencies
> > are ordered.
> 
> Well, I wouldn't say that it's exactly "clear".
> 
> The fact that they explicitly say "Note that the DP relation does not
> directly impose a BEFORE (⇐) ordering between accesses u and v" makes
> it all clear as mud.
> 
> They *then* go on to talk about how the DP relationship *together*
> with the odd "is source of" ordering (which in turn is defined in
> terms of BEFORE ordering) cannot have cycles.
> 
> I have no idea why they do it that way, but the reason seems to be
> that they wanted to make "BEFORE" be purely about barriers and
> accesses, and make the other orderings be described separately. So the
> "BEFORE" ordering is used to define how memory must act, which is then
> used as a basis for that storage definition and the "is source of"
> thing.
> 
> But none of that seems to make much sense to a *user*.
> 
> The fact that they seem to equate "BEFORE" with "Processor Issue
> Constraints" also makes me think that the whole logic was written by a
> CPU designer, and part of why they document it that way is that the
> CPU designer literally thought of "can I issue this access" as being
> very different from "is there some inherent ordering that just results
> from issues outside of my design".
> 
> I really don't know. That whole series of memory ordering rules makes
> my head hurt.

The honest answer is that for Alpha, I don't know either.  But if your
head isn't hurting enough yet, feel free to read on...



My guess, based loosely on ARM and PowerPC, is that memory barriers
provide global ordering but that the load-to-store dependency ordering is
strictly local.  Which as you say does not necessarily make much sense
to a user.  One guess is that the following would never trigger the
BUG_ON() given x, y, and z initially zero (and ignoring the possibility
of compiler mischief):

	CPU 0		CPU 1		CPU 2
	r1 = x;		r2 = y;		r3 = z;
	if (r1)		if (r2)		if (r3)
		y = 1;		z = 1;		x = 1;
	BUG_ON(r1 == 1 && r2 == 1 && r3 == 1); /* after the dust settles */

The write-to-read relationships prevent misordering.  The copy of the
Alpha manual I downloaded hints that this BUG_ON() could not fire.

However, the following might well trigger:

	CPU 0		CPU 1		CPU 2
	x = 1;		r1 = x;		r2 = y;
			if (r1)		if (r2)
				y = 1;		x = 2;
	BUG_ON(r1 == 1 && r2 == 1 && x == 1); /* after the dust settles */

The dependency ordering orders each CPU individually, but might not force
CPU 0's write to reach CPU 1 and CPU 2 at the same time.  So the BUG_ON()
case would happen if CPU 0's write to x reach CPU 1 before it reached
CPU 2, in which case the x==2 value might not be seen outside of CPU 2,
so that everyone agrees on the order of values taken on by x.

And this could be prevented by enforcing global (rather than local)
ordering by placing a memory barrier in CPU 1:

	CPU 0		CPU 1		CPU 2
	x = 1;		r1 = x;		r2 = y;
			smp_mb();	if (r2)
			if (r1)			x = 2;
				y = 1;
	BUG_ON(r1 == 1 && r2 == 1 && x == 1); /* after the dust settles */

But the reference manual doesn't have this sort of litmus test, so who
knows?  CCing the Alpha maintainers in case they know.

> But I do think the only thing that matters in the end is that they do
> have that DP relationship between reads and subsequently dependent
> writes, but  basically not for *any* other relationship.
> 
> So on alpha read-vs-read, write-vs-later-read, and write-vs-write all
> have to have memory barriers, unless the accesses physically overlap.

Agreed.

							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]

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

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > +       smp_read_barrier_depends(); /* ctrl */  \
> > +       smp_rmb(); /* ctrl + rmb := acquire */  \

Doesn't smp_rmb() imply an smp_read_barrier_depends() anyway?  In
memory-barriers.txt, it says:

     Read memory barriers imply data dependency barriers, and so can
     substitute for them.

David
--
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]

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

On Mon, Nov 02, 2015 at 08:36:49PM +0000, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > +       smp_read_barrier_depends(); /* ctrl */  \
> > > +       smp_rmb(); /* ctrl + rmb := acquire */  \
> 
> Doesn't smp_rmb() imply an smp_read_barrier_depends() anyway?  In
> memory-barriers.txt, it says:
> 
>      Read memory barriers imply data dependency barriers, and so can
>      substitute for them.

Yes, I noted that in a follow up email, Alpha implements both as MB. So
just the smp_rmb() is sufficient.

--
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]

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

On Mon, Nov 2, 2015 at 12:36 PM, David Howells <dhowells@redhat.com> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> > +       smp_read_barrier_depends(); /* ctrl */  \
>> > +       smp_rmb(); /* ctrl + rmb := acquire */  \
>
> Doesn't smp_rmb() imply an smp_read_barrier_depends() anyway?

Yes, it does. But that "smp_read_barrier_depends()" is actually
mis-used as a "barrier against subsequent dependent writes, thanks to
the control flow". It's not protecting against subsequent reads -
which is what the smp_rmb() is about.

Which is completely bogus, but that's what the comment implies.

Of course, on alpha (which is where smp_read_barrier_depends() makes a
difference), both that and smp_rmb() are just full memory barriers,
because alpha is some crazy sh*t. So yes, a "smp_rmb()" is sufficient
everywhere, but that is actually not where the confusion comes from in
the first place.

                   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]

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

On 11/02, Peter Zijlstra wrote:
>
> +#define smp_cond_acquire(cond)	do {		\
> +	while (!(cond))				\
> +		cpu_relax();			\
> +	smp_read_barrier_depends(); /* ctrl */	\
> +	smp_rmb(); /* ctrl + rmb := acquire */	\
> +} while (0)

...

> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -102,13 +102,13 @@ void task_work_run(void)
>
>  		if (!work)
>  			break;
> +
>  		/*
>  		 * Synchronize with task_work_cancel(). It can't remove
>  		 * the first entry == work, cmpxchg(task_works) should
>  		 * fail, but it can play with *work and other entries.
>  		 */
> -		raw_spin_unlock_wait(&task->pi_lock);
> -		smp_mb();
> +		smp_cond_acquire(!raw_spin_is_locked(&task->pi_lock));

Unfortunately this doesn't look exactly right...

spin_unlock_wait() is not equal to "while (locked) relax", the latter
is live-lockable or at least sub-optimal: we do not really need to spin
until we observe !spin_is_locked(), we only need to synchronize with the
current owner of this lock. Once it drops the lock we can proceed, we
do not care if another thread takes the same lock right after that.

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]

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

On Tue, Nov 03, 2015 at 06:59:58PM +0100, Oleg Nesterov wrote:

> > -		raw_spin_unlock_wait(&task->pi_lock);
> > -		smp_mb();
> > +		smp_cond_acquire(!raw_spin_is_locked(&task->pi_lock));
> 
> Unfortunately this doesn't look exactly right...
> 
> spin_unlock_wait() is not equal to "while (locked) relax", the latter
> is live-lockable or at least sub-optimal: we do not really need to spin
> until we observe !spin_is_locked(), we only need to synchronize with the
> current owner of this lock. Once it drops the lock we can proceed, we
> do not care if another thread takes the same lock right after that.

Ah indeed. And while every use of spin_unlock_wait() has 'interesting'
barriers associated, they all seem different.
--
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]

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

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

Hi Oleg,

On Tue, Nov 03, 2015 at 06:59:58PM +0100, Oleg Nesterov wrote:
[snip]
> 
> Unfortunately this doesn't look exactly right...
> 
> spin_unlock_wait() is not equal to "while (locked) relax", the latter
> is live-lockable or at least sub-optimal: we do not really need to spin

Just be curious, should spin_unlock_wait() semantically be an ACQUIRE?
Because spin_unlock_wait() is used for us to wait for a certain lock to
RELEASE so that we can do something *after* we observe the RELEASE.
Considering the follow example:

	CPU 0 				CPU 1
	============================	===========================
	{ X = 0 }
					WRITE_ONCE(X, 1);
					spin_unlock(&lock);
	spin_unlock_wait(&lock)
	r1 = READ_ONCE(X);

If spin_unlock_wait() is not an ACQUIRE, r1 can be 0 in this case,
right? Am I missing something subtle here? Or spin_unlock_wait() itself
doesn't have the ACQUIRE semantics, but it should always come with a
smp_mb() *following* it to achieve the ACQUIRE semantics? However in
do_exit(), an smp_mb() is preceding raw_spin_unlock_wait() rather than
following, which makes me confused... could you explain that? Thank you
;-)

Regards,
Boqun

> until we observe !spin_is_locked(), we only need to synchronize with the
> current owner of this lock. Once it drops the lock we can proceed, we
> do not care if another thread takes the same lock right after that.
> 
> Oleg.
> 

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

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

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

On Wed, Nov 11, 2015 at 05:39:40PM +0800, Boqun Feng wrote:
> Hi Oleg,
> 
> On Tue, Nov 03, 2015 at 06:59:58PM +0100, Oleg Nesterov wrote:
> [snip]
> > 
> > Unfortunately this doesn't look exactly right...
> > 
> > spin_unlock_wait() is not equal to "while (locked) relax", the latter
> > is live-lockable or at least sub-optimal: we do not really need to spin
> 
> Just be curious, should spin_unlock_wait() semantically be an ACQUIRE?

Hmm.. I guess I was wrong, it doesn't need to be an ACQUIRE, it needs
only to use the control dependency to order the load of lock state and
stores following it.

> Because spin_unlock_wait() is used for us to wait for a certain lock to
> RELEASE so that we can do something *after* we observe the RELEASE.
> Considering the follow example:
> 
> 	CPU 0 				CPU 1
> 	============================	===========================
> 	{ X = 0 }
> 					WRITE_ONCE(X, 1);
> 					spin_unlock(&lock);
> 	spin_unlock_wait(&lock)
> 	r1 = READ_ONCE(X);
> 
> If spin_unlock_wait() is not an ACQUIRE, r1 can be 0 in this case,
> right? Am I missing something subtle here? Or spin_unlock_wait() itself
> doesn't have the ACQUIRE semantics, but it should always come with a
> smp_mb() *following* it to achieve the ACQUIRE semantics? However in
> do_exit(), an smp_mb() is preceding raw_spin_unlock_wait() rather than
> following, which makes me confused... could you explain that? Thank you
> ;-)
> 

But still, there is one suspicious use of smp_mb() in do_exit():

	/*
	 * 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);

	/* causes final put_task_struct in finish_task_switch(). */
	tsk->state = TASK_DEAD;
	tsk->flags |= PF_NOFREEZE;	/* tell freezer to ignore us */
	schedule();

Seems like smp_mb() doesn't need here? Because the control dependency
already orders load of tsk->pi_lock and store of tsk->state, and this
control dependency order guarantee pairs with the spin_unlock(->pi_lock)
in try_to_wake_up() to avoid data race on ->state.

Regards,
Boqun

> Regards,
> Boqun
> 
> > until we observe !spin_is_locked(), we only need to synchronize with the
> > current owner of this lock. Once it drops the lock we can proceed, we
> > do not care if another thread takes the same lock right after that.
> > 
> > Oleg.
> > 

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

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

He Boqun,

Let me first state that I can't answer authoritatively when it comes to
barriers. That said,

On 11/11, Boqun Feng wrote:
>
> But still, there is one suspicious use of smp_mb() in do_exit():
>
> 	/*
> 	 * 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);
>
> 	/* causes final put_task_struct in finish_task_switch(). */
> 	tsk->state = TASK_DEAD;
> 	tsk->flags |= PF_NOFREEZE;	/* tell freezer to ignore us */
> 	schedule();
>
> Seems like smp_mb() doesn't need here?

Please see my reply to peterz's email.

AFAICS, we need the barries on both sides. But, since we only need to
STORE into tsk->state after unlock_wait(), we can rely on the control
dependency and avoid the 2nd mb().

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]

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

On Wed, Nov 11, 2015 at 06:34:56PM +0800, Boqun Feng wrote:
> On Wed, Nov 11, 2015 at 05:39:40PM +0800, Boqun Feng wrote:
> > Hi Oleg,
> > 
> > On Tue, Nov 03, 2015 at 06:59:58PM +0100, Oleg Nesterov wrote:
> > [snip]
> > > 
> > > Unfortunately this doesn't look exactly right...
> > > 
> > > spin_unlock_wait() is not equal to "while (locked) relax", the latter
> > > is live-lockable or at least sub-optimal: we do not really need to spin
> > 
> > Just be curious, should spin_unlock_wait() semantically be an ACQUIRE?
> 
> Hmm.. I guess I was wrong, it doesn't need to be an ACQUIRE, it needs
> only to use the control dependency to order the load of lock state and
> stores following it.

I must say that spin_unlock_wait() and friends are a bit tricky.
There are only a few uses in drivers/ata/libata-eh.c, ipc/sem.c,
kernel/exit.c, kernel/sched/completion.c, and kernel/task_work.c.
Almost all of these seem to assume that spin_unlock_wait() provides
no ordering.  We might have just barely enough uses to produce useful
abstractions, but my guess is that it would not hurt to wait.

> > Because spin_unlock_wait() is used for us to wait for a certain lock to
> > RELEASE so that we can do something *after* we observe the RELEASE.
> > Considering the follow example:
> > 
> > 	CPU 0 				CPU 1
> > 	============================	===========================
> > 	{ X = 0 }
> > 					WRITE_ONCE(X, 1);
> > 					spin_unlock(&lock);
> > 	spin_unlock_wait(&lock)
> > 	r1 = READ_ONCE(X);
> > 
> > If spin_unlock_wait() is not an ACQUIRE, r1 can be 0 in this case,
> > right? Am I missing something subtle here? Or spin_unlock_wait() itself
> > doesn't have the ACQUIRE semantics, but it should always come with a
> > smp_mb() *following* it to achieve the ACQUIRE semantics? However in
> > do_exit(), an smp_mb() is preceding raw_spin_unlock_wait() rather than
> > following, which makes me confused... could you explain that? Thank you
> > ;-)
> > 
> 
> But still, there is one suspicious use of smp_mb() in do_exit():
> 
> 	/*
> 	 * 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);
> 
> 	/* causes final put_task_struct in finish_task_switch(). */
> 	tsk->state = TASK_DEAD;
> 	tsk->flags |= PF_NOFREEZE;	/* tell freezer to ignore us */
> 	schedule();
> 
> Seems like smp_mb() doesn't need here? Because the control dependency
> already orders load of tsk->pi_lock and store of tsk->state, and this
> control dependency order guarantee pairs with the spin_unlock(->pi_lock)
> in try_to_wake_up() to avoid data race on ->state.

The exit() path is pretty heavyweight, so I suspect that an extra smp_mb()
is down in the noise.  Or are you saying that this is somehow unsafe?

							Thanx, Paul

> Regards,
> Boqun
> 
> > Regards,
> > Boqun
> > 
> > > until we observe !spin_is_locked(), we only need to synchronize with the
> > > current owner of this lock. Once it drops the lock we can proceed, we
> > > do not care if another thread takes the same lock right after that.
> > > 
> > > 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]

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

On Wed, Nov 11, 2015 at 05:39:40PM +0800, Boqun Feng wrote:

> Just be curious, should spin_unlock_wait() semantically be an ACQUIRE?

I did wonder the same thing, it would simplify a number of things if
this were so.
--
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]

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

On 11/11, Peter Zijlstra wrote:
>
> On Wed, Nov 11, 2015 at 05:39:40PM +0800, Boqun Feng wrote:
>
> > Just be curious, should spin_unlock_wait() semantically be an ACQUIRE?
>
> I did wonder the same thing, it would simplify a number of things if
> this were so.

Yes, me too.

Sometimes I even think it should have both ACQUIRE + RELEASE semantics.
IOW, it should be "equivalent" to spin_lock() + spin_unlock().

Consider this code:

	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.

If it is wrong, then task_work_run() is buggy: it relies on mb() implied by
cmpxchg() before spin_unlock_wait() the same way: the next task_work_cancel()
should see the result of our cmpxchg(), it must not try to read work->next or
work->func.

Hmm. Not sure I really understand what I am trying to say... Perhaps in fact
I mean that unlock_wait() should be removed because it is too subtle for me ;)

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]

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

On Wed, Nov 11, 2015 at 11:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 11/11, Peter Zijlstra wrote:
>
> Sometimes I even think it should have both ACQUIRE + RELEASE semantics.
> IOW, it should be "equivalent" to spin_lock() + spin_unlock().

That's insane.

"Release" semantics are - by definition - about stuff that *predeces* it.

It is simply not sensible to have a "wait_for_unlock()" that then
synchronizes loads or stores that happened *before* the wait. That's
some crazy voodoo programming.

And if you do need to have model where you do "store something, then
make sure that the unlock we wait for happens after the store", then
you need to just add a "smp_mb()" in between that store and the
waiting for unlock.

Or just take the damn lock, and don't play any games at all.

                   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]

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

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

On Wed, Nov 11, 2015 at 08:39:53PM +0100, Oleg Nesterov wrote:
> On 11/11, Peter Zijlstra wrote:
> >
> > On Wed, Nov 11, 2015 at 05:39:40PM +0800, Boqun Feng wrote:
> >
> > > Just be curious, should spin_unlock_wait() semantically be an ACQUIRE?
> >
> > I did wonder the same thing, it would simplify a number of things if
> > this were so.
> 
> Yes, me too.
> 
> Sometimes I even think it should have both ACQUIRE + RELEASE semantics.
> IOW, it should be "equivalent" to spin_lock() + spin_unlock().
> 
> Consider this code:
> 
> 	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)

Therefore the case below can happen:

	CPU 1			CPU 2			CPU 3
	==================	====================	==============
							spin_unlock(&lock);
				spin_lock(&lock):
				  r1 = *lock; // r1 == 0;
				o = READ_ONCE(object); // reordered here
	object = NULL;
	smp_mb();
	spin_unlock_wait(&lock);
				  *lock = 1;
	smp_mb();		
	o->dead = true;
				if (o) // true
				  BUG_ON(o->dead); // true!!


To show this, I also translate this situation into a PPC litmus for
herd[1]:

	PPC spin-lock-wait
	"
	r1: local variable of lock
	r2: constant 1
	r3: constant 0 or NULL
	r4: local variable of object, i.e. o
	r5: local variable of *o (simulate ->dead as I didn't know 
	    how to write fields of structure in herd ;-()
	r13: the address of lock, i.e. &lock
	r14: the address of object, i.e. &object
	"
	{
	0:r1=0;0:r2=1;0:r3=0;0:r13=lock;0:r14=object;
	1:r1=0;1:r2=1;1:r3=0;1:r4=0;1:r5=0;1:r13=lock;1:r14=object;
	2:r1=0;2:r13=lock;
	lock=1; object=old; old=0;
	}

	P0              | P1                 | P2 ;
	ld r4,0(r14)    | Lock:              | stw r1,0(r13);
	std r3,0(r14)   | lwarx r1,r3,r13    | ;
	                | cmpwi r1,0         | ;
	sync            | bne Lock           | ;
	                | stwcx. r2,r3,r13   | ;
	Wait:           | bne Lock           | ;
	lwz r1,0(r13)   | lwsync             | ;
	cmpwi r1,0      | ld r4,0(r14)       | ;
	bne Wait        | cmpwi r4,0         | ;
	                | beq Fail           | ;
	sync            | lwz r5, 0(r4)      | ;
	stw r2,0(r4)    | Fail:              | ;
	                | lwsync             | ;
	                | stw r3, 0(r13)     | ;

	exists
	(1:r4=old /\ 1:r5=1)

,whose result says that (1:r4=old /\ 1:r5=1) can happen:

	Test spin-lock-wait Allowed
	States 3
	1:r4=0; 1:r5=0;
	1:r4=old; 1:r5=0;
	1:r4=old; 1:r5=1;
	Loop Ok
	Witnesses
	Positive: 18 Negative: 108
	Condition exists (1:r4=old /\ 1:r5=1)
	Observation spin-lock-wait Sometimes 18 108
	Hash=244f8c0f91df5a5ed985500ed7230272

Please note that I use backwards jump in this litmus, which is only
supported by herd(not by ppcmem[2]), and it will take a while to get the
result. And I'm not that confident that I'm familiar with this tool,
maybe Paul and Will can help check my translate and usage here ;-)


IIUC, the problem here is that spin_lock_wait() can be implemented with
only LOAD operations, and to have a RELEASE semantics, one primivite
must have a *STORE* part, therefore spin_lock_wait() can not be a
RELEASE.

So.. we probably need to take the lock here.

> If it is wrong, then task_work_run() is buggy: it relies on mb() implied by
> cmpxchg() before spin_unlock_wait() the same way: the next task_work_cancel()
> should see the result of our cmpxchg(), it must not try to read work->next or
> work->func.
> 
> Hmm. Not sure I really understand what I am trying to say... Perhaps in fact
> I mean that unlock_wait() should be removed because it is too subtle for me ;)
> 

;-)

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);

	/* causes final put_task_struct in finish_task_switch(). */
	tsk->state = TASK_DEAD;

Ref:
[1]: https://github.com/herd/herdtools
[2]: http://www.cl.cam.ac.uk/~pes20/ppcmem/index.html

Regards,
Boqun

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

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

On Thu, Nov 12, 2015 at 03:14:51PM +0800, Boqun Feng 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
> 
> This could happen because of the ACQUIRE semantics of spin_lock(), and 
> the current implementation of spin_lock() on PPC allows this happen.

Urgh, you just _had_ to send an email like this, didn't you ;-)

I think AARGH64 does the same thing. They either use LDAXR/STXR, which
also places the ACQUIRE on the load, or use LDADDA (v8.1) which I
suspect does the same, but I'm not entirely sure how to decode these 8.1
atomics yet.

Let me think a little more on this..


--
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]

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

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.

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().

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?

And smp_mb__before_spinlock() looks wrong too 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]

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

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

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.

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.

Regards,
Boqun

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

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

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.

							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]

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

Back to top | Article view | linux.kernel

csiph-web