Re: [Qemu-devel] [PATCH v2] qxl: async I/O

[Gerd Hoffmann <kraxel@redhat.com>]

> +void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
> +                           struct QXLRect *area, struct QXLRect *dirty_rects,
> +                           uint32_t num_dirty_rects, uint32_t clear_dirty_region,
> +                           int async)
> +{
> +    if (async) {
> +        qxl->ssd.worker->update_area_async(qxl->ssd.worker, surface_id, area, dirty_rects,
> +                                 num_dirty_rects, clear_dirty_region, 0);

Fails to build with older libspice.

> +    } else {
> +        qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, dirty_rects,
> +                                 num_dirty_rects, clear_dirty_region);
> +    }
> +}
>
>   void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
>                              struct QXLRect *area, struct QXLRect *dirty_rects,
>                              uint32_t num_dirty_rects, uint32_t clear_dirty_region)
>   {
> -    qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, dirty_rects,
> -                             num_dirty_rects, clear_dirty_region);
> +    qxl_spice_update_area_async(qxl, surface_id, area, dirty_rects,
> +                                num_dirty_rects, clear_dirty_region, 0);
>   }

Pretty pointless wrapper IMHO.

> -void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id)
> +static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl)
>   {
>       qemu_mutex_lock(&qxl->track_lock);
> -    PANIC_ON(id>= NUM_SURFACES);
> -    qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
> -    qxl->guest_surfaces.cmds[id] = 0;
> +    qxl->guest_surfaces.cmds[qxl->io_data.surface_id] = 0;

I'd suggest to pass in the surface id as argument instead.

>       qxl->guest_surfaces.count--;
>       qemu_mutex_unlock(&qxl->track_lock);
>   }
>
> +static void qxl_spice_destroy_surface_wait_async(PCIQXLDevice *qxl, uint32_t id, int async)
> +{
> +    qxl->io_data.surface_id = id;
> +    if (async) {
> +        qxl->ssd.worker->destroy_surface_wait_async(qxl->ssd.worker, id, 0);
> +    } else {
> +        qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
> +        qxl_spice_destroy_surface_wait_complete(qxl);

qxl_spice_destroy_surface_wait_complete(qxl, id);

> +    }
> +}
> +
>   void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
>                                  uint32_t count)
>   {
> @@ -171,15 +193,29 @@ void qxl_spice_reset_memslots(PCIQXLDevice *qxl)
>       qxl->ssd.worker->reset_memslots(qxl->ssd.worker);
>   }
>
> -void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
> +static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
>   {
>       qemu_mutex_lock(&qxl->track_lock);
> -    qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
>       memset(&qxl->guest_surfaces.cmds, 0, sizeof(qxl->guest_surfaces.cmds));
>       qxl->guest_surfaces.count = 0;
>       qemu_mutex_unlock(&qxl->track_lock);
>   }
>
> +static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
> +{
> +    qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
> +    qxl_spice_destroy_surfaces_complete(qxl);
> +}
> +
> +static void qxl_spice_destroy_surfaces_async(PCIQXLDevice *qxl, int async)
> +{
> +    if (async) {
> +        qxl->ssd.worker->destroy_surfaces_async(qxl->ssd.worker, 0);
> +    } else {
> +        qxl_spice_destroy_surfaces(qxl);
> +    }
> +}

I'd combine those into one function simliar to 
qxl_spice_destroy_surface_wait_async (and we don't need the _async 
suffix if we have a single version only which gets passed in async as 
argument).

> +
>   void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
>   {
>       qxl->ssd.worker->reset_image_cache(qxl->ssd.worker);
> @@ -706,6 +742,38 @@ static int interface_flush_resources(QXLInstance *sin)
>       return ret;
>   }
>
> +static void qxl_add_memslot_complete(PCIQXLDevice *d);
> +static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
> +
> +/* called from spice server thread context only */
> +static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    uint32_t current_async;
> +
> +    qemu_mutex_lock(&qxl->async_lock);
> +    current_async = qxl->current_async;
> +    qxl->current_async = QXL_UNDEFINED_IO;
> +    qemu_mutex_unlock(&qxl->async_lock);

I'd tend to use the cookie to pass that information (also the stuff in 
io_data).

> -static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta)
> +static void qxl_add_memslot_complete(PCIQXLDevice *d)

I think it isn't needed to move that to the completion callback.  Memory 
slots can be created and destroyed with I/O commands only, so there is 
no need to care about the ordering like we have to with surfaces.

>       qemu_mutex_init(&qxl->track_lock);
> +    qemu_mutex_init(&qxl->async_lock);

Do we really need two locks?
When passing info via cookie, doesn't the need for the async lock go 
away completely?

> index af10ae8..b7bc0de 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -62,6 +62,20 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
>       dest->right = MAX(dest->right, r->right);
>   }
>
> +int qemu_spice_supports_async(SimpleSpiceDisplay *ssd)
> +{
> +    return (ssd->worker->major_version>  3 ||
> +            (ssd->worker->major_version == 3&&  ssd->worker->minor_version>= 1));
> +}

Doing a runtime check here is pointless, just use
#if SPICE_INTERFACE_QXL_MINOR >= 1
...
#endif

>   void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
>                                          QXLDevSurfaceCreate *surface)
>   {
>       ssd->worker->create_primary_surface(ssd->worker, id, surface);
>   }
>
> +void qemu_spice_destroy_primary_surface_async(SimpleSpiceDisplay *ssd, uint32_t id, int async)
> +{
> +    if (async) {
> +        ssd->worker->destroy_primary_surface_async(ssd->worker, id, 0);
> +    } else {
> +        qemu_spice_destroy_primary_surface(ssd, id);
> +    }
> +}

Like for all others: one only please.

cheers,
   Gerd