Xen devel - Nov 2005 - First post-xenbus-change USB patch

Here''s the first snapshot of the USB code after updating to the new
xenbus.

The xenidc code provides a much higher level of abstraction than the new
xenbus code so the changes didn''t affect the usb driver much.

I had to put in empty frontend_changed and backend_changed functions
because the new xenbus code segfaults without them.  The USB code
doesn''t need them because the entire state machine and all associated
ring/shared page setup is factored out into the xenidc_endpoint object
and the driver code gets connect and disconnect callbacks from the
endpoint instead.

I''d like the otherend_changed entrypoint to be optional please.  This
will also let me remove the MixedCaps XenbusState typename from my
code :-)

I took the opportunity to implement suspend and resume in the frontend
(and the backend to keep the code equivalent) though this is currently
untested.

I did a quick bit of testing and managed to mount the USB key again etc
but had a few problems with module unload and reload.  I think I maybe
got hit by the unregister_watch deadlock then, after I killed the FE
domain, the new xenbus code gave an error message when I tried to unload
and reload the BE.

I think there must have been some stale state in the store which was
confusing the state machine in the new xenbus code.

I''m using this state machine
http://lists.xensource.com/archives/html/xen-devel/2005-10/msg01199.html
in the xenidc endpoint code.  It is designed to be robust against stale
state left in the store and was working nicely for module load and
unload.

There were a couple of small wins for my code: I could use the otherend
and otherend_id fields from the xenbus_device instead of having my own
and I was able to remove the xenbus_gather from the FE since it doesn''t
need any configuration parameters other than the otherend and
otherend_id fields.

The BE still needs to read the USB path so I couldn''t remove the gather
from the BE.

Thanks for your offer of help BTW.  Just saw it as I''m writing this :-)

Harry.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Nov 15, 2005 at 03:23:21PM +0000, harry wrote:
> I''d like the otherend_changed entrypoint to be optional please.
No problem.  I''m sure you could whizz up a patch for us.
> I did a quick bit of testing and managed to mount the USB key again etc
> but had a few problems with module unload and reload.  I think I maybe
> got hit by the unregister_watch deadlock then
Yes, we really need to address that one.  The code says 

	/* Flush any currently-executing callback, unless we are it. :-) */

(IIRC, that''s the deadlock point that Rusty highlighted) but I
don''t really
know what this means.  Can you shed some light here on what we ought to be
doing?
> after I killed the FE domain, the new xenbus code gave an error message
when
> I tried to unload and reload the BE.
Which error message?

Cheers,

Ewan.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 15 Nov 2005, at 15:37, Ewan Mellor wrote:
>> I did a quick bit of testing and managed to mount the USB key again 
>> etc
>> but had a few problems with module unload and reload.  I think I maybe
>> got hit by the unregister_watch deadlock then
>
> Yes, we really need to address that one.  The code says
>
> 	/* Flush any currently-executing callback, unless we are it. :-) */
>
> (IIRC, that''s the deadlock point that Rusty highlighted) but I
don''t
> really
> know what this means.  Can you shed some light here on what we ought 
> to be
> doing?
I''d love to see the xenwatch_mutex disappear -- it could well be 
involved in a deadlock situation. It is only used to ensure that an 
unregistered watch is not still executing when 
unregister_xenbus_watch() returns, but that is kind of important if 
e.g., module code is about to get blown away. :-)

I''ll have a think about it...

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 15 Nov 2005, at 16:12, Keir Fraser wrote:
>> Yes, we really need to address that one.  The code says
>>
>> 	/* Flush any currently-executing callback, unless we are it. :-) */
>>
>> (IIRC, that''s the deadlock point that Rusty highlighted) but I
don''t
>> really
>> know what this means.  Can you shed some light here on what we ought 
>> to be
>> doing?
>
> I''d love to see the xenwatch_mutex disappear -- it could well be 
> involved in a deadlock situation. It is only used to ensure that an 
> unregistered watch is not still executing when 
> unregister_xenbus_watch() returns, but that is kind of important if 
> e.g., module code is about to get blown away. :-)
>
> I''ll have a think about it...
Actually I now remember that this is a hard one to avoid. Callers will 
often be interested in unregister being synchronous (so they can safely 
unload module code, free the xenbus_watch, or whatever) but if they 
ever call unregister_xenbus_watch() with a resource held (e.g., a 
mutex) that the watch callback also may acquire, then we have 
possibility of a deadlock. And there are various slightly hidden 
mutexes in sysfs.... :-(

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel