On Wed, 7 Dec 2011, Sarah Sharp wrote:
> > Here's one way to do it. Have two new flags: one for reset-in-progress
> > and one for abort-in-progress. If the reset-in-progress flag is set,
> > then uas_do_work and uas_stat_cmplt would know not to submit any URBs
> > at all. If that flag isn't set but the other one is, they would know
> > to check whether the particular command they are working on at the
> > moment is being aborted.
> >
> > Then all you need is some way to tell when all the outstanding commands
> > have completed. For example, an atomic counter of outstanding
> > commands. When the counter goes to 0, if the reset-in-progress flag is
> > set then you signal the pre-reset thread that it may proceed (use a
> > struct completion or something like that).
>
> Ok, I've tried to wrap my head around this strategy all day yesterday,
> and I can't see how it can be done without races.
What races?
> I feel like I'm going
> in mental circles, and missing information about how the SCSI layer
> works.
What information?
> It would be best if you wrote this patch yourself.
>
> There's nothing strictly speaking wrong with the original patchset,
> aside from the race condition between the abort handler and
> uas_stat_cmplt, is there?
I don't know; I didn't look at the patches in any detail.
> You've expressed that you think it's a bit
> heavy-handed, because it kills the commands on reset, but Oliver has
> also expressed the opinion that when another driver wants a reset on a
> composite device, it often means there's something wrong with the other
> interface.
>
> If we find out that it is an improvement to flush rather than kill
> commands on reset, that patch can always be added later. The patchset
> as it was submitted is an improvement over the current code, since it
> adds device error handling where there was none before.
Sure. Killing everything at once is okay for now. Don't worry about
that.
> The simplest solution (and the one I understand) to make the original
> patchset race-free is to add an aborting flag to the uas_command_info
> structure, make the abort handler set it before it takes the work queue
> lock, and rearrange the function that is called by uas_stat_cmplt such
> that it takes the uas_work_lock before submitting URBs and checks the
> flag:
>
> 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, srcu_idx;
>
> srcu_idx = srcu_read_lock(devinfo->srcu);
> if (atomic_read(&devinfo->resetting)) {
> srcu_read_unlock(devinfo->srcu, srcu_idx);
> return -ENODEV;
> }
>
> cmdinfo->state = direction | SUBMIT_STATUS_URB;
> spin_lock(&uas_work_lock);
> /* XXX: check the aborting flag here. */
> err = uas_submit_urbs(cmnd, cmnd->device->hostdata, GFP_ATOMIC);
> if (err) {
> list_add_tail(&cmdinfo->list, &uas_work_list);
> spin_unlock(&uas_work_lock);
> schedule_work(&uas_work);
> } else {
> spin_unlock(&uas_work_lock);
> }
> srcu_read_unlock(devinfo->srcu, srcu_idx);
> return 0;
> }
>
> That does mean that the USB 2.0 UAS fast path will have to take a lock
> on every submit, which is not ideal, but usb-storage also takes a mutex,
> so it's a wash. I'm not convinced the USB 2.0 UAS device will get much
> better performance than BoT anyway. If performance testing shows that
> the spinlock is the bottleneck, I can re-look at the solution.
Aborting is tricky. You have to synchronize it with both URB
submission and command completion. SRCU can take care of the URB
submission part, but for command completion you'll need something
stronger. The race to worry about is what happens when the error
handler gets invoked at the same time as the command's last URB
completes and you call scsi_done. Maybe this race is harmless for uas,
I don't know. But if you don't want to risk trying to abort a command
that has already completed, it may well turn out that a spinlock
is the only way to handle it.
> The good news is that the USB 3.0 UAS fast path does not include
> uas_xfer_data, so it will only see the RCU readlock. USB 3.0 UAS is
> where I expect the performance of the code to matter the most.
>
> So is there anything horrendously objectionable about closing the
> abort/resubmit race condition with a spinlock and using the RCU
> implementation from the original patch?
What you should do is break the problem into two pieces. Work on
aborting first, and then add resetting. I suspect aborting may be more
complex then resetting.
For aborting, the races to worry about are abort/resubmit and
abort/complete. As mentioned above, abort/resubmit could be handled
via SRCU but abort/complete may need some sort of lock.
For resetting, the races to worry about are reset/queuecommand and
waiting for all outstanding commands to complete. The queuecommand
race could be handled with either regular RCU or a spinlock. Either a
private lock or the host lock (you know, uas_queuecommand doesn't
have to be called with the host lock held if you don't want it).
Waiting for the outstanding commands to complete can be done without
locking if you don't mind wasting a little time in pre_reset. All you
have to do is test an atomic commands_in_flight counter, and if it is
non-zero then sleep for a few tens of milliseconds and try again.
Does that make things clearer?
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
