[Spice-devel] [PATCH 28/35] vdi port: redesign.

[Alon Levy]

----- "Gerd Hoffmann" <kraxel at redhat.com> wrote:

> > To check this I'll have to compile the vdi_port driver, since back
> > porting the spice_vmc.c file from qemu-kvm-rhel6 would be too much
> > work (there is not virtio-serial in current qemu used in upstrea
> > spice). But I'll try to do it and send the patch over.
> 
> qemu on freedesktop.org has both vdi-port and spice-vmc drivers.  With
> 
> vdi-port not being intented to be submitted upstream, but it is useful
> 
> to have for the time being as the spice-vmc windows bits are not ready
> yet.
> 
> > Actually, the movement of write to the plug doesn't look right -
> the
> > interface is basically a table that is filled by the user, in this
> > case qemu.
> 
> Which plug?
> 
> Data structures we have (for all interface types):
> 
>    (1) Implementation -- static data for the (qemu) implementation of
>        the interface (the function pointers).
>    (2) Instance -- data for the actual instance of the interface.
> 
> The whole idea is best explained with qxl where you can actually have
> 
> multiple instances:  There is a single qxl implementation, and there
> is 
> one instance per qxl device (i.e. for the multihead case you have 
> multiple instances).
> 
> The instance is used everythere to refer to the interface:  libspice 
> passes it when calling the interface callbacks.  qemu passes it when 
> calling spice_server_$interface_$action() functions.  It holds a
> pointer 
> to the implementation, a pointer to the private libspice data and 
> anything else needed (qxl id for example).
> 
> 
> Oh, and I think the current wakeup + read/write callback scheme has it
> 
> advantages.  Especially spice-server can call read() whenever it is 
> ready to accept data.  With the data rates we have today spice-server
> 
> will grab the data instantly after wakeup, but that will change once
> we 
> xfer bulky clipboard data.
> 

I'm not sure how that should be done. My main dislike of the current interface
is that it forces either a copy from the driver to an intermediate, like
the code I had checked in, or it forces spice-server to consume everything it gets.
I think the later is still a better option, combined with throttling. This has
nothing to do with interface change, but it suggests it because if you agree that
making another copy on the way of the data is redundant, then having an interface
that lets the consumer decide how much bytes it wants to get every time doesn't fit.

I see two solutions - one is to leave it as is, return the copy in the middle, put
in the throttling like you have, throttle on have data, unthrottle when all data
is consumed.
 + no changes in API
 - additional copy of the data.

The other is to have a havedata callback instead of read, called from driver,
allow the havedata to return a boolean telling it if to throttle or not, and
have another "unthrottle" function called from spice.
 - change of API
 + no additional copy of data

(Just a note: we don't have to limit the amount of data we get in a single havedata callback
because virtio-serial already has a limit on it for us (I think it's 4096 bytes,
the size of a single buffer in the virtqueue.)

The later option means that if we want to throttle we keep the latest data (we
have to, other option is to throw it away, won't work), but there is already
a ring that does exactly that in spice-server.

> cheers,
>    Gerd