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
