Re: [Xen-devel] USB Xen Summit status summary

[Anthony Liguori <aliguori@xxxxxxxxxx>]

Harry Butterworth wrote:

> Would someone please explain why people thought the xenidc code is
> unlikely to get accepted into mainline?
It's the sheer volume of code.  The size of your xenidc patch is on par 
with early versions of the hypervisor in terms of SLOCs.
Abstractions without a clear need for the abstraction is heavily frowned 
upon in Linux.  It's just way too much code without a compelling 
justification for what it's needed.
Regards,

Anthony Liguori

>  It seems to me that Linux has a
> common network stack for the different network card drivers and a common
> USB stack for the different USB devices---I don't see why Xen shouldn't
> have a common inter-domain communication stack for the different virtual
> xen devices.  Is it that there are style issues remaining?  Or perhaps
> it's the problem of better integrating the code with the current Xen
> infrastructure.
>
> I did spend a week or so trying to work out what the best way to
> integrate was and came to the conclusion that the best bet (with minimum
> impact to ongoing development) was to leave the existing xen drivers and
> API in place and write a new set of drivers to the new API on the side
> making the choice of old or new driver selectable in the Kconfig.  After
> the new drivers were stable it would be possible to remove the old ones
> and then remove the old API and coalesce the infrastructure code.  The
> other approach of attempting to make atomic incremental changes to the
> existing API and all the drivers is just going to destabilise the tree
> and impact out-of-tree development too much.
>
>  
>
> > - API code is orthogonal to USB driver piece, should be
> >   a seperate patch/discussion [consensus]
> >    
> I needed a stable API to code the driver to which is why I did both
> together.  Now the native Xen API is more stable I'm doing a version of
> the USB driver with the xenidc code factored out.
>
>  
>
> > - Best option is (2), rewrite code to leaner, simpler
> >   USB driver with minimal functionality, and get that into
> >   tree
> >    
> Factoring out the xenidc code just means duplicating the function it
> provided in the usbback and usbfront directories, replacing the xenidc
> channel code with a version constructed using the ring macros and making
> use of Ewan's state change info to drive connect/disconnect.
>
> Whilst the sum of xenidc and the USB driver code will be reduced I'm not
> sure that the USB driver itself is actually going to get that much
> smaller with the low-level comms code put back into it.
>
>  
>
> > - Noone in session had looked at USBoverIp patches
> > - There were some good ideas in the IDC API that needed
> >   to be discussed/incorporated in Xen
> >
> > Other input/questions received:
> > - Need to get USB community input
> >    
> I have had some input on the design which has been helpful. I posted a
> code snippet too which led to a discussion about changing Linux to make
> it possible to bind to devices rather than interfaces.
>
>  
>
> > - What were the issues that were left? Are they resolved?
> >   If so, what's the current working state of the patch?
> >    
> No, I have wasted too much time on the Xen APIs to have got around to
> the remaining issues yet.  In particular, a correct solution to the
> problem of stalling and retrying URBs during error recovery looks fairly
> tricky.  Also changes are required in Linux to allow binding to a USB
> device rather than an interface so as to allow the correct handling of
> devices with multiple configurations.
>
> Having said this, I don't think the remaining USB issues should be the
> thing that stops the driver from going into the tree.  My intention was
> always just to get the basic function working and then let it get
> improved afterwards.  The problem has been syncing up with the xen
> driver infrastructure.
>
> The current state of the patch is that when I last posted it it was
> working for my test USB devices (USB key, keyboard) against the then
> current unstable tree.  Since then the unstable tree has moved on---the
> xenbus API has changed slightly, all the header files have been moved
> and the Kconfig file has been split up---the version of the driver that
> I'm factoring xenidc out of compiles but is currently untested.
>
>  
>
> > - Keir: rewrite to a simpler driver without the IDC API
> >   as the xenbus/store stuff is pretty baked into Xen now,
> >    
> The xenbus/xenstore APIs are pretty bad from the device driver side.  It
> would be a shame to have to live with them indefinitely.
>
> In particular, I think that the following issues need to be addressed:
>
> 1) The RING macros and interrupt handlers for inter-domain messages are
> too difficult to use. The interrupt handler code to service the ring
> queue correctly is pretty subtle and it's totally unnecessary to have
> this functionality duplicated in each driver.
>
> 2) The low-level memory manipulation in drivers is unnecessary.  Linux
> has DMA APIs to do DMA.  Xen should have equivalent high-level APIs for
> inter-domain bulk data transport.  I pointed this out at the first Xen
> summit and Christian claimed it was impossible because each driver
> required specific performance optimisations but I think the xenidc code
> demonstrates that it is actually quite easy to achieve without having to
> sacrifice performance.
>
> 3) Marshalling parameters from xenstore.  This wasn't addressed by
> xenidc but it's quite clear that the driver-side xenstore API is not a
> good match for the C programming language.  On device create, it would
> be much easier to get parameters in a struct.  If it's necessary to
> register an XML definition of the struct expected that would be OK.
>
> 4) Sequencing operations using xenstore. This is a real problem because
> of the level-sensitive nature of watches on the store.  From the
> driver's perspective if there's going to be a hot configuration change,
> the easiest interface to deal with is receiving the reconfiguration
> request and all the parameters in one operation in a struct.  Again,
> having to marshall the parameters individually and then work out which
> ones have changed and what configuration request that implies is just
> too painful.
>
> 5) The xenbus otherend_changed state machine API.  This API is largely a
> result of having half of the communication channel implementation in
> xenbus and the other half in the drivers.  Xenidc demonstrates a better
> channel implementation and a better driver API here.
>
> 6) I'm not sure that the function in the xenbus state machine which
> drives a clean shutdown from the back-end closing is useful because the
> FE can just flush if it wants to do a clean shutdown.  On the other
> hand, for multi-pathing device drivers it's more normal for FE's to have
> to deal cleanly with unclean termination of a channel BE which isn't
> catered for in the xenbus state machine.
>
> 7) Exposing the implementation details of everything in xenstore isn't
> good from the point of view of creating a well defined API for third
> parties.  I get the impression this is being addressed by using XML-RPC
> to define a public interface to xend which I think is probably a good
> thing.
>
>  
>
> >   might want to do some cleanups in this area.
> > - Ian: look at USBoverIP, tried it and it seems to
> >   work, but not sure if that's the right solution
> >
> > Current Issues/Design questions:
> > - Harry's code supports back/front module load/unload
> >   (useful during development, if nothing else).
> >    
> Without working driver domains I was finding module unload of the
> back-end useful because it saves a 5 min reboot.  Driver domains are not
> quite as useful as module unload because you have to reboot both the FE
> and BE domains whereas with module unload you can leave the FE set up.
>
>  
>
> > - Harry's code is not written to Ewan's last common
> >   code pullout API
> > - What other code functionality can be dropped in order
> >   to make it smaller?
> >    
> A fair amount of code comes from doing pre-allocation of resources and
> resource management.  This means that once the module is initialised it
> has all the resources required to guarantee progress is made with normal
> operation.  If I drop all the resource management stuff and just do
> allocations on the I/O path and allow I/O failures due to ENOMEM then I
> can make the code simpler.
>
> I think the current bounded approach is better though.
>
>  
>
> > [All misrepresentations and errors are mine, I'm operating
> >  from memory and on occasion what I heard over the crowd noise :)]
> >
> > Hope that initiates the necessary conversation on this...
> >    
> Thanks Niv, very useful.
>
>  
>
> > thanks,
> > Nivedita
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-devel
> >
> >    
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>
>  

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