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

Groups > linux.kernel > #1248849 > unrolled thread

Q: schedule() and implied barriers on arm64

Back to article view | Back to linux.kernel

Contents

  Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-16 17:20 +0200
    Re: Q: schedule() and implied barriers on arm64 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-16 18:10 +0200
      Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-16 18:20 +0200
        Re: Q: schedule() and implied barriers on arm64 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-16 18:40 +0200
          Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-16 18:40 +0200
        Re: Q: schedule() and implied barriers on arm64 Catalin Marinas <catalin.marinas@arm.com> - 2015-10-16 19:00 +0200
          Re: Q: schedule() and implied barriers on arm64 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-16 19:30 +0200
            Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-16 21:10 +0200
              Re: Q: schedule() and implied barriers on arm64 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-10-16 21:30 +0200
            Re: Q: schedule() and implied barriers on arm64 Catalin Marinas <catalin.marinas@arm.com> - 2015-10-19 17:20 +0200
          Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-16 21:10 +0200
            Re: Q: schedule() and implied barriers on arm64 Ingo Molnar <mingo@kernel.org> - 2015-10-19 09:10 +0200
              Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-19 11:10 +0200
              Re: Q: schedule() and implied barriers on arm64 Catalin Marinas <catalin.marinas@arm.com> - 2015-10-19 17:30 +0200
                Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-19 18:30 +0200
                  Re: Q: schedule() and implied barriers on arm64 Ingo Molnar <mingo@kernel.org> - 2015-10-20 10:40 +0200
                  Re: Q: schedule() and implied barriers on arm64 Will Deacon <will.deacon@arm.com> - 2015-10-27 17:30 +0100
                    Re: Q: schedule() and implied barriers on arm64 Peter Zijlstra <peterz@infradead.org> - 2015-10-28 01:20 +0100
                      Re: Q: schedule() and implied barriers on arm64 Will Deacon <will.deacon@arm.com> - 2015-10-28 11:50 +0100
      Re: Q: schedule() and implied barriers on arm64 Catalin Marinas <catalin.marinas@arm.com> - 2015-10-16 19:20 +0200

#1248849 — Q: schedule() and implied barriers on arm64

Hi,

IIRC Paul relies on schedule() implying a full memory barrier with
strong transitivity for RCU.

If not, ignore this email.

If so, however, I suspect AARGH64 is borken and would need (just like
PPC):

#define smp_mb__before_spinlock()	smp_mb()

The problem is that schedule() (when a NO-OP) does:

	smp_mb__before_spinlock();
	LOCK rq->lock

	clear_bit()

	UNLOCK rq->lock

And nothing there implies a full barrier on AARGH64, since
smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
anything.

Pretty much every other arch has LOCK implying a full barrier, either
because its strongly ordered or because it needs one for the ACQUIRE
semantics.


--
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] | [next] | [standalone]

#1248944

On Fri, Oct 16, 2015 at 05:18:30PM +0200, Peter Zijlstra wrote:
> Hi,
> 
> IIRC Paul relies on schedule() implying a full memory barrier with
> strong transitivity for RCU.
> 
> If not, ignore this email.

Not so sure about schedule(), but definitely need strong transitivity
for the rcu_node structure's ->lock field.  And the atomic operations
on the rcu_dyntick structure's fields when entering or leaving the
idle loop.

With schedule, the thread later reports the quiescent state, which
involves acquiring the rcu_node structure's ->lock field.  So I -think-
that the locks in the scheduler can be weakly transitive.

> If so, however, I suspect AARGH64 is borken and would need (just like
> PPC):
> 
> #define smp_mb__before_spinlock()	smp_mb()
> 
> The problem is that schedule() (when a NO-OP) does:
> 
> 	smp_mb__before_spinlock();
> 	LOCK rq->lock
> 
> 	clear_bit()
> 
> 	UNLOCK rq->lock
> 
> And nothing there implies a full barrier on AARGH64, since
> smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
> load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
> anything.
> 
> Pretty much every other arch has LOCK implying a full barrier, either
> because its strongly ordered or because it needs one for the ACQUIRE
> semantics.

Well, arm64 might well need smp_mb__after_unlock_lock() to be non-empty.
But I thought that it used a dmb in the spinlock code somewhere or
another...

							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]

#1248950

On Fri, Oct 16, 2015 at 09:04:22AM -0700, Paul E. McKenney wrote:
> On Fri, Oct 16, 2015 at 05:18:30PM +0200, Peter Zijlstra wrote:
> > Hi,
> > 
> > IIRC Paul relies on schedule() implying a full memory barrier with
> > strong transitivity for RCU.
> > 
> > If not, ignore this email.
> 
> Not so sure about schedule(), but definitely need strong transitivity
> for the rcu_node structure's ->lock field.  And the atomic operations
> on the rcu_dyntick structure's fields when entering or leaving the
> idle loop.
> 
> With schedule, the thread later reports the quiescent state, which
> involves acquiring the rcu_node structure's ->lock field.  So I -think-
> that the locks in the scheduler can be weakly transitive.

So I _thought_ you needed this to separate the preempt_disabled
sections. Such that rcu_note_context_switch() is guaranteed to be done
before a new preempt_disabled region starts.

But if you really only need program order guarantees for that, and deal
with everything else from your tick, then that's fine too.

Maybe some previous RCU variant relied on this?

> > If so, however, I suspect AARGH64 is borken and would need (just like
> > PPC):
> > 
> > #define smp_mb__before_spinlock()	smp_mb()
> > 
> > The problem is that schedule() (when a NO-OP) does:
> > 
> > 	smp_mb__before_spinlock();
> > 	LOCK rq->lock
> > 
> > 	clear_bit()
> > 
> > 	UNLOCK rq->lock
> > 
> > And nothing there implies a full barrier on AARGH64, since
> > smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
> > load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
> > anything.
> > 
> > Pretty much every other arch has LOCK implying a full barrier, either
> > because its strongly ordered or because it needs one for the ACQUIRE
> > semantics.
> 
> But I thought that it used a dmb in the spinlock code somewhere or
> another...

arm does, arm64 not so much.

> Well, arm64 might well need smp_mb__after_unlock_lock() to be non-empty.

Its UNLOCK+LOCK should be RCsc, so that should be good. Its just that
LOCK+UNLOCK isn't anything.
--
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]

#1248964

On Fri, Oct 16, 2015 at 06:16:08PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 16, 2015 at 09:04:22AM -0700, Paul E. McKenney wrote:
> > On Fri, Oct 16, 2015 at 05:18:30PM +0200, Peter Zijlstra wrote:
> > > Hi,
> > > 
> > > IIRC Paul relies on schedule() implying a full memory barrier with
> > > strong transitivity for RCU.
> > > 
> > > If not, ignore this email.
> > 
> > Not so sure about schedule(), but definitely need strong transitivity
> > for the rcu_node structure's ->lock field.  And the atomic operations
> > on the rcu_dyntick structure's fields when entering or leaving the
> > idle loop.
> > 
> > With schedule, the thread later reports the quiescent state, which
> > involves acquiring the rcu_node structure's ->lock field.  So I -think-
> > that the locks in the scheduler can be weakly transitive.
> 
> So I _thought_ you needed this to separate the preempt_disabled
> sections. Such that rcu_note_context_switch() is guaranteed to be done
> before a new preempt_disabled region starts.
> 
> But if you really only need program order guarantees for that, and deal
> with everything else from your tick, then that's fine too.
> 
> Maybe some previous RCU variant relied on this?

Yes, older versions did rely on this.  Now, only the CPU itself observes
RCU's state changes during context switch.  I couldn't tell you exactly
when this changed.  :-/

With the exception of some synchronize_sched_expedited() cases, but in
those cases, RCU code acquires the CPU's leaf rcu_node structure's
->lock, and with the required strong transitivity.

> > > If so, however, I suspect AARGH64 is borken and would need (just like
> > > PPC):
> > > 
> > > #define smp_mb__before_spinlock()	smp_mb()
> > > 
> > > The problem is that schedule() (when a NO-OP) does:
> > > 
> > > 	smp_mb__before_spinlock();
> > > 	LOCK rq->lock
> > > 
> > > 	clear_bit()
> > > 
> > > 	UNLOCK rq->lock
> > > 
> > > And nothing there implies a full barrier on AARGH64, since
> > > smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
> > > load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
> > > anything.
> > > 
> > > Pretty much every other arch has LOCK implying a full barrier, either
> > > because its strongly ordered or because it needs one for the ACQUIRE
> > > semantics.
> > 
> > But I thought that it used a dmb in the spinlock code somewhere or
> > another...
> 
> arm does, arm64 not so much.
> 
> > Well, arm64 might well need smp_mb__after_unlock_lock() to be non-empty.
> 
> Its UNLOCK+LOCK should be RCsc, so that should be good. Its just that
> LOCK+UNLOCK isn't anything.

Ah!  If RCU relies on LOCK+UNLOCK being a barrier of any sort, that is a
bug in RCU that needs fixing.

							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]

#1248969

On Fri, Oct 16, 2015 at 09:28:24AM -0700, Paul E. McKenney wrote:

> > Maybe some previous RCU variant relied on this?
> 
> Yes, older versions did rely on this.  Now, only the CPU itself observes
> RCU's state changes during context switch.  I couldn't tell you exactly
> when this changed.  :-/
> 
> With the exception of some synchronize_sched_expedited() cases, but in
> those cases, RCU code acquires the CPU's leaf rcu_node structure's
> ->lock, and with the required strong transitivity.

OK, so I can scrap this 'requirement' from my list. All sorted, thanks!

> > > Well, arm64 might well need smp_mb__after_unlock_lock() to be non-empty.
> > 
> > Its UNLOCK+LOCK should be RCsc, so that should be good. Its just that
> > LOCK+UNLOCK isn't anything.
> 
> Ah!  If RCU relies on LOCK+UNLOCK being a barrier of any sort, that is a
> bug in RCU that needs fixing.

Don't think RCU does that, But its what schedule() provides in the
weakest case. Hence my question here.
--
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]

#1248984

I'll try to reply in Will's absence, though I gave up trying to
understand these threads long time ago ;).

On 16 October 2015 at 17:16, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Oct 16, 2015 at 09:04:22AM -0700, Paul E. McKenney wrote:
>> On Fri, Oct 16, 2015 at 05:18:30PM +0200, Peter Zijlstra wrote:
>> > If so, however, I suspect AARGH64 is borken and would need (just like
>> > PPC):
>> >
>> > #define smp_mb__before_spinlock()   smp_mb()
>> >
>> > The problem is that schedule() (when a NO-OP) does:
>> >
>> >     smp_mb__before_spinlock();
>> >     LOCK rq->lock
>> >
>> >     clear_bit()
>> >
>> >     UNLOCK rq->lock
>> >
>> > And nothing there implies a full barrier on AARGH64, since
>> > smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
>> > load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
>> > anything.
>> >
>> > Pretty much every other arch has LOCK implying a full barrier, either
>> > because its strongly ordered or because it needs one for the ACQUIRE
>> > semantics.
>>
>> But I thought that it used a dmb in the spinlock code somewhere or
>> another...
>
> arm does, arm64 not so much.

arm64 indeed does not have a dmb after spin_lock, it only has a
load-acquire. So with the default smp_mb__before_spinlock() +
spin_lock we have:

  smp_wmb()
  loop
    load-acquire
    store

So (I think) this guarantees that any writes before wmb+lock would be
visible before any reads _and_ writes after wmb+lock. However, the
ordering with reads before wmb+lock is not guaranteed.

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

#1249015

On Fri, Oct 16, 2015 at 05:55:35PM +0100, Catalin Marinas wrote:
> I'll try to reply in Will's absence, though I gave up trying to
> understand these threads long time ago ;).
> 
> On 16 October 2015 at 17:16, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, Oct 16, 2015 at 09:04:22AM -0700, Paul E. McKenney wrote:
> >> On Fri, Oct 16, 2015 at 05:18:30PM +0200, Peter Zijlstra wrote:
> >> > If so, however, I suspect AARGH64 is borken and would need (just like
> >> > PPC):
> >> >
> >> > #define smp_mb__before_spinlock()   smp_mb()
> >> >
> >> > The problem is that schedule() (when a NO-OP) does:
> >> >
> >> >     smp_mb__before_spinlock();
> >> >     LOCK rq->lock
> >> >
> >> >     clear_bit()
> >> >
> >> >     UNLOCK rq->lock
> >> >
> >> > And nothing there implies a full barrier on AARGH64, since
> >> > smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
> >> > load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
> >> > anything.
> >> >
> >> > Pretty much every other arch has LOCK implying a full barrier, either
> >> > because its strongly ordered or because it needs one for the ACQUIRE
> >> > semantics.
> >>
> >> But I thought that it used a dmb in the spinlock code somewhere or
> >> another...
> >
> > arm does, arm64 not so much.
> 
> arm64 indeed does not have a dmb after spin_lock, it only has a
> load-acquire. So with the default smp_mb__before_spinlock() +
> spin_lock we have:
> 
>   smp_wmb()
>   loop
>     load-acquire
>     store
> 
> So (I think) this guarantees that any writes before wmb+lock would be
> visible before any reads _and_ writes after wmb+lock. However, the
> ordering with reads before wmb+lock is not guaranteed.

So RCU needs the following sort of guarantee:

	void task1(unsigned long flags)
	{
		WRITE_ONCE(x, 1);
		WRITE_ONCE(z, 1);
		raw_spin_unlock_irqrestore(&rnp->lock, flags);
	}

	void task2(unsigned long *flags)
	{
		raw_spin_lock_irqsave(&rnp->lock, *flags);
		smp_mb__after_unlock_lock();
		r1 = READ_ONCE(y);
		r2 = READ_ONCE(z);
	}

	void task3(void)
	{
		WRITE_ONCE(y, 1);
		smp_mb();
		r3 = READ_ONCE(x);
	}

	BUG_ON(!r1 && r2 && !r3); /* After the dust settles. */

In other words, if task2() acquires the lock after task1() releases it,
all CPUs must agree on the order of the operations in the two critical
sections, even if these other CPUs don't acquire the lock.

This same guarantee is needed if task1() and then task2() run in
succession on the same CPU with no additional synchronization of any sort.

Does this work on arm64?

							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]

#1249083

On Fri, Oct 16, 2015 at 10:28:11AM -0700, Paul E. McKenney wrote:
> In other words, if task2() acquires the lock after task1() releases it,
> all CPUs must agree on the order of the operations in the two critical
> sections, even if these other CPUs don't acquire the lock.
> 
> This same guarantee is needed if task1() and then task2() run in
> succession on the same CPU with no additional synchronization of any sort.
> 
> Does this work on arm64?

Yes, their load-acquire and store-release are 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]

#1249103

On Fri, Oct 16, 2015 at 09:07:41PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 16, 2015 at 10:28:11AM -0700, Paul E. McKenney wrote:
> > In other words, if task2() acquires the lock after task1() releases it,
> > all CPUs must agree on the order of the operations in the two critical
> > sections, even if these other CPUs don't acquire the lock.
> > 
> > This same guarantee is needed if task1() and then task2() run in
> > succession on the same CPU with no additional synchronization of any sort.
> > 
> > Does this work on arm64?
> 
> Yes, their load-acquire and store-release are RCsc.

Whew!!!

							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]

#1250820

On Fri, Oct 16, 2015 at 10:28:11AM -0700, Paul E. McKenney wrote:
> So RCU needs the following sort of guarantee:
> 
> 	void task1(unsigned long flags)
> 	{
> 		WRITE_ONCE(x, 1);
> 		WRITE_ONCE(z, 1);
> 		raw_spin_unlock_irqrestore(&rnp->lock, flags);
> 	}
> 
> 	void task2(unsigned long *flags)
> 	{
> 		raw_spin_lock_irqsave(&rnp->lock, *flags);
> 		smp_mb__after_unlock_lock();
> 		r1 = READ_ONCE(y);
> 		r2 = READ_ONCE(z);
> 	}
> 
> 	void task3(void)
> 	{
> 		WRITE_ONCE(y, 1);
> 		smp_mb();
> 		r3 = READ_ONCE(x);
> 	}
> 
> 	BUG_ON(!r1 && r2 && !r3); /* After the dust settles. */
> 
> In other words, if task2() acquires the lock after task1() releases it,
> all CPUs must agree on the order of the operations in the two critical
> sections, even if these other CPUs don't acquire the lock.
> 
> This same guarantee is needed if task1() and then task2() run in
> succession on the same CPU with no additional synchronization of any sort.
> 
> Does this work on arm64?

I think it does. If r3 == 0, it means that READ_ONCE(x) in task3 is
"observed" (in ARM ARM terms) by task1 before WRITE_ONCE(x, 1). The
smp_mb() in task3 implies that WRITEONCE(y, 1) is also observed by
task1.

A store-release is multi-copy atomic when "observed" with a load-acquire
(from task2). When on the same CPU, they are always observed in program
order. The store-release on ARM has the property that writes observed by
task1 before store-release (that is WRITE_ONCE(y, 1) in task3) will be
observed by other observers (task2) before the store-release is observed
(the unlock).

The above rules guarantee that, when r3 == 0, WRITE_ONCE(y, 1) in task3
is observed by task2 (and task1), hence r1 == 1.

(a more formal proof would have to wait for Will to come back from
holiday ;))

-- 
Catalin

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

#1249088

On Fri, Oct 16, 2015 at 05:55:35PM +0100, Catalin Marinas wrote:
> arm64 indeed does not have a dmb after spin_lock, it only has a
> load-acquire. So with the default smp_mb__before_spinlock() +
> spin_lock we have:
> 
>   smp_wmb()
>   loop
>     load-acquire
>     store
> 
> So (I think) this guarantees that any writes before wmb+lock would be
> visible before any reads _and_ writes after wmb+lock. However, the
> ordering with reads before wmb+lock is not guaranteed.

That is my understanding as well, and stores could creep up from below
the unlock and then the reads and those stores can cross and you've
lost.

In any case, its all moot now, since Paul no longer requires schedule()
to imply a full barrier.
--
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]

#1250394

* Peter Zijlstra <peterz@infradead.org> wrote:

> In any case, its all moot now, since Paul no longer requires schedule() to imply 
> a full barrier.
>
> [...]

Nevertheless from a least-surprise POV it might be worth guaranteeing it, because 
I bet there's tons of code that assumes that schedule() is a heavy operation and 
it's such an easy mistake to make. Since we are so close to having that guarantee, 
we might as well codify it?

Just like system calls are assumed to be barriers in general - and system calls 
are more lightweight than schedule() ...

Thanks,

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

#1250485

On Mon, Oct 19, 2015 at 09:06:05AM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > In any case, its all moot now, since Paul no longer requires schedule() to imply 
> > a full barrier.
> >
> > [...]
> 
> Nevertheless from a least-surprise POV it might be worth guaranteeing it, because 
> I bet there's tons of code that assumes that schedule() is a heavy operation and 
> it's such an easy mistake to make. Since we are so close to having that guarantee, 
> we might as well codify it?
> 
> Just like system calls are assumed to be barriers in general - 

Are they? I know they are on some platforms, but I'm not sure we've
audited them all and established this.

> and system calls are more lightweight than schedule() ...

Hopefully, although if you enable nohz_full there's a fair chance to
reverse that :-)
--
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]

#1250839

On Mon, Oct 19, 2015 at 09:06:05AM +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > In any case, its all moot now, since Paul no longer requires schedule() to imply 
> > a full barrier.
> >
> > [...]
> 
> Nevertheless from a least-surprise POV it might be worth guaranteeing it, because 
> I bet there's tons of code that assumes that schedule() is a heavy operation and 
> it's such an easy mistake to make. Since we are so close to having that guarantee, 
> we might as well codify it?

FWIW, the arm64 __switch_to() has a heavy barrier (DSB) but the reason
for this was to cope with potentially interrupted cache or TLB
maintenance (which require a DSB on the same CPU) and thread migration
to another CPU.

-- 
Catalin

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

#1250896

On Mon, Oct 19, 2015 at 04:21:08PM +0100, Catalin Marinas wrote:
> On Mon, Oct 19, 2015 at 09:06:05AM +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > In any case, its all moot now, since Paul no longer requires schedule() to imply 
> > > a full barrier.
> > >
> > > [...]
> > 
> > Nevertheless from a least-surprise POV it might be worth guaranteeing it, because 
> > I bet there's tons of code that assumes that schedule() is a heavy operation and 
> > it's such an easy mistake to make. Since we are so close to having that guarantee, 
> > we might as well codify it?
> 
> FWIW, the arm64 __switch_to() has a heavy barrier (DSB) but the reason
> for this was to cope with potentially interrupted cache or TLB
> maintenance (which require a DSB on the same CPU) and thread migration
> to another CPU.

Right, but there's a path through schedule() that does not pass through
__switch_to(); when we pick the current task as the most eligible task
and next == prev.

In that case there really only is the wmb, a spin lock, an atomic op and
a spin unlock (and a whole bunch of 'normal' code of course).
--
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]

#1251438

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Oct 19, 2015 at 04:21:08PM +0100, Catalin Marinas wrote:
> > On Mon, Oct 19, 2015 at 09:06:05AM +0200, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > In any case, its all moot now, since Paul no longer requires schedule() to imply 
> > > > a full barrier.
> > > >
> > > > [...]
> > > 
> > > Nevertheless from a least-surprise POV it might be worth guaranteeing it, 
> > > because I bet there's tons of code that assumes that schedule() is a heavy 
> > > operation and it's such an easy mistake to make. Since we are so close to 
> > > having that guarantee, we might as well codify it?
> > 
> > FWIW, the arm64 __switch_to() has a heavy barrier (DSB) but the reason for 
> > this was to cope with potentially interrupted cache or TLB maintenance (which 
> > require a DSB on the same CPU) and thread migration to another CPU.
> 
> Right, but there's a path through schedule() that does not pass through 
> __switch_to(); when we pick the current task as the most eligible task and next 
> == prev.
> 
> In that case there really only is the wmb, a spin lock, an atomic op and a spin 
> unlock (and a whole bunch of 'normal' code of course).

Yeah, so my concern is that this is a rare race that might be 'surprising' for 
developers relying on various schedule() constructs. Especially as it's a full 
barrier on x86 (the most prominent SMP platform at the moment) there's a real 
danger of hard to debug bugs creeping to other architectures.

So I think we should just do the small step of making it a full barrier everywhere 
- it's very close to it in any case, and it shouldn't really matter for 
performance. Agreed?


Thanks,

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

#1256913

On Mon, Oct 19, 2015 at 06:24:23PM +0200, Peter Zijlstra wrote:
> On Mon, Oct 19, 2015 at 04:21:08PM +0100, Catalin Marinas wrote:
> > On Mon, Oct 19, 2015 at 09:06:05AM +0200, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > In any case, its all moot now, since Paul no longer requires schedule() to imply 
> > > > a full barrier.
> > > >
> > > > [...]
> > > 
> > > Nevertheless from a least-surprise POV it might be worth guaranteeing it, because 
> > > I bet there's tons of code that assumes that schedule() is a heavy operation and 
> > > it's such an easy mistake to make. Since we are so close to having that guarantee, 
> > > we might as well codify it?
> > 
> > FWIW, the arm64 __switch_to() has a heavy barrier (DSB) but the reason
> > for this was to cope with potentially interrupted cache or TLB
> > maintenance (which require a DSB on the same CPU) and thread migration
> > to another CPU.
> 
> Right, but there's a path through schedule() that does not pass through
> __switch_to(); when we pick the current task as the most eligible task
> and next == prev.
> 
> In that case there really only is the wmb, a spin lock, an atomic op and
> a spin unlock (and a whole bunch of 'normal' code of course).

... and the 'normal' code will have a control hazard somewhere, followed
by the implicit ISB in exception return, so there's a barrier of sorts
there too.

The problem is that people say "full barrier" without defining what it
really means, and we end up going round the houses on things like
transitivity (which ctrl + isb doesn't always give you).

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]

#1257493

On Tue, Oct 27, 2015 at 04:19:48PM +0000, Will Deacon wrote:
> ... and the 'normal' code will have a control hazard somewhere, followed
> by the implicit ISB in exception return, so there's a barrier of sorts
> there too.

Which exception return?

> The problem is that people say "full barrier" without defining what it
> really means, and we end up going round the houses on things like
> transitivity (which ctrl + isb doesn't always give you).

I pretty much meant smp_mb() here :-)
--
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]

#1257894

On Tue, Oct 27, 2015 at 07:40:20PM +0100, Peter Zijlstra wrote:
> On Tue, Oct 27, 2015 at 04:19:48PM +0000, Will Deacon wrote:
> > ... and the 'normal' code will have a control hazard somewhere, followed
> > by the implicit ISB in exception return, so there's a barrier of sorts
> > there too.
> 
> Which exception return?

The return to userspace after the interrupt/fault/system call that got us
into the kernel.

> > The problem is that people say "full barrier" without defining what it
> > really means, and we end up going round the houses on things like
> > transitivity (which ctrl + isb doesn't always give you).
> 
> I pretty much meant smp_mb() here :-)

In which case, we don't provide the transitivity guarantees that you would
get from an smp_mb().

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]

#1249009

On 16 October 2015 at 17:04, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Fri, Oct 16, 2015 at 05:18:30PM +0200, Peter Zijlstra wrote:
>> If so, however, I suspect AARGH64 is borken and would need (just like
>> PPC):
>>
>> #define smp_mb__before_spinlock()     smp_mb()
>>
>> The problem is that schedule() (when a NO-OP) does:
>>
>>       smp_mb__before_spinlock();
>>       LOCK rq->lock
>>
>>       clear_bit()
>>
>>       UNLOCK rq->lock
>>
>> And nothing there implies a full barrier on AARGH64, since
>> smp_mb__before_spinlock() defaults to WMB, LOCK is an "ldaxr" or
>> load-acquire, UNLOCK is "stlrh" or store-release and clear_bit() isn't
>> anything.
>>
>> Pretty much every other arch has LOCK implying a full barrier, either
>> because its strongly ordered or because it needs one for the ACQUIRE
>> semantics.
>
> Well, arm64 might well need smp_mb__after_unlock_lock() to be non-empty.
> But I thought that it used a dmb in the spinlock code somewhere or
> another...

unlock+lock on arm64 is a full barrier, so, IIUC the semantics of
smp_mb__after_unlock_lock(), I don't think we need this to be
non-empty. Maybe redefining smp_mb__before_spinlock() to be smp_mb()
(rather than wmb) on arm64 if we need smp_mb__before_spinlock+lock to
act as a barrier for both reads and writes (see my other reply to
Peter).

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

Back to top | Article view | linux.kernel

csiph-web