Re: g_mass_storage not queueing requests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2 Sep 2016, Felipe Balbi wrote:

> >> I just noticed that the kerneldoc for wake_up_process() says that the 
> >> caller should assume a write memory barrier if and only if any tasks 
> >> are woken up.  So if the task is already running, there is no barrier.
> >>
> >> This means that in f_mass_storage's wakeup_thread(), there may be a 
> >> race involving wake_up_process() reading the thread's state and the 
> >> write to common->thread_wakeup_needed.
> >>
> >> 	wakeup_thread():
> >> 		common->thread_wakeup_needed = 1;
> >> 		wake_up_process(common->thread_task);
> >> 			/* reads the thread's state */
> >>
> >> 	sleep_thread():
> >> 		set_current_state(TASK_INTERRUPTIBLE);
> >> 		if (common->thread_wakeup_needed)
> >> 			break;
> >>
> >> Now, set_current_state() implicitly has a memory barrier at the end.  
> >> But since wake_up_process() doesn't always add a barrier the start, 
> >> we may need to do this explicitly.
> >>
> >> Without that barrier, the CPU might execute the read in
> >> wake_up_process() before setting thread_wakeup_needed to 1.  Then
> >> sleep_thread() would slip through the crack, and the thread wouldn't
> >> run.
> >>
> >> You can try adding smp_mb() in wakeup_thread(), just before the call to 
> >> wake_up_process().
> >
> > heh, it would've taken me weeks to figure this out :-) thanks
> >
> > BTW, what are those barriers protecting in g_mass_storage? Its own
> > internal flags or the task state?

I'm not sure which barriers you're referring to; there's a number of
them.  But I think they are all intended to protect the driver's
internal variables, not the task state.

> > If it's protecting own flags, then isn't the following enough?
> >
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> > index 8f3659b65f53..e3b03decdb6b 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)
> >
> > I'm probably missing some detail here :-s
> 
> oh no, you're trying to make sure the write to
> common->thread_wakeup_needed is seen by sleep_thread(), right?

Exactly.

> In that case, couldn't we conditionally add smp_mb() based on the result
> of wake_up_process()? IOW:
> 
> 	if (!wake_up_process(common->thread_task))
>         	smp_mb();
> 
> would this still work?

No, that would be too late.  The barrier needs to go between the write 
to common->thead_wakeup_needed and the call to wake_up_process().  

Otherwise what I wrote above could happen: The CPU could read the task
state (in wake_up_process()) before it writes out the new value for
thread_wakeup_needed, and sleep_thread() could run on another CPU
during that tiny window.  wake_up_process() would see that the thread's
state was still TASK_RUNNING, so it wouldn't do anything, and
sleep_thread() would see that thread_wakeup_needed was still 0, so it
would put the thread to sleep.

Now, I can't tell if this is really what caused your problem.  Still,
it seems like a necessary thing to do.  Maybe I'll check this with Paul
McKenney.

(IMO, the default version of wake_up_process() should have this memory
barrier built-in.  Otherwise races like this are too hard to track
down.  There could be a different version without the barrier, say
__wake_up_process(), for places where it's known to be unnecessary.  
That's how set_current_state() and __set_current_state() are
implemented.)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux