On Mon, Dec 05, 2011 at 09:15:13AM -0800, Paul E. McKenney wrote:
> On Fri, Dec 02, 2011 at 11:55:58AM -0800, Sarah Sharp wrote:
> Looks good, especially given that it is a first posting!
>
> A few questions and comments interspersed below.
Thanks for the review!
> > -static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
> > - unsigned direction)
> > +static int uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
> > + struct uas_dev_info *devinfo, unsigned direction)
> > {
> > struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> > - int err;
> > + int err, srcu_idx;
> > +
> > + srcu_idx = srcu_read_lock(devinfo->srcu);
> > + if (atomic_read(&devinfo->resetting)) {
>
> As Alan notes, atomic_read() doesn't do much of anything atomic for
> you. I suggest using a bare int, which the compiler will properly
> align for you. Then use ACCESS_ONCE() when reading or updating it,
> for example:
>
> if (ACCESS_ONCE(devinfo->resetting)) {
>
> or:
>
> ACCESS_ONCE(devinfo->resetting) = new_value;
>
> The ACCESS_ONCE macro uses a volatile cast to force the compiler to
> suppress optimizations that might otherwise result in invented loads
> and other compiler mischief.
Oh, right, I was using an atomic_t to avoiding the compiler optimizing
the resetting flag away, not because of memory barrier semantics. But I
can certainly change that to use the ACCESS_ONCE macro.
>
> > + srcu_read_unlock(devinfo->srcu, srcu_idx);
> > + return -ENODEV;
> > + }
> >
> > cmdinfo->state = direction | SUBMIT_STATUS_URB;
> > err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
> > @@ -260,6 +270,8 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
> > spin_unlock(&uas_work_lock);
> > schedule_work(&uas_work);
>
> The function scheduled by schedule_work() will -not- be covered by the
> SRCU read-side critical section. Just checking to make sure you are
> not assuming that it is covered.
That's fine. What I want out of the synchronize SRCU in the uas_preset
method is the guarantee that the readers have seen (or will see) the
resetting flag, and thus won't attempt to schedule the work queue again.
Once the readers are synchronized, I cancel the work queue. From what
I've read about the cancel_work_sync() call, by the time it returns, the
work queue function will have finished, and it will have been prevented
from rescheduling itself:
/**
* cancel_work_sync - cancel a work and wait for it to finish
* @work: the work to cancel
*
* Cancel @work and wait for its execution to finish. This function
* can be used even if the work re-queues itself or migrates to
* another workqueue. On return from this function, @work is
* guaranteed to be not pending or executing on any CPU.
*
* cancel_work_sync(&delayed_work->work) must not be used for
* delayed_work's. Use cancel_delayed_work_sync() instead.
*
* The caller must ensure that the workqueue on which @work was last
* queued can't be destroyed before this function returns.
*
* RETURNS:
* %true if @work was pending, %false otherwise.
*/
bool cancel_work_sync(struct work_struct *work)
{
return __cancel_work_timer(work, NULL);
}
EXPORT_SYMBOL_GPL(cancel_work_sync);
Then uas_preset can remove/kill any partially submitted commands in the
workqueue, and kill any commands that have been fully submitted.
If cancel_work_sync() doesn't work like my mental model, then, yes, the
work queue also needs to be an SRCU reader, so that it will not
reschedule itself when the resetting flag is set.
> > @@ -904,6 +935,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > if (!devinfo)
> > return -ENOMEM;
> >
> > + atomic_set(&devinfo->resetting, 0);
> > result = -ENOMEM;
> > shost = scsi_host_alloc(&uas_host_template, sizeof(void *));
> > if (!shost)
> > @@ -923,15 +955,23 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > for (i = 0; i < devinfo->qdepth; i++)
> > init_usb_anchor(&devinfo->anchors[i]);
> >
> > + devinfo->srcu = kmalloc(sizeof(*devinfo->srcu), GFP_KERNEL);
>
> I take it that there is a size constraint that prevents embedding the
> struct srcu_struct in the devinfo structure? The srcu_struct is an
> int, a pointer, and a struct mutex (assuming CONFIG_PROVE_LOCKING=n).
>
> In the absence of a size constraint, embedding would of course be simpler.
Yes, there is a size limit on devinfo. It overrides the SCSI host
pointer, so it can't be bigger than that. There wasn't enough room for
a USB anchor to be embedded in the structure, so (to be honest) I just
didn't try embedding the SRCU structure either. I'd rather keep some
space in the structure in case it needs to grow more state.
> > static int uas_pre_reset(struct usb_interface *intf)
> > {
> > + int i;
> > + struct usb_device *udev;
> > + struct Scsi_Host *shost;
> > + struct uas_dev_info *devinfo;
> > + struct uas_cmd_info *cmdinfo;
> > + struct uas_cmd_info *temp;
> > + struct usb_host_endpoint *eps[3];
> > +
> > /* XXX: Need to return 1 if it's not our device in error handling */
> > + udev = interface_to_usbdev(intf);
> > + shost = (struct Scsi_Host *) usb_get_intfdata(intf);
> > + devinfo = (struct uas_dev_info *) shost->hostdata[0];
> > +
> > + /* Synchronize with the readers of the resetting flag.
> > + *
> > + * Any currently running readers may submit URBs or schedule the
> > + * workqueue, but any future readers will see the flag and return
> > + * immediately. Thus, when synchronize_srcu() finishes, we can stop the
> > + * workqueue safely, and flush any pending commands.
> > + *
> > + * synchronize_srcu() will run an implicit memory barrier on all CPUs,
> > + * so we're guaranteed future readers will see the flag when they grab
> > + * the srcu_read_lock(). We only need to make sure the compiler doesn't
> > + * optimize away the flag setting away by using atomic_t (which will do
> > + * the same volatile trick as ACCESS_ONCE()).
> > + */
> > + atomic_set(&devinfo->resetting, 1);
> > + synchronize_srcu(devinfo->srcu);
>
> This waits until all pre-existing readers complete.
Right.
> > +
> > + /* Stop the workqueue, and stop it from rescheduling itself. */
> > + cancel_work_sync(&uas_work);
>
> And if this gets rid of any workqueue items that might have been scheduled
> by pre-existing readers, then I believe you are OK.
I believe it will stop the work queue from being rescheduled by itself,
as I said above. The next bit of code removes the items from the
workqueue list.
> > + /* Remove any partially queued commands from the work queue */
> > + list_for_each_entry_safe(cmdinfo, temp, &uas_work_list, list) {
> > + struct scsi_pointer *scp = (void *)cmdinfo;
> > + struct scsi_cmnd *cmnd = container_of(scp,
> > + struct scsi_cmnd, SCp);
> > + if (blk_rq_tagged(cmnd->request))
> > + uas_kill_tagged_urbs(
> > + &devinfo->anchors[cmnd->request->tag],
> > + cmdinfo);
> > + else
> > + uas_kill_tagged_urbs(
> > + &devinfo->anchors[0],
> > + cmdinfo);
> > + list_del_init(&cmdinfo->list);
> > + }
> > +
> > + /* Kill any commands that were fully queued. */
> > + for (i = 0; i < devinfo->qdepth; i++)
> > + usb_kill_anchored_urbs(&devinfo->anchors[i]);
> > +
> > + /* Free streams. */
> > + eps[0] = usb_pipe_endpoint(udev, devinfo->status_pipe);
> > + eps[1] = usb_pipe_endpoint(udev, devinfo->data_in_pipe);
> > + eps[2] = usb_pipe_endpoint(udev, devinfo->data_out_pipe);
> > + usb_free_streams(intf, eps, 3, GFP_KERNEL);
> > +
> > return 0;
> > }
> >
> > static int uas_post_reset(struct usb_interface *intf)
> > {
> > + struct Scsi_Host *shost;
> > + struct uas_dev_info *devinfo;
> > + int qdepth;
> > +
> > + shost = (struct Scsi_Host *) usb_get_intfdata(intf);
> > + devinfo = (struct uas_dev_info *) shost->hostdata[0];
> > /* XXX: Need to return 1 if it's not our device in error handling */
> > + /* Set the correct alt setting and re-allocate streams.
> > + * The USB core will disconnect and re-probe if the descriptors
> > + * (including the SS endpoint companion descriptors) change.
> > + * So qdepth should be the same, unless the xHCI driver failed to
> > + * allocate streams. Disconnect and re-probe in that case.
> > + */
> > + qdepth = devinfo->qdepth;
> > + uas_configure_endpoints(devinfo);
> > + if (qdepth != devinfo->qdepth)
> > + return -ENODEV;
> > +
> > + /* Make sure that the CPU doesn't reorder writes such that clearing the
> > + * reset flag happens before setting up the streams
> > + */
> > + smp_wmb();
> > + atomic_set(&devinfo->resetting, 0);
> > + /* Make sure that all future readers will see the cleared flag before
> > + * returning from post-reset. Otherwise the SCSI core could schedule a
> > + * command queue on a separate CPU where the flag might be stale.
> > + */
> > + synchronize_srcu(devinfo->srcu);
>
> Do we also need to handle workqueue items that were scheduled
> by pre-existing readers?
Once uas_prereset returns, the workqueue will be empty and all commands
will have been killed.
> (My guess is that the checks I see in
> uas_xfer_data() will have prevented pre-existing readers from scheduling
> workqueue items, but just in case there are a few schedule_work() calls
> that aren't visible in the patch...)
No, you didn't miss any. All instances of schedule_work() are covered
by the SRCU reader lock, except the one in the workqueue function,
uas_do_work().
> If the answer to this question is "no", please expand the comment to
> document the answer.
What sort of expansion do you want? A note that the workqueue will have
been stopped by pre-reset and thus doesn't need to be a reader?
Sarah Sharp
--
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
