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

#1477281

On Mon, Sep 05, 2016 at 10:43:11AM +0100, Will Deacon wrote:
> On Sat, Sep 03, 2016 at 12:16:29AM +0200, Peter Zijlstra wrote:

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

Indeed.

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

#1475479

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> 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.

What arch are you seeing this on?

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

#1475563

hi,

Peter Zijlstra <peterz@infradead.org> writes:
> On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
>> 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.
>
> What arch are you seeing this on?

x86. Skylake to be exact.

The following change survived through the night:

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 8f3659b65f53..d31581dd5ce5 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
 /* Caller must hold fsg->lock */
 static void wakeup_thread(struct fsg_common *common)
 {
-	smp_wmb();	/* ensure the write of bh->state is complete */
+	smp_mb();	/* ensure the write of bh->state is complete */
 	/* Tell the main thread that something has happened */
 	common->thread_wakeup_needed = 1;
 	if (common->thread_task)
@@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool can_freeze)
 	}
 	__set_current_state(TASK_RUNNING);
 	common->thread_wakeup_needed = 0;
-	smp_rmb();	/* ensure the latest bh->state is visible */
+	smp_mb();	/* ensure the latest bh->state is visible */
 	return rc;
 }
 

I'll keep it running until Monday at least

-- 
balbi

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

#1475628

On Sat, Sep 03, 2016 at 09:58:09AM +0300, Felipe Balbi wrote:

> > What arch are you seeing this on?
> 
> x86. Skylake to be exact.

So it _cannot_ be the thing Alan mentioned. By the simple fact that
spin_lock() is a full barrier on x86 (every LOCK prefixed instruction
is).

> The following change survived through the night:
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 8f3659b65f53..d31581dd5ce5 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
>  /* Caller must hold fsg->lock */
>  static void wakeup_thread(struct fsg_common *common)
>  {
> -	smp_wmb();	/* ensure the write of bh->state is complete */
> +	smp_mb();	/* ensure the write of bh->state is complete */
>  	/* Tell the main thread that something has happened */
>  	common->thread_wakeup_needed = 1;
>  	if (common->thread_task)
> @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool can_freeze)
>  	}
>  	__set_current_state(TASK_RUNNING);
>  	common->thread_wakeup_needed = 0;
> -	smp_rmb();	/* ensure the latest bh->state is visible */
> +	smp_mb();	/* ensure the latest bh->state is visible */
>  	return rc;
>  }

Sorry, but that is horrible code. A barrier cannot ensure writes are
'complete', at best they can ensure order between writes (or reads
etc..).

Also, looking at that thing, that common->thread_wakeup_needed variable
is 100% redundant. All sleep_thread() invocations are inside a loop of
sorts and basically wait for other conditions to become true.

For example:

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

All you care about there is bh->state, _not_
common->thread_wakeup_needed.

That said, I cannot spot an obvious fail, but the code can certainly use
help.

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

#1475645

Hi,

Peter Zijlstra <peterz@infradead.org> writes:
> On Sat, Sep 03, 2016 at 09:58:09AM +0300, Felipe Balbi wrote:
>
>> > What arch are you seeing this on?
>> 
>> x86. Skylake to be exact.
>
> So it _cannot_ be the thing Alan mentioned. By the simple fact that
> spin_lock() is a full barrier on x86 (every LOCK prefixed instruction
> is).

I still have this working even after 15 hours of runtime on a test case
that was failing consistently within few minutes. At a minimum smp_mb()
has some side effect which is hiding the actual problem.

>> The following change survived through the night:
>> 
>> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
>> index 8f3659b65f53..d31581dd5ce5 100644
>> --- a/drivers/usb/gadget/function/f_mass_storage.c
>> +++ b/drivers/usb/gadget/function/f_mass_storage.c
>> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
>>  /* Caller must hold fsg->lock */
>>  static void wakeup_thread(struct fsg_common *common)
>>  {
>> -	smp_wmb();	/* ensure the write of bh->state is complete */
>> +	smp_mb();	/* ensure the write of bh->state is complete */
>>  	/* Tell the main thread that something has happened */
>>  	common->thread_wakeup_needed = 1;
>>  	if (common->thread_task)
>> @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool can_freeze)
>>  	}
>>  	__set_current_state(TASK_RUNNING);
>>  	common->thread_wakeup_needed = 0;
>> -	smp_rmb();	/* ensure the latest bh->state is visible */
>> +	smp_mb();	/* ensure the latest bh->state is visible */
>>  	return rc;
>>  }
>
> Sorry, but that is horrible code. A barrier cannot ensure writes are
> 'complete', at best they can ensure order between writes (or reads
> etc..).

not arguing ;-)

> Also, looking at that thing, that common->thread_wakeup_needed variable
> is 100% redundant. All sleep_thread() invocations are inside a loop of
> sorts and basically wait for other conditions to become true.
>
> For example:
>
> 	while (bh->state != BUF_STATE_EMPTY) {
> 		rc = sleep_thread(common, false);
> 		if (rc)
> 			return rc;
> 	}

right

> All you care about there is bh->state, _not_
> common->thread_wakeup_needed.
>
> That said, I cannot spot an obvious fail,

okay, but a fail does exist. Any hints on what extra information I could
capture to help figuring this one out?

> but the code can certainly use help.

Sure, that can be done for v4.9 (if I have time) or v4.10 merge
window. Meanwhile, we're trying to find a minimal fix for the -rc which
can also be backported to stable, right?

-- 
balbi

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

#1476178

On Sat, Sep 03, 2016 at 04:51:07PM +0300, Felipe Balbi wrote:
> > That said, I cannot spot an obvious fail,
> 
> okay, but a fail does exist. Any hints on what extra information I could
> capture to help figuring this one out?

More information on which sleep is not waking woudl help I suppose. That
greatly limits the amount of code one has to stare at.

Also, a better picture of all threads involved in the wakeup. Alan seems
to suggest there's multiple CPUs involved in the wakeup.

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

#1475646

On Sat, 3 Sep 2016, Peter Zijlstra wrote:

> On Sat, Sep 03, 2016 at 09:58:09AM +0300, Felipe Balbi wrote:
> 
> > > What arch are you seeing this on?
> > 
> > x86. Skylake to be exact.
> 
> So it _cannot_ be the thing Alan mentioned. By the simple fact that
> spin_lock() is a full barrier on x86 (every LOCK prefixed instruction
> is).

True, my guess was wrong.

> > The following change survived through the night:
> > 
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> > index 8f3659b65f53..d31581dd5ce5 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
> >  /* Caller must hold fsg->lock */
> >  static void wakeup_thread(struct fsg_common *common)
> >  {
> > -	smp_wmb();	/* ensure the write of bh->state is complete */
> > +	smp_mb();	/* ensure the write of bh->state is complete */
> >  	/* Tell the main thread that something has happened */
> >  	common->thread_wakeup_needed = 1;
> >  	if (common->thread_task)
> > @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool can_freeze)
> >  	}
> >  	__set_current_state(TASK_RUNNING);
> >  	common->thread_wakeup_needed = 0;
> > -	smp_rmb();	/* ensure the latest bh->state is visible */
> > +	smp_mb();	/* ensure the latest bh->state is visible */
> >  	return rc;
> >  }
> 
> Sorry, but that is horrible code. A barrier cannot ensure writes are
> 'complete', at best they can ensure order between writes (or reads
> etc..).

The code is better than the comment.  What I really meant was that the 
write of bh->state needs to be visible to the thread after it wakes up 
(or after it checks the wakeup condition and skips going to sleep).

> Also, looking at that thing, that common->thread_wakeup_needed variable
> is 100% redundant. All sleep_thread() invocations are inside a loop of
> sorts and basically wait for other conditions to become true.
> 
> For example:
> 
> 	while (bh->state != BUF_STATE_EMPTY) {
> 		rc = sleep_thread(common, false);
> 		if (rc)
> 			return rc;
> 	}
> 
> All you care about there is bh->state, _not_
> common->thread_wakeup_needed.

You know, I never went through and verified that _all_ the invocations 
of sleep_thread() are like that.  In fact, I wrote the sleep/wakeup 
routines _before_ the rest of the code, and I didn't know in advance 
exactly how they were going to be called.

> That said, I cannot spot an obvious fail, but the code can certainly use
> help.

The problem may be that when the thread wakes up (or skips going to 
sleep), it needs to see more than just bh->state.  Those other values 
it needs are not written by the same CPU that calls wakeup_thread(), 
and so to ensure that they are visible that smp_wmb() really ought to 
be smp_mb() (and correspondingly in the thread.  That's what Felipe has 
been testing.

Alan Stern

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

#1476180

On Sat, Sep 03, 2016 at 10:16:31AM -0400, Alan Stern wrote:

> > Sorry, but that is horrible code. A barrier cannot ensure writes are
> > 'complete', at best they can ensure order between writes (or reads
> > etc..).
> 
> The code is better than the comment.  What I really meant was that the 
> write of bh->state needs to be visible to the thread after it wakes up 
> (or after it checks the wakeup condition and skips going to sleep).

Yeah, I got that.

> > Also, looking at that thing, that common->thread_wakeup_needed variable
> > is 100% redundant. All sleep_thread() invocations are inside a loop of
> > sorts and basically wait for other conditions to become true.
> > 
> > For example:
> > 
> > 	while (bh->state != BUF_STATE_EMPTY) {
> > 		rc = sleep_thread(common, false);
> > 		if (rc)
> > 			return rc;
> > 	}
> > 
> > All you care about there is bh->state, _not_
> > common->thread_wakeup_needed.
> 
> You know, I never went through and verified that _all_ the invocations 
> of sleep_thread() are like that. 

Well, thing is, they're all inside a loop which checks other conditions
for forward progress. Therefore the loop inside sleep_thread() is
pointless. Even if you were to return early, you'd simply loop in the
outer loop and go back to sleep again.

> In fact, I wrote the sleep/wakeup 
> routines _before_ the rest of the code, and I didn't know in advance 
> exactly how they were going to be called.

Still seems strange to me, why not use wait-queues for the first cut?

Only if you find a performance issue with wait-queues, which cannot be
fixed in the wait-queue proper, then do you do custom thingies.

Starting with a custom sleeper, just doesn't make sense to me.

> > That said, I cannot spot an obvious fail, but the code can certainly use
> > help.
> 
> The problem may be that when the thread wakes up (or skips going to 
> sleep), it needs to see more than just bh->state.  Those other values 
> it needs are not written by the same CPU that calls wakeup_thread(), 
> and so to ensure that they are visible that smp_wmb() really ought to 
> be smp_mb() (and correspondingly in the thread.  That's what Felipe has 
> been testing.

So you're saying something like:


	CPU0		CPU1		CPU2

	X = 1				sleep_thread()
			wakeup_thread()
					r = X

But how does CPU1 know to do the wakeup? That is, how are CPU0 and CPU1
coupled.

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

#1476515

On Mon, 5 Sep 2016, Peter Zijlstra wrote:

> > You know, I never went through and verified that _all_ the invocations 
> > of sleep_thread() are like that. 
> 
> Well, thing is, they're all inside a loop which checks other conditions
> for forward progress. Therefore the loop inside sleep_thread() is
> pointless. Even if you were to return early, you'd simply loop in the
> outer loop and go back to sleep again.
> 
> > In fact, I wrote the sleep/wakeup 
> > routines _before_ the rest of the code, and I didn't know in advance 
> > exactly how they were going to be called.
> 
> Still seems strange to me, why not use wait-queues for the first cut?
> 
> Only if you find a performance issue with wait-queues, which cannot be
> fixed in the wait-queue proper, then do you do custom thingies.
> 
> Starting with a custom sleeper, just doesn't make sense to me.

I really don't remember.  Felipe says that the ancient history shows
the initial implementation did use a wait-queue, and then it was
changed.  Perhaps I was imitating the structure of
scsi_error_handler().

> > The problem may be that when the thread wakes up (or skips going to 
> > sleep), it needs to see more than just bh->state.  Those other values 
> > it needs are not written by the same CPU that calls wakeup_thread(), 
> > and so to ensure that they are visible that smp_wmb() really ought to 
> > be smp_mb() (and correspondingly in the thread.  That's what Felipe has 
> > been testing.
> 
> So you're saying something like:
> 
> 
> 	CPU0		CPU1		CPU2
> 
> 	X = 1				sleep_thread()
> 			wakeup_thread()
> 					r = X
> 
> But how does CPU1 know to do the wakeup? That is, how are CPU0 and CPU1
> coupled.

As mentioned later on, "CPU0" is actually a DMA master, not another 
CPU.

Alan Stern

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

#1475482

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.

[toc] | [prev] | [standalone]

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

Back to top | Article view | linux.kernel

csiph-web