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

Groups > linux.kernel > #1475645

Re: Memory barrier needed with wake_up_process()?

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

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

csiph-web