Re: [RFC] usbfs2 aio cancellation code

>   It's making my
> head hurt, so I'd appreciate some code review.

That's what AIO does :).

> The in-kernel aio patches that Zack is working will ensure that the

(sekrit tip: Zac*k* Brown: did KT, now at Google.  Zac*h* Brown: me.   
Aren't you glad we both ended up in the same field?  Someone is  
having a laugh.)

> retry function and the cancel function will run serially.  If the  
> cancel
> function returns 0, the iocb will be destroyed.  If cancel returns  
> zero,
> it guarantees that the iocb will never be referenced again,  
> specifically
> by making sure that kick_iocb() and aio_complete() have never been and
> never will be called.

Yeah.  Think of it like reference counting.  By being called in the  
aio_ file operation methods you're being given a reference on the  
iocb.  Four things drop that reference:

  - *not* returning magical aio errnos from the aio_ functions
  - not returning magical aio errnos from ki_retry (after kick_iocb())
  - calling aio_complete()
  - returning 0 from ki_cancel

A provider of aio ops can do only one of these to drop their iocb  
reference.

> Some USB rules apply here too.  To cancel a USB transaction, you can
> call either usb_kill_urb() or usb_unlink_urb().  usb_kill_urb() will
> wait for the URB callback to complete, but it cannot be called with a
> spin lock held.  usb_unlink_urb() guarantees that the URB callback  
> will
> be called sometime in the future with an URB status that indicates it
> was cancelled.  usb_unlink_urb() will return -EBUSY if the URB  
> callback
> was about to run or was in the middle of running.

What happens if you call usb_unlink_urb() after the URB callback has  
been called?

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.

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.

> I *think* I can follow the new aio rules, if the aio core provides a
> spinlock for the iocb priv pointer.

It wouldn't do that.  If you needed a spinlock you could put one in  
the struct that you hung off of iocb->private.

>   Here's how it would work:
>
> cancel
> 	ret = 0;
> 	lock and disable interrupts
> 	/* Check to see if the URB has been freed yet.
> 	 * If not, attempt to cancel the URB;
> 	 * fail if the URB callback has already started.
> 	 * We know that read_retry can't run while cancel is running. */
> 	if(iocb->priv == NULL ||
> 			usb_unlink_urb(iocb->priv->urb) == -EBUSY)
> 		ret = -EBUSY;
> 	unlock
> 	return ret

Hmm.  If usb_unlink_urb() returns 0 then operation is still in  
flight, right?  It'll be completed soon by the URB callback, but that  
hasn't happened yet.

So I don't think returning 0 from ki_cancel is right.  The iocb is  
still in flight.  The operation could still be DMAing.  I think we  
only want to return 0 once the operation is well and truly done.   
That's why sys_io_cancel() provides an event if it returns 0.

If all usb_unlink_urb() does is accelerate completion, perhaps now  
with an error, that's fine.  Just always return -EBUSY from  
ki_cancel.  To the app it'll look like their cancelation attempt  
raced with completion, as it might have done anyway.  You're still  
honoring the intent of the sys_io_cancel() call.

> Does the cancel function follow the new aio cancel rules (i.e. it
> doesn't return 0 if kick_iocb() or aio_complete() has been called or
> will be called)?  Are there any races?  I can't see any, but I've been
> banging my head against this a lot this weekend.

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();

copy_read_result:
	if (error)
		return error'
	copy_to_user();
	return bytes_copied;

ki_retry:
	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.

> p.s. Zack - did you mean to do something with the return value from  
> the
> cancel function?  AFAIK, it simply checks that it's nonzero.

I think it was wrong for the previous implementation to pass the  
errno directly to userspace.  I think userspace would be right to be  
awfully confused if it got, say, EINVAL from ki_cancel instead of  
from the parts of sys_io_cancel() that return EINVAL. We just map an  
error from the ki_cancel function into a consistent errno which just  
says that the operation is still pending.

- z

-------------------------------------------------------------------------
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