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

Groups > linux.kernel > #1475385 > unrolled thread

Memory barrier needed with wake_up_process()?

Back to article view | Back to linux.kernel

Contents

  Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-02 20:20 +0200
    Re: Memory barrier needed with wake_up_process()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-02 20:50 +0200
      Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-02 22:30 +0200
        Re: Memory barrier needed with wake_up_process()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-03 11:10 +0200
        Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-03 14:40 +0200
          Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-03 16:30 +0200
            Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-03 16:50 +0200
              Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:40 +0200
                Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-05 17:30 +0200
                  Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-06 13:40 +0200
                    Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-06 13:50 +0200
                      Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-06 14:30 +0200
                        Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-06 16:50 +0200
                          Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-06 17:10 +0200
                          Re: Memory barrier needed with wake_up_process()? Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-09-07 12:20 +0200
                    Re: Memory barrier needed with wake_up_process()? Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-09-06 13:50 +0200
    Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-02 21:30 +0200
      Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-02 22:20 +0200
        Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-03 00:20 +0200
          Re: Memory barrier needed with wake_up_process()? Will Deacon <will.deacon@arm.com> - 2016-09-05 11:50 +0200
            Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-06 13:20 +0200
        Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-03 00:20 +0200
          Re: Memory barrier needed with wake_up_process()? Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-09-03 09:00 +0200
            Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-03 14:50 +0200
              Re: Memory barrier needed with wake_up_process()? Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-09-03 16:00 +0200
                Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:10 +0200
              Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-03 16:20 +0200
                Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-05 10:10 +0200
                  Re: Memory barrier needed with wake_up_process()? Alan Stern <stern@rowland.harvard.edu> - 2016-09-05 16:40 +0200
        Re: Memory barrier needed with wake_up_process()? Peter Zijlstra <peterz@infradead.org> - 2016-09-03 00:20 +0200

Page 1 of 2  [1] 2  Next page →

#1475385 — Memory barrier needed with wake_up_process()?

Paul, Peter, and Ingo:

This must have come up before, but I don't know what was decided.

Isn't it often true that a memory barrier is needed before a call to 
wake_up_process()?  A typical scenario might look like this:

	CPU 0
	-----
	for (;;) {
		set_current_state(TASK_INTERRUPTIBLE);
		if (signal_pending(current))
			break;
		if (wakeup_flag)
			break;
		schedule();
	}
	__set_current_state(TASK_RUNNING);
	wakeup_flag = 0;


	CPU 1
	-----
	wakeup_flag = 1;
	wake_up_process(my_task);

The underlying pattern is:

	CPU 0				CPU 1
	-----				-----
	write current->state		write wakeup_flag
	smp_mb();
	read wakeup_flag		read my_task->state

where set_current_state() does the write to current->state and 
automatically adds the smp_mb(), and wake_up_process() reads 
my_task->state to see whether the task needs to be woken up.

The kerneldoc for wake_up_process() says that it has no implied memory
barrier if it doesn't actually wake anything up.  And even when it
does, the implied barrier is only smp_wmb, not smp_mb.

This is the so-called SB (Store Buffer) pattern, which is well known to
require a full smp_mb on both sides.  Since wake_up_process() doesn't
include smp_mb(), isn't it correct that the caller must add it
explicitly?

In other words, shouldn't the code for CPU 1 really be:

	wakeup_flag = 1;
	smp_mb();
	wake_up_process(task);

If my reasoning is correct, then why doesn't wake_up_process() include 
this memory barrier automatically, the way set_current_state() does?  
There could be an alternate version (__wake_up_process()) which omits 
the barrier, just like __set_current_state().

Alan Stern

[toc] | [next] | [standalone]

#1475404

On Fri, Sep 02, 2016 at 02:10:13PM -0400, Alan Stern wrote:
> Paul, Peter, and Ingo:
> 
> This must have come up before, but I don't know what was decided.
> 
> Isn't it often true that a memory barrier is needed before a call to 
> wake_up_process()?  A typical scenario might look like this:
> 
> 	CPU 0
> 	-----
> 	for (;;) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (signal_pending(current))
> 			break;
> 		if (wakeup_flag)
> 			break;
> 		schedule();
> 	}
> 	__set_current_state(TASK_RUNNING);
> 	wakeup_flag = 0;
> 
> 
> 	CPU 1
> 	-----
> 	wakeup_flag = 1;
> 	wake_up_process(my_task);
> 
> The underlying pattern is:
> 
> 	CPU 0				CPU 1
> 	-----				-----
> 	write current->state		write wakeup_flag
> 	smp_mb();
> 	read wakeup_flag		read my_task->state
> 
> where set_current_state() does the write to current->state and 
> automatically adds the smp_mb(), and wake_up_process() reads 
> my_task->state to see whether the task needs to be woken up.
> 
> The kerneldoc for wake_up_process() says that it has no implied memory
> barrier if it doesn't actually wake anything up.  And even when it
> does, the implied barrier is only smp_wmb, not smp_mb.
> 
> This is the so-called SB (Store Buffer) pattern, which is well known to
> require a full smp_mb on both sides.  Since wake_up_process() doesn't
> include smp_mb(), isn't it correct that the caller must add it
> explicitly?
> 
> In other words, shouldn't the code for CPU 1 really be:
> 
> 	wakeup_flag = 1;
> 	smp_mb();
> 	wake_up_process(task);
> 
> If my reasoning is correct, then why doesn't wake_up_process() include 
> this memory barrier automatically, the way set_current_state() does?  
> There could be an alternate version (__wake_up_process()) which omits 
> the barrier, just like __set_current_state().

A common case uses locking, in which case additional memory barriers
inside of the wait/wakeup functions are not needed.  Any accesses made
while holding the lock before invoking the wakeup function (e.g.,
wake_up()) are guaranteed to be seen after acquiring that same
lock following return from the wait function (e.g., wait_event()).
In this case, adding barriers to the wait and wakeup functions would
just add overhead.

But yes, this decision does mean that people using the wait/wakeup
functions without locking need to be more careful.  Something like
this:

	/* prior accesses. */
	smp_mb();
	wakeup_flag = 1;
	wake_up(...);

And on the other task:

	wait_event(... wakeup_flag == 1 ...);
	smp_mb();
	/* The waker's prior accesses will be visible here. */

Or am I missing your point?

						Thanx, Paul

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

#1475435

On Fri, 2 Sep 2016, Paul E. McKenney wrote:

> On Fri, Sep 02, 2016 at 02:10:13PM -0400, Alan Stern wrote:
> > Paul, Peter, and Ingo:
> > 
> > This must have come up before, but I don't know what was decided.
> > 
> > Isn't it often true that a memory barrier is needed before a call to 
> > wake_up_process()?  A typical scenario might look like this:
> > 
> > 	CPU 0
> > 	-----
> > 	for (;;) {
> > 		set_current_state(TASK_INTERRUPTIBLE);
> > 		if (signal_pending(current))
> > 			break;
> > 		if (wakeup_flag)
> > 			break;
> > 		schedule();
> > 	}
> > 	__set_current_state(TASK_RUNNING);
> > 	wakeup_flag = 0;
> > 
> > 
> > 	CPU 1
> > 	-----
> > 	wakeup_flag = 1;
> > 	wake_up_process(my_task);
> > 
> > The underlying pattern is:
> > 
> > 	CPU 0				CPU 1
> > 	-----				-----
> > 	write current->state		write wakeup_flag
> > 	smp_mb();
> > 	read wakeup_flag		read my_task->state
> > 
> > where set_current_state() does the write to current->state and 
> > automatically adds the smp_mb(), and wake_up_process() reads 
> > my_task->state to see whether the task needs to be woken up.
> > 
> > The kerneldoc for wake_up_process() says that it has no implied memory
> > barrier if it doesn't actually wake anything up.  And even when it
> > does, the implied barrier is only smp_wmb, not smp_mb.
> > 
> > This is the so-called SB (Store Buffer) pattern, which is well known to
> > require a full smp_mb on both sides.  Since wake_up_process() doesn't
> > include smp_mb(), isn't it correct that the caller must add it
> > explicitly?
> > 
> > In other words, shouldn't the code for CPU 1 really be:
> > 
> > 	wakeup_flag = 1;
> > 	smp_mb();
> > 	wake_up_process(task);
> > 
> > If my reasoning is correct, then why doesn't wake_up_process() include 
> > this memory barrier automatically, the way set_current_state() does?  
> > There could be an alternate version (__wake_up_process()) which omits 
> > the barrier, just like __set_current_state().
> 
> A common case uses locking, in which case additional memory barriers
> inside of the wait/wakeup functions are not needed.  Any accesses made
> while holding the lock before invoking the wakeup function (e.g.,
> wake_up()) are guaranteed to be seen after acquiring that same
> lock following return from the wait function (e.g., wait_event()).
> In this case, adding barriers to the wait and wakeup functions would
> just add overhead.
> 
> But yes, this decision does mean that people using the wait/wakeup
> functions without locking need to be more careful.  Something like
> this:
> 
> 	/* prior accesses. */
> 	smp_mb();
> 	wakeup_flag = 1;
> 	wake_up(...);
> 
> And on the other task:
> 
> 	wait_event(... wakeup_flag == 1 ...);
> 	smp_mb();
> 	/* The waker's prior accesses will be visible here. */
> 
> Or am I missing your point?

I'm afraid so.  The code doesn't use wait_event(), in part because
there's no wait_queue (since only one task is involved).

But maybe there's another barrier which needs to be fixed.  Felipe, can
you check to see if received_cbw() is getting called in
get_next_command(), and if so, what value it returns?  Or is the
preceding sleep_thread() the one that never wakes up?

It could be that the smp_wmb() in wakeup_thread() needs to be smp_mb().  
The reason being that get_next_command() runs outside the protection of 
the spinlock.

Alan Stern

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

#1475573

On Fri, Sep 02, 2016 at 04:29:19PM -0400, Alan Stern wrote:
> On Fri, 2 Sep 2016, Paul E. McKenney wrote:
> 
> > On Fri, Sep 02, 2016 at 02:10:13PM -0400, Alan Stern wrote:
> > > Paul, Peter, and Ingo:
> > > 
> > > This must have come up before, but I don't know what was decided.
> > > 
> > > Isn't it often true that a memory barrier is needed before a call to 
> > > wake_up_process()?  A typical scenario might look like this:
> > > 
> > > 	CPU 0
> > > 	-----
> > > 	for (;;) {
> > > 		set_current_state(TASK_INTERRUPTIBLE);
> > > 		if (signal_pending(current))
> > > 			break;
> > > 		if (wakeup_flag)
> > > 			break;
> > > 		schedule();
> > > 	}
> > > 	__set_current_state(TASK_RUNNING);
> > > 	wakeup_flag = 0;
> > > 
> > > 
> > > 	CPU 1
> > > 	-----
> > > 	wakeup_flag = 1;
> > > 	wake_up_process(my_task);
> > > 
> > > The underlying pattern is:
> > > 
> > > 	CPU 0				CPU 1
> > > 	-----				-----
> > > 	write current->state		write wakeup_flag
> > > 	smp_mb();
> > > 	read wakeup_flag		read my_task->state
> > > 
> > > where set_current_state() does the write to current->state and 
> > > automatically adds the smp_mb(), and wake_up_process() reads 
> > > my_task->state to see whether the task needs to be woken up.
> > > 
> > > The kerneldoc for wake_up_process() says that it has no implied memory
> > > barrier if it doesn't actually wake anything up.  And even when it
> > > does, the implied barrier is only smp_wmb, not smp_mb.
> > > 
> > > This is the so-called SB (Store Buffer) pattern, which is well known to
> > > require a full smp_mb on both sides.  Since wake_up_process() doesn't
> > > include smp_mb(), isn't it correct that the caller must add it
> > > explicitly?
> > > 
> > > In other words, shouldn't the code for CPU 1 really be:
> > > 
> > > 	wakeup_flag = 1;
> > > 	smp_mb();
> > > 	wake_up_process(task);
> > > 
> > > If my reasoning is correct, then why doesn't wake_up_process() include 
> > > this memory barrier automatically, the way set_current_state() does?  
> > > There could be an alternate version (__wake_up_process()) which omits 
> > > the barrier, just like __set_current_state().
> > 
> > A common case uses locking, in which case additional memory barriers
> > inside of the wait/wakeup functions are not needed.  Any accesses made
> > while holding the lock before invoking the wakeup function (e.g.,
> > wake_up()) are guaranteed to be seen after acquiring that same
> > lock following return from the wait function (e.g., wait_event()).
> > In this case, adding barriers to the wait and wakeup functions would
> > just add overhead.
> > 
> > But yes, this decision does mean that people using the wait/wakeup
> > functions without locking need to be more careful.  Something like
> > this:
> > 
> > 	/* prior accesses. */
> > 	smp_mb();
> > 	wakeup_flag = 1;
> > 	wake_up(...);
> > 
> > And on the other task:
> > 
> > 	wait_event(... wakeup_flag == 1 ...);
> > 	smp_mb();
> > 	/* The waker's prior accesses will be visible here. */
> > 
> > Or am I missing your point?
> 
> I'm afraid so.  The code doesn't use wait_event(), in part because
> there's no wait_queue (since only one task is involved).

Ah, got it.

The required pattern should be very similar, however.

> But maybe there's another barrier which needs to be fixed.  Felipe, can
> you check to see if received_cbw() is getting called in
> get_next_command(), and if so, what value it returns?  Or is the
> preceding sleep_thread() the one that never wakes up?
> 
> It could be that the smp_wmb() in wakeup_thread() needs to be smp_mb().  
> The reason being that get_next_command() runs outside the protection of 
> the spinlock.

This sounds very likely to me.

							Thanx, Paul

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

#1475620

On Fri, Sep 02, 2016 at 04:29:19PM -0400, Alan Stern wrote:
> I'm afraid so.  The code doesn't use wait_event(), in part because
> there's no wait_queue (since only one task is involved).

You can use wait_queue fine with just one task, and it would clean up
the code tremendously.

You can replace things like the earlier mentioned:

	while (bh->state != BUF_STATE_EMPTY) {
		rc = sleep_thread(common, false);
		if (rc)
			return rc;
	}

with:

	rc = wait_event_interruptible(&common->wq, bh->state == BUF_STATE_EMPTY);
	if (rc)
		return rc;

> But maybe there's another barrier which needs to be fixed.  Felipe, can
> you check to see if received_cbw() is getting called in
> get_next_command(), and if so, what value it returns?  Or is the
> preceding sleep_thread() the one that never wakes up?
> 
> It could be that the smp_wmb() in wakeup_thread() needs to be smp_mb().  
> The reason being that get_next_command() runs outside the protection of 
> the spinlock.

Being somewhat confused by the code, I fail to follow that argument.
wakeup_thread() is always called under that spinlock(), but since the
critical section is 2 stores, I fail to see how a smp_mb() can make any
difference over the smp_wmb() already there.

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

#1475649

On Sat, 3 Sep 2016, Peter Zijlstra wrote:

> On Fri, Sep 02, 2016 at 04:29:19PM -0400, Alan Stern wrote:
> > I'm afraid so.  The code doesn't use wait_event(), in part because
> > there's no wait_queue (since only one task is involved).
> 
> You can use wait_queue fine with just one task, and it would clean up
> the code tremendously.
> 
> You can replace things like the earlier mentioned:
> 
> 	while (bh->state != BUF_STATE_EMPTY) {
> 		rc = sleep_thread(common, false);
> 		if (rc)
> 			return rc;
> 	}
> 
> with:
> 
> 	rc = wait_event_interruptible(&common->wq, bh->state == BUF_STATE_EMPTY);
> 	if (rc)
> 		return rc;

If someone wants to devote time and effort to cleaning up the driver, 
that would be a good start.

> > But maybe there's another barrier which needs to be fixed.  Felipe, can
> > you check to see if received_cbw() is getting called in
> > get_next_command(), and if so, what value it returns?  Or is the
> > preceding sleep_thread() the one that never wakes up?
> > 
> > It could be that the smp_wmb() in wakeup_thread() needs to be smp_mb().  
> > The reason being that get_next_command() runs outside the protection of 
> > the spinlock.
> 
> Being somewhat confused by the code, I fail to follow that argument.
> wakeup_thread() is always called under that spinlock(), but since the
> critical section is 2 stores, I fail to see how a smp_mb() can make any
> difference over the smp_wmb() already there.

But sleep_thread() and the code that follows it are _not_ called under
the spinlock.  And the following code examines values that were written
by DMA, not by the CPU calling wakeup_thread().  (Although that CPU
_is_ the one that receives the DMA-completion notice.)

In other words, we have:

	CPU 0				CPU 1
	-----				-----
	Start DMA			Handle DMA-complete irq
	Sleep until bh->state		Set bh->state
					smp_wmb()
					Wake up CPU 0
	smp_rmb()
	Compute rc based on contents
		of the DMA buffer

This was written many years ago, at a time when I did not fully
understand all the details of memory ordering.  Do you agree that both
of those barriers should really be smp_mb()?  That's what Felipe has
been testing.

Alan Stern

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

#1475650

On Sat, 3 Sep 2016, Alan Stern wrote:

> In other words, we have:
> 
> 	CPU 0				CPU 1
> 	-----				-----
> 	Start DMA			Handle DMA-complete irq
> 	Sleep until bh->state		Set bh->state
> 					smp_wmb()
> 					Wake up CPU 0
> 	smp_rmb()
> 	Compute rc based on contents
> 		of the DMA buffer
> 
> This was written many years ago, at a time when I did not fully
> understand all the details of memory ordering.  Do you agree that both
> of those barriers should really be smp_mb()?  That's what Felipe has
> been testing.

Actually, seeing it written out like this, one realizes that it really 
ought to be:

	CPU 0				CPU 1
        -----				-----
        Start DMA			Handle DMA-complete irq
        Sleep until bh->state		smp_mb()
					set bh->state
					Wake up CPU 0
	smp_mb()
	Compute rc based on contents of the DMA buffer

(Bear in mind also that on some platforms, the I/O operation is carried 
out by PIO rather than DMA.)

Also, the smp_wmb() in bulk_out_complete() looks unnecessary.  I can't 
remember why I put it there originally.

Alan Stern

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

#1476198

On Sat, Sep 03, 2016 at 10:49:39AM -0400, Alan Stern wrote:
> On Sat, 3 Sep 2016, Alan Stern wrote:
> 
> > In other words, we have:
> > 
> > 	CPU 0				CPU 1
> > 	-----				-----
> > 	Start DMA			Handle DMA-complete irq
> > 	Sleep until bh->state		Set bh->state
> > 					smp_wmb()
> > 					Wake up CPU 0
> > 	smp_rmb()
> > 	Compute rc based on contents
> > 		of the DMA buffer
> > 
> > This was written many years ago, at a time when I did not fully
> > understand all the details of memory ordering.  Do you agree that both
> > of those barriers should really be smp_mb()?  That's what Felipe has
> > been testing.
> 
> Actually, seeing it written out like this, one realizes that it really 
> ought to be:
> 
> 	CPU 0				CPU 1
>         -----				-----
>         Start DMA			Handle DMA-complete irq
>         Sleep until bh->state		smp_mb()
> 					set bh->state
> 					Wake up CPU 0
> 	smp_mb()
> 	Compute rc based on contents of the DMA buffer
> 
> (Bear in mind also that on some platforms, the I/O operation is carried 
> out by PIO rather than DMA.)

I'm sorry, but I still don't follow. This could be because I seldom
interact with DMA agents and therefore am not familiar with that stuff.

Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
Its only defined to do CPU/CPU interactions.

I would very much expect the device IO stuff to order things for us in
this case. "Start DMA" should very much include sufficient fences to
ensure the data under DMA is visible to the DMA engine, this would very
much include things like flushing store buffers and maybe even writeback
caches, depending on platform needs.

At the same time, I would expect "Handle DMA-complete irq", even if it
were done with a PIO polling loop, to guarantee ordering against later
operations such that 'complete' really means that.

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

#1476560

On Mon, 5 Sep 2016, Peter Zijlstra wrote:

> > Actually, seeing it written out like this, one realizes that it really 
> > ought to be:
> > 
> > 	CPU 0				CPU 1
> >         -----				-----
> >         Start DMA			Handle DMA-complete irq
> >         Sleep until bh->state		smp_mb()
> > 					set bh->state
> > 					Wake up CPU 0
> > 	smp_mb()
> > 	Compute rc based on contents of the DMA buffer
> > 
> > (Bear in mind also that on some platforms, the I/O operation is carried 
> > out by PIO rather than DMA.)
> 
> I'm sorry, but I still don't follow. This could be because I seldom
> interact with DMA agents and therefore am not familiar with that stuff.

I haven't seen the details of memory ordering requirements in the
presence of DMA spelled out anywhere.  The documents I have read merely
state that you have to careful to flush caches before doing DMA OUT and
to avoid filling caches before DMA IN is complete.  Neither of those is
an issue here, apparently.

> Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
> Its only defined to do CPU/CPU interactions.

Suppose the DMA master finishes filling up an input buffer and issues a
completion irq to CPU1.  Presumably the data would then be visible to
CPU1 if the interrupt handler looked at it.  So if CPU1 executes smp_mb
before setting bh->state and waking up CPU0, and if CPU0 executes
smp_mb after testing bh->state and before reading the data buffer,
wouldn't CPU0 then see the correct data in the buffer?  Even if CPU0 
never did go to sleep?

Or would something more be needed?

> I would very much expect the device IO stuff to order things for us in
> this case. "Start DMA" should very much include sufficient fences to
> ensure the data under DMA is visible to the DMA engine, this would very
> much include things like flushing store buffers and maybe even writeback
> caches, depending on platform needs.
> 
> At the same time, I would expect "Handle DMA-complete irq", even if it
> were done with a PIO polling loop, to guarantee ordering against later
> operations such that 'complete' really means that.

That's what I would expect too.

Back in the original email thread where the problem was first reported, 
Felipe says that the problem appears to be something else.  Here's what 
it looks like now, in schematic form:

	CPU0
	----
	get_next_command():
	  while (bh->state != BUF_STATE_EMPTY)
		sleep_thread();
	  start an input request for bh
	  while (bh->state != BUF_STATE_FULL)
		sleep_thread();

As mentioned above, the input involves DMA and is terminated by an irq.
The request's completion handler is bulk_out_complete():

	CPU1
	----
	bulk_out_complete():
	  bh->state = BUF_STATE_FULL;
	  wakeup_thread();

According to Felipe, when CPU0 wakes up and checks bh->state, it sees a 
value different from BUF_STATE_FULL.  So it goes back to sleep again 
and doesn't make any forward progress.

It's possible that something else is changing bh->state when it 
shouldn't.  But if this were the explanation, why would Felipe see that 
the problem goes away when he changes the memory barriers in 
sleep_thread() and wakeup_thread() to smp_mb()?

Alan Stern

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

#1477293

On Mon, Sep 05, 2016 at 11:29:26AM -0400, Alan Stern wrote:
> On Mon, 5 Sep 2016, Peter Zijlstra wrote:
> 
> > > Actually, seeing it written out like this, one realizes that it really 
> > > ought to be:
> > > 
> > > 	CPU 0				CPU 1
> > >         -----				-----
> > >         Start DMA			Handle DMA-complete irq
> > >         Sleep until bh->state		smp_mb()
> > > 					set bh->state
> > > 					Wake up CPU 0
> > > 	smp_mb()
> > > 	Compute rc based on contents of the DMA buffer
> > > 
> > > (Bear in mind also that on some platforms, the I/O operation is carried 
> > > out by PIO rather than DMA.)

> > Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
> > Its only defined to do CPU/CPU interactions.
> 
> Suppose the DMA master finishes filling up an input buffer and issues a
> completion irq to CPU1.  Presumably the data would then be visible to
> CPU1 if the interrupt handler looked at it.  So if CPU1 executes smp_mb
> before setting bh->state and waking up CPU0, and if CPU0 executes
> smp_mb after testing bh->state and before reading the data buffer,
> wouldn't CPU0 then see the correct data in the buffer?  Even if CPU0 
> never did go to sleep?

Couple of notes here; I would expect the DMA master to make its stores
_globally_ visible on 'completion'. Because I'm not sure our smp_mb()
would help make it globally visible, since its only defined on CPU/CPU
interactions, not external.

Note that for example ARM has the distinction where smp_mb() uses
DMB-ISH barrier, dma_[rw]mb() uses DMB-OSH{LD,ST} and mb() uses DSB-SY.

The ISH domain is the Inner-SHarable (IIRC) and only includes CPUs. The
OSH is Outer-SHarable and adds external agents like DMA (also includes
CPUs). The DSB-SY thing is even heavier and syncs world or something; I
always forget these details.

> Or would something more be needed?

The thing is, this is x86 (TSO). Most everything is globally
ordered/visible and full barriers.

The only reorder allowed by TSO is a later read can happen before a
prior store. That is, only:

	X = 1;
	smp_mb();
	r = Y;

actually needs a full barrier. All the other variants are no-ops.

Note that x86's dma_[rw]mb() are no-ops (like the regular smp_[rw]mb()).
Which seems to imply DMA is coherent and globally visible.

> > I would very much expect the device IO stuff to order things for us in
> > this case. "Start DMA" should very much include sufficient fences to
> > ensure the data under DMA is visible to the DMA engine, this would very
> > much include things like flushing store buffers and maybe even writeback
> > caches, depending on platform needs.
> > 
> > At the same time, I would expect "Handle DMA-complete irq", even if it
> > were done with a PIO polling loop, to guarantee ordering against later
> > operations such that 'complete' really means that.
> 
> That's what I would expect too.
> 
> Back in the original email thread where the problem was first reported, 
> Felipe says that the problem appears to be something else.  Here's what 
> it looks like now, in schematic form:
> 
> 	CPU0
> 	----
> 	get_next_command():
> 	  while (bh->state != BUF_STATE_EMPTY)
> 		sleep_thread();
> 	  start an input request for bh
> 	  while (bh->state != BUF_STATE_FULL)
> 		sleep_thread();
> 
> As mentioned above, the input involves DMA and is terminated by an irq.
> The request's completion handler is bulk_out_complete():
> 
> 	CPU1
> 	----
> 	bulk_out_complete():
> 	  bh->state = BUF_STATE_FULL;
> 	  wakeup_thread();
> 
> According to Felipe, when CPU0 wakes up and checks bh->state, it sees a 
> value different from BUF_STATE_FULL.  So it goes back to sleep again 
> and doesn't make any forward progress.
> 
> It's possible that something else is changing bh->state when it 
> shouldn't.  But if this were the explanation, why would Felipe see that 
> the problem goes away when he changes the memory barriers in 
> sleep_thread() and wakeup_thread() to smp_mb()?

Good question, let me stare at the code more.

Could you confirm that bulk_{in,out}_complete() work on different
usb_request structures, and they can not, at any time, get called on the
_same_ request?

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

#1477294

On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:

> > Could you confirm that bulk_{in,out}_complete() work on different
> > usb_request structures, and they can not, at any time, get called on the
> > _same_ request?
> 
> usb_requests are allocated for a specific endpoint and USB Device
> Controller (UDC) drivers refuse to queue requests allocated for epX to
> epY, so this really can never happen.

Good, thanks!

> My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> adds extra overhead which makes the problem much, much less likely to
> happen. Does that sound plausible to you?

I did consider that, but I've not sufficiently grokked the code to rule
out actual fail. So let me stare at this a bit more.

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

#1477321

On Tue, Sep 06, 2016 at 01:49:37PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:

> > My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> > adds extra overhead which makes the problem much, much less likely to
> > happen. Does that sound plausible to you?
> 
> I did consider that, but I've not sufficiently grokked the code to rule
> out actual fail. So let me stare at this a bit more.

OK, so I'm really not seeing it, we've got:

while (bh->state != FULL) {
        for (;;) {
                set_current_state(INTERRUPTIBLE); /* MB after */
                if (signal_pending(current))
                        return -EINTR;
                if (common->thread_wakeup_needed)
                        break;
                schedule(); /* MB */
        }
        __set_current_state(RUNNING);
        common->thread_wakeup_needed = 0;
        smp_rmb(); /* NOP */
}


VS.


spin_lock(&common->lock); /* MB */
bh->state = FULL;
smp_wmb(); /* NOP */
common->thread_wakeup_needed = 1;
wake_up_process(common->thread_task); /* MB before */
spin_unlock(&common->lock);



(the MB annotations specific to x86, not true in general)


If we observe thread_wakeup_needed, we must also observe bh->state.

And the sleep/wakeup ordering is also correct, we either see
thread_wakeup_needed and continue, or we see task->state == RUNNING
(from the wakeup) and NO-OP schedule(). The MB from set_current_statE()
then matches with the MB from wake_up_process() to ensure we must see
thead_wakeup_needed.

Or, we go sleep, and get woken up, at which point the same happens.
Since the waking CPU gets the task back on its RQ the happens-before
chain includes the waking CPUs state along with the state of the task
itself before it went to sleep.

At which point we're back where we started, once we see
thread_wakeup_needed we must then also see bh->state (and all state
prior to that on the waking CPU).



There's enough cruft in the while-sleep loop to force reload bh->state.

Load/store tearing cannot be a problem because all values are single
bytes (the variables are multi bytes, but all values used only affect
the LSB).

Colour me puzzled.

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

#1477478

On Tue, 6 Sep 2016, Peter Zijlstra wrote:

> On Tue, Sep 06, 2016 at 01:49:37PM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:
> 
> > > My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> > > adds extra overhead which makes the problem much, much less likely to
> > > happen. Does that sound plausible to you?
> > 
> > I did consider that, but I've not sufficiently grokked the code to rule
> > out actual fail. So let me stare at this a bit more.
> 
> OK, so I'm really not seeing it, we've got:
> 
> while (bh->state != FULL) {
>         for (;;) {
>                 set_current_state(INTERRUPTIBLE); /* MB after */
>                 if (signal_pending(current))
>                         return -EINTR;
>                 if (common->thread_wakeup_needed)
>                         break;
>                 schedule(); /* MB */
>         }
>         __set_current_state(RUNNING);
>         common->thread_wakeup_needed = 0;
>         smp_rmb(); /* NOP */
> }
> 
> 
> VS.
> 
> 
> spin_lock(&common->lock); /* MB */
> bh->state = FULL;
> smp_wmb(); /* NOP */
> common->thread_wakeup_needed = 1;
> wake_up_process(common->thread_task); /* MB before */
> spin_unlock(&common->lock);
> 
> 
> 
> (the MB annotations specific to x86, not true in general)
> 
> 
> If we observe thread_wakeup_needed, we must also observe bh->state.
> 
> And the sleep/wakeup ordering is also correct, we either see
> thread_wakeup_needed and continue, or we see task->state == RUNNING
> (from the wakeup) and NO-OP schedule(). The MB from set_current_statE()
> then matches with the MB from wake_up_process() to ensure we must see
> thead_wakeup_needed.
> 
> Or, we go sleep, and get woken up, at which point the same happens.
> Since the waking CPU gets the task back on its RQ the happens-before
> chain includes the waking CPUs state along with the state of the task
> itself before it went to sleep.
> 
> At which point we're back where we started, once we see
> thread_wakeup_needed we must then also see bh->state (and all state
> prior to that on the waking CPU).
> 
> 
> 
> There's enough cruft in the while-sleep loop to force reload bh->state.
> 
> Load/store tearing cannot be a problem because all values are single
> bytes (the variables are multi bytes, but all values used only affect
> the LSB).
> 
> Colour me puzzled.

Felipe, can you please try this patch on an unmodified tree?  If the 
problem still occurs, what shows up in the kernel log?

Alan Stern



Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
+++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
@@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb
 	spin_lock(&common->lock);
 	bh->outreq_busy = 0;
 	bh->state = BUF_STATE_FULL;
+	if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
+		INFO(common, "compl: bh %p state %d\n", bh, bh->state);
 	wakeup_thread(common);
 	spin_unlock(&common->lock);
 }
@@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_c
 		rc = sleep_thread(common, true);
 		if (rc)
 			return rc;
+		INFO(common, "next: bh %p state %d\n", bh, bh->state);
 	}
 	smp_rmb();
 	rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;

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

#1477493

On Tue, Sep 06, 2016 at 10:46:55AM -0400, Alan Stern wrote:

Not knowing where INFO() goes, you should use trace_printk() not
printk(), as the former is strictly per cpu, while the latter is
globally serialized and can hide all these problems.

> Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
> +++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> @@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb
>  	spin_lock(&common->lock);
>  	bh->outreq_busy = 0;
>  	bh->state = BUF_STATE_FULL;
> +	if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
> +		INFO(common, "compl: bh %p state %d\n", bh, bh->state);
>  	wakeup_thread(common);
>  	spin_unlock(&common->lock);
>  }
> @@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_c
>  		rc = sleep_thread(common, true);
>  		if (rc)
>  			return rc;
> +		INFO(common, "next: bh %p state %d\n", bh, bh->state);
>  	}
>  	smp_rmb();
>  	rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;
> 

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

#1478140

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

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Tue, 6 Sep 2016, Peter Zijlstra wrote:
>
>> On Tue, Sep 06, 2016 at 01:49:37PM +0200, Peter Zijlstra wrote:
>> > On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:
>> 
>> > > My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
>> > > adds extra overhead which makes the problem much, much less likely to
>> > > happen. Does that sound plausible to you?
>> > 
>> > I did consider that, but I've not sufficiently grokked the code to rule
>> > out actual fail. So let me stare at this a bit more.
>> 
>> OK, so I'm really not seeing it, we've got:
>> 
>> while (bh->state != FULL) {
>>         for (;;) {
>>                 set_current_state(INTERRUPTIBLE); /* MB after */
>>                 if (signal_pending(current))
>>                         return -EINTR;
>>                 if (common->thread_wakeup_needed)
>>                         break;
>>                 schedule(); /* MB */
>>         }
>>         __set_current_state(RUNNING);
>>         common->thread_wakeup_needed = 0;
>>         smp_rmb(); /* NOP */
>> }
>> 
>> 
>> VS.
>> 
>> 
>> spin_lock(&common->lock); /* MB */
>> bh->state = FULL;
>> smp_wmb(); /* NOP */
>> common->thread_wakeup_needed = 1;
>> wake_up_process(common->thread_task); /* MB before */
>> spin_unlock(&common->lock);
>> 
>> 
>> 
>> (the MB annotations specific to x86, not true in general)
>> 
>> 
>> If we observe thread_wakeup_needed, we must also observe bh->state.
>> 
>> And the sleep/wakeup ordering is also correct, we either see
>> thread_wakeup_needed and continue, or we see task->state == RUNNING
>> (from the wakeup) and NO-OP schedule(). The MB from set_current_statE()
>> then matches with the MB from wake_up_process() to ensure we must see
>> thead_wakeup_needed.
>> 
>> Or, we go sleep, and get woken up, at which point the same happens.
>> Since the waking CPU gets the task back on its RQ the happens-before
>> chain includes the waking CPUs state along with the state of the task
>> itself before it went to sleep.
>> 
>> At which point we're back where we started, once we see
>> thread_wakeup_needed we must then also see bh->state (and all state
>> prior to that on the waking CPU).
>> 
>> 
>> 
>> There's enough cruft in the while-sleep loop to force reload bh->state.
>> 
>> Load/store tearing cannot be a problem because all values are single
>> bytes (the variables are multi bytes, but all values used only affect
>> the LSB).
>> 
>> Colour me puzzled.
>
> Felipe, can you please try this patch on an unmodified tree?  If the 
> problem still occurs, what shows up in the kernel log?
>
> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
> +++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> @@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb
>  	spin_lock(&common->lock);
>  	bh->outreq_busy = 0;
>  	bh->state = BUF_STATE_FULL;
> +	if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
> +		INFO(common, "compl: bh %p state %d\n", bh, bh->state);
>  	wakeup_thread(common);
>  	spin_unlock(&common->lock);
>  }
> @@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_c
>  		rc = sleep_thread(common, true);
>  		if (rc)
>  			return rc;
> +		INFO(common, "next: bh %p state %d\n", bh, bh->state);
>  	}
>  	smp_rmb();
>  	rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;

I've replace INFO() with trace_printk() (which is what I have been using
anyway):

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 2505117e88e8..dbc6a380b38b 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb_ep *ep, struct usb_request *req)
 	spin_lock(&common->lock);
 	bh->outreq_busy = 0;
 	bh->state = BUF_STATE_FULL;
+	if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
+		trace_printk("compl: bh %p state %d\n", bh, bh->state);
 	wakeup_thread(common);
 	spin_unlock(&common->lock);
 }
@@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_common *common)
 		rc = sleep_thread(common, true);
 		if (rc)
 			return rc;
+		trace_printk("next: bh %p state %d\n", bh, bh->state);
 	}
 	smp_rmb();
 	rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;

But I can't reproduce as reliably as before. I'll keep the thing running
an infinite loop which will stop only when interrupts in UDC (dwc3 in
this case) stop increasing.

-- 
balbi

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

#1477298

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

Hi,

Peter Zijlstra <peterz@infradead.org> writes:
> On Mon, Sep 05, 2016 at 11:29:26AM -0400, Alan Stern wrote:
>> On Mon, 5 Sep 2016, Peter Zijlstra wrote:
>> 
>> > > Actually, seeing it written out like this, one realizes that it really 
>> > > ought to be:
>> > > 
>> > > 	CPU 0				CPU 1
>> > >         -----				-----
>> > >         Start DMA			Handle DMA-complete irq
>> > >         Sleep until bh->state		smp_mb()
>> > > 					set bh->state
>> > > 					Wake up CPU 0
>> > > 	smp_mb()
>> > > 	Compute rc based on contents of the DMA buffer
>> > > 
>> > > (Bear in mind also that on some platforms, the I/O operation is carried 
>> > > out by PIO rather than DMA.)
>
>> > Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
>> > Its only defined to do CPU/CPU interactions.
>> 
>> Suppose the DMA master finishes filling up an input buffer and issues a
>> completion irq to CPU1.  Presumably the data would then be visible to
>> CPU1 if the interrupt handler looked at it.  So if CPU1 executes smp_mb
>> before setting bh->state and waking up CPU0, and if CPU0 executes
>> smp_mb after testing bh->state and before reading the data buffer,
>> wouldn't CPU0 then see the correct data in the buffer?  Even if CPU0 
>> never did go to sleep?
>
> Couple of notes here; I would expect the DMA master to make its stores
> _globally_ visible on 'completion'. Because I'm not sure our smp_mb()
> would help make it globally visible, since its only defined on CPU/CPU
> interactions, not external.
>
> Note that for example ARM has the distinction where smp_mb() uses
> DMB-ISH barrier, dma_[rw]mb() uses DMB-OSH{LD,ST} and mb() uses DSB-SY.
>
> The ISH domain is the Inner-SHarable (IIRC) and only includes CPUs. The
> OSH is Outer-SHarable and adds external agents like DMA (also includes
> CPUs). The DSB-SY thing is even heavier and syncs world or something; I
> always forget these details.
>
>> Or would something more be needed?
>
> The thing is, this is x86 (TSO). Most everything is globally
> ordered/visible and full barriers.
>
> The only reorder allowed by TSO is a later read can happen before a
> prior store. That is, only:
>
> 	X = 1;
> 	smp_mb();
> 	r = Y;
>
> actually needs a full barrier. All the other variants are no-ops.
>
> Note that x86's dma_[rw]mb() are no-ops (like the regular smp_[rw]mb()).
> Which seems to imply DMA is coherent and globally visible.
>
>> > I would very much expect the device IO stuff to order things for us in
>> > this case. "Start DMA" should very much include sufficient fences to
>> > ensure the data under DMA is visible to the DMA engine, this would very
>> > much include things like flushing store buffers and maybe even writeback
>> > caches, depending on platform needs.
>> > 
>> > At the same time, I would expect "Handle DMA-complete irq", even if it
>> > were done with a PIO polling loop, to guarantee ordering against later
>> > operations such that 'complete' really means that.
>> 
>> That's what I would expect too.
>> 
>> Back in the original email thread where the problem was first reported, 
>> Felipe says that the problem appears to be something else.  Here's what 
>> it looks like now, in schematic form:
>> 
>> 	CPU0
>> 	----
>> 	get_next_command():
>> 	  while (bh->state != BUF_STATE_EMPTY)
>> 		sleep_thread();
>> 	  start an input request for bh
>> 	  while (bh->state != BUF_STATE_FULL)
>> 		sleep_thread();
>> 
>> As mentioned above, the input involves DMA and is terminated by an irq.
>> The request's completion handler is bulk_out_complete():
>> 
>> 	CPU1
>> 	----
>> 	bulk_out_complete():
>> 	  bh->state = BUF_STATE_FULL;
>> 	  wakeup_thread();
>> 
>> According to Felipe, when CPU0 wakes up and checks bh->state, it sees a 
>> value different from BUF_STATE_FULL.  So it goes back to sleep again 
>> and doesn't make any forward progress.
>> 
>> It's possible that something else is changing bh->state when it 
>> shouldn't.  But if this were the explanation, why would Felipe see that 
>> the problem goes away when he changes the memory barriers in 
>> sleep_thread() and wakeup_thread() to smp_mb()?
>
> Good question, let me stare at the code more.
>
> Could you confirm that bulk_{in,out}_complete() work on different
> usb_request structures, and they can not, at any time, get called on the
> _same_ request?

usb_requests are allocated for a specific endpoint and USB Device
Controller (UDC) drivers refuse to queue requests allocated for epX to
epY, so this really can never happen.

My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
adds extra overhead which makes the problem much, much less likely to
happen. Does that sound plausible to you?

-- 
balbi

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

#1475420

On Fri, Sep 02, 2016 at 02:10:13PM -0400, Alan Stern wrote:
> Paul, Peter, and Ingo:
> 
> This must have come up before, but I don't know what was decided.
> 
> Isn't it often true that a memory barrier is needed before a call to 
> wake_up_process()?  A typical scenario might look like this:
> 
> 	CPU 0
> 	-----
> 	for (;;) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (signal_pending(current))
> 			break;
> 		if (wakeup_flag)
> 			break;
> 		schedule();
> 	}
> 	__set_current_state(TASK_RUNNING);
> 	wakeup_flag = 0;
> 
> 
> 	CPU 1
> 	-----
> 	wakeup_flag = 1;
> 	wake_up_process(my_task);
> 
> The underlying pattern is:
> 
> 	CPU 0				CPU 1
> 	-----				-----
> 	write current->state		write wakeup_flag
> 	smp_mb();
> 	read wakeup_flag		read my_task->state
> 
> where set_current_state() does the write to current->state and 
> automatically adds the smp_mb(), and wake_up_process() reads 
> my_task->state to see whether the task needs to be woken up.
> 
> The kerneldoc for wake_up_process() says that it has no implied memory
> barrier if it doesn't actually wake anything up.  And even when it
> does, the implied barrier is only smp_wmb, not smp_mb.
> 
> This is the so-called SB (Store Buffer) pattern, which is well known to
> require a full smp_mb on both sides.  Since wake_up_process() doesn't
> include smp_mb(), isn't it correct that the caller must add it
> explicitly?
> 
> In other words, shouldn't the code for CPU 1 really be:
> 
> 	wakeup_flag = 1;
> 	smp_mb();
> 	wake_up_process(task);
> 

No, it doesn't need to do that. try_to_wake_up() does the right thing.

It does:

	smp_mb__before_spinlock();
	raw_spin_lock_irqsave(&p->pi_lock);

Now, smp_mb__before_spinlock() is a bit of an odd duck, if you look at
its comment it says:

/*
 * Despite its name it doesn't necessarily has to be a full barrier.
 * It should only guarantee that a STORE before the critical section
 * can not be reordered with LOADs and STOREs inside this section.
 * spin_lock() is the one-way barrier, this LOAD can not escape out
 * of the region. So the default implementation simply ensures that
 * a STORE can not move into the critical section, smp_wmb() should
 * serialize it with another STORE done by spin_lock().
 */
#ifndef smp_mb__before_spinlock
#define smp_mb__before_spinlock()	smp_wmb()
#endif


So per default it ends up being:

	WMB
	LOCK

Which is sufficient to order the prior store vs the later load as is
required. Note that a spinlock acquire _must_ imply a store (we need to
mark the lock as taken), therefore the prior store is ordered against
the lock store per the wmb, and since the lock must imply an ACQUIRE
that limits the load.


Now, PowerPC defines smp_mb__before_spinlock as smp_mb(), and this is
because PowerPC ACQUIRE is a bit of an exception, if you want more
details I'm sure I or Paul can dredge them up :-)

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

#1475432

On Fri, 2 Sep 2016, Peter Zijlstra wrote:

> On Fri, Sep 02, 2016 at 02:10:13PM -0400, Alan Stern wrote:
> > Paul, Peter, and Ingo:
> > 
> > This must have come up before, but I don't know what was decided.
> > 
> > Isn't it often true that a memory barrier is needed before a call to 
> > wake_up_process()?  A typical scenario might look like this:
> > 
> > 	CPU 0
> > 	-----
> > 	for (;;) {
> > 		set_current_state(TASK_INTERRUPTIBLE);
> > 		if (signal_pending(current))
> > 			break;
> > 		if (wakeup_flag)
> > 			break;
> > 		schedule();
> > 	}
> > 	__set_current_state(TASK_RUNNING);
> > 	wakeup_flag = 0;
> > 
> > 
> > 	CPU 1
> > 	-----
> > 	wakeup_flag = 1;
> > 	wake_up_process(my_task);
> > 
> > The underlying pattern is:
> > 
> > 	CPU 0				CPU 1
> > 	-----				-----
> > 	write current->state		write wakeup_flag
> > 	smp_mb();
> > 	read wakeup_flag		read my_task->state
> > 
> > where set_current_state() does the write to current->state and 
> > automatically adds the smp_mb(), and wake_up_process() reads 
> > my_task->state to see whether the task needs to be woken up.
> > 
> > The kerneldoc for wake_up_process() says that it has no implied memory
> > barrier if it doesn't actually wake anything up.  And even when it
> > does, the implied barrier is only smp_wmb, not smp_mb.
> > 
> > This is the so-called SB (Store Buffer) pattern, which is well known to
> > require a full smp_mb on both sides.  Since wake_up_process() doesn't
> > include smp_mb(), isn't it correct that the caller must add it
> > explicitly?
> > 
> > In other words, shouldn't the code for CPU 1 really be:
> > 
> > 	wakeup_flag = 1;
> > 	smp_mb();
> > 	wake_up_process(task);
> > 
> 
> No, it doesn't need to do that. try_to_wake_up() does the right thing.
> 
> It does:
> 
> 	smp_mb__before_spinlock();
> 	raw_spin_lock_irqsave(&p->pi_lock);
> 
> Now, smp_mb__before_spinlock() is a bit of an odd duck, if you look at
> its comment it says:
> 
> /*
>  * Despite its name it doesn't necessarily has to be a full barrier.
>  * It should only guarantee that a STORE before the critical section
>  * can not be reordered with LOADs and STOREs inside this section.
>  * spin_lock() is the one-way barrier, this LOAD can not escape out
>  * of the region. So the default implementation simply ensures that
>  * a STORE can not move into the critical section, smp_wmb() should
>  * serialize it with another STORE done by spin_lock().
>  */
> #ifndef smp_mb__before_spinlock
> #define smp_mb__before_spinlock()	smp_wmb()
> #endif

I see.  So the kerneldoc for wake_up_process() is misleading at best.

> So per default it ends up being:
> 
> 	WMB
> 	LOCK
> 
> Which is sufficient to order the prior store vs the later load as is
> required. Note that a spinlock acquire _must_ imply a store (we need to
> mark the lock as taken), therefore the prior store is ordered against
> the lock store per the wmb, and since the lock must imply an ACQUIRE
> that limits the load.

Actually, that's not entirely true (although presumably it works okay
for most architectures).  In theory, the first write and the WMB can
move down after the lock's ACQUIRE.  Then the later read could move
earlier, before the lock's store, the WMB, and the first write, but
still after the ACQUIRE.  Thus the later read could be reordered with
the first write.

In other words, these actions:

	store wakeup_flag
	WMB
	load-ACQUIRE lock
	store lock
	read task->state

can be reordered to:

	load-ACQUIRE lock
	store wakeup_flag
	WMB
	store lock
	read task->state

and then to:

	load-ACQUIRE lock
	read task->state
	store wakeup_flag
	WMB
	store lock

without violating any ordering rules.

> Now, PowerPC defines smp_mb__before_spinlock as smp_mb(), and this is
> because PowerPC ACQUIRE is a bit of an exception, if you want more
> details I'm sure I or Paul can dredge them up :-)

PPC can't do the reordering described above, but it does have other
weaknesses.  Without smp_mb(), the write to wakeup_flag doesn't have to
propagate from one CPU to the other before the second CPU tries to read
it.

Felipe, your tests will show whether my guess was totally off-base.  
For the new people, Felipe is tracking down a problem that involves
exactly the code sequence listed at the top of the email, where we know
that the wakeup routine runs but nevertheless the task sleeps.  At
least, that's what it looks like at the moment.

Alan Stern

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

#1475477

On Sat, Sep 03, 2016 at 12:14:13AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> > 
> > Actually, that's not entirely true (although presumably it works okay
> > for most architectures).
> 
> Yeah, all load-store archs (with exception of PowerPC and ARM64 and
> possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).
> 
> ( and MIPS doesn't use their fancy barriers in Linux )
> 
> PowerPC does the full fence for smp_mb__before_spinlock, which leaves
> ARM64, I'm not sure its correct, but I'm way too tired to think about
> that now.
> 
> The TSO archs imply full barriers with all atomic RmW ops and are
> therefore also good.
> 

Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
smp_mb() too?

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

#1476241

On Sat, Sep 03, 2016 at 12:16:29AM +0200, Peter Zijlstra wrote:
> On Sat, Sep 03, 2016 at 12:14:13AM +0200, Peter Zijlstra wrote:
> > On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> > > 
> > > Actually, that's not entirely true (although presumably it works okay
> > > for most architectures).
> > 
> > Yeah, all load-store archs (with exception of PowerPC and ARM64 and
> > possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).
> > 
> > ( and MIPS doesn't use their fancy barriers in Linux )
> > 
> > PowerPC does the full fence for smp_mb__before_spinlock, which leaves
> > ARM64, I'm not sure its correct, but I'm way too tired to think about
> > that now.
> > 
> > The TSO archs imply full barriers with all atomic RmW ops and are
> > therefore also good.
> > 
> 
> Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
> smp_mb() too?

Yes, probably. Just to confirm, the test is something like:


CPU0
----

Wx=1
smp_mb__before_spinlock()
LOCK(y)
Rz=0

CPU1
----

Wz=1
smp_mb()
Rx=0


and that should be forbidden?

Will

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web