Re: [RFC] usbfs2 aio cancellation code

On Tue, Oct 02, 2007 at 03:07:03PM -0400, Alan Stern wrote:
> On Tue, 2 Oct 2007, Zach Brown wrote:
> 
> > > Do things really need to be this complicated?
> >
> > Do you think the reply I sent to Sarah safely relies on usb_unlink_urb
> > ()?  I don't know if it provides the guarantees which that proposal
> > would rely on.
>  
> The guarantees it provides aren't exactly what Sarah wrote, but
> they are close.
>  
> > What happens if you call usb_unlink_urb() after the URB callback has  
> > been called?
> 
> Then usb_unlink_urb() fails.  But it might not return -EBUSY; it might
> return some other error code instead.

Hmm, I thought I'd traced it correctly...  usb_unlink_urb in core/usb.c
calls usb_hcd_unlink_urb in core/hcd.c, which returns -EBUSY if the URB
status is anything but -EINPROGRESS.  (Assuming the URB still exists and
none of the other checks are triggered.)  Did I just misread it, or will
something further up the stack mess with the return value?

> > I'm hoping that we can *just* call usb_unlink_urb() from ki_cancel  
> > and *always* return -EBUSY.  The URB callback will either be called  
> > very soon on error or will already be in flight or have been called.
> 
> Right, there's no point in having ki_cancel ever complete an aio 
> operation.  The operation should complete when the URB completes, 
> whether it succeeded, was unlinked, or encountered some other error.

So then there's no point in using usb_kill_urb() in the cancel function,
correct?  The cancellation function doesn't care about waiting for the
URB callback to run, it just cares that it queued up the request for it
to be canceled.

> > The trick there is to make sure that the URB callback doesn't free  
> > resources that ki_cancel needs to be able to call usb_unlink_urb() to  
> > find that it's already been called.  I would propose putting a struct  
> > on iocb->private which is freed in the ki_dtor method.  It's called  
> > once all the iocb references have been dropped.

Sounds good.  If I had known the ki_dtor method existed, I would have
proposed a different patch. :)

> > I got lost in the pseudo code for testing for cancelation and  
> > things.  How about this:
> > 
> > aio_ method:
> > 	/* this does not race with ki_cancel */
> > 	iocb->private = alloc_urb_goo();
> > 	iocb->ki_dtor = free_urb_goo();
> > 	queue URB callback
> > 	if (read) {
> > 		iocb->ki_retry = copy_read_result();
> > 		return -EIOCBRETRY:
> > 	} else
> > 		return -EIOCBQUEUED:
> > 
> > URB callback:
> > 	/* usb_unlink_urb() protects the race with ki_cancel */
> > 	/* this cannot race with ki_retry, it triggers it */
> > 	if (read)
> > 		kick_iocb();
> > 	else
> > 		aio_complete();

Do we want to test for cancel in the callback function?  It seems
pointless to call with the kick_iocb() when there's no data to copy.

> > copy_read_result:
> > 	if (error)
> > 		return error'
> > 	copy_to_user();
> > 	return bytes_copied;
> > 
> > ki_retry:
> 
> You mean ki_cancel:
> 
> > 	some_usb_goo = iocb->private;
> > 	/* usb_unlink_urb() protects us from races */
> > 	/* urb goo won't be freed until ki_dtor, which we hold a reference  
> > for */
> > 	usb_unlink_urb(some_usb_goo);
> > 	return -EBUSY;
> > 
> > ki_dtor:
> > 	free(iocb->private);
> > 
> > That should work, presuming that usb_unlink_urb() has strong internal  
> > serialization to have calls from ki_cancel fail if the callback is  
> > pending or has been completed already.
> 
> Yes, I think this will work.

Thanks for looking at this.  I like this simplier solution much better.

Sarah

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel