[Qemu-devel] [PATCH 1/2] spice: Change NUM_SURFACES to 4096

[Qemu-devel] [PATCH 1/2] spice: Change NUM_SURFACES to 4096

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

It's not uncommon for an X workload to have more than 1024 pixmaps
live at the same time. Ideally, there wouldn't be any fixed limit like
this, but since we have one, increase it to 4096.
---
 ui/spice-display.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ui/spice-display.h b/ui/spice-display.h
index 12e50b6..e8d01a5 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -32,7 +32,7 @@
 #define MEMSLOT_GROUP_GUEST 1
 #define NUM_MEMSLOTS_GROUPS 2
 
-#define NUM_SURFACES 1024
+#define NUM_SURFACES 4096
 
 /*
  * Internal enum to differenciate between options for
-- 
1.7.4

Re: [Qemu-devel] [PATCH 1/2] spice: Change NUM_SURFACES to 4096

[Gerd Hoffmann]

On 08/27/12 18:21, Søren Sandmann wrote:
> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> It's not uncommon for an X workload to have more than 1024 pixmaps
> live at the same time. Ideally, there wouldn't be any fixed limit like
> this, but since we have one, increase it to 4096.
> ---
>  ui/spice-display.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index 12e50b6..e8d01a5 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -32,7 +32,7 @@
>  #define MEMSLOT_GROUP_GUEST 1
>  #define NUM_MEMSLOTS_GROUPS 2
>  
> -#define NUM_SURFACES 1024
> +#define NUM_SURFACES 4096

Breaks live migration.

First, we must make this runtime-configurable.  rev3 qxl devices should
continue to operate with 1024 surfaces for compatibility with older qemu
versions.

Second the vmstate must be adapted to handle this.  The number of
surfaces is in the migration data stream, so this should be doable
without too much trouble.  Right now it looks like this:

        [ ... ]
        VMSTATE_INT32_EQUAL(num_surfaces, PCIQXLDevice),
        VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
                      vmstate_info_uint64, uint64_t),
        [ ... ]

Juan?  Suggestions how to handle this?  There seems to be no direct way
to make the array size depend on num_surfaces.  I think we could have
two VMSTATE_ARRAY_TEST() entries, one for 1024 and one for 4096.

Better ideas?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] spice: Change NUM_SURFACES to 4096

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 08/27/12 18:21, Søren Sandmann wrote:
>> From: Søren Sandmann Pedersen <ssp@redhat.com>
>> 
>> It's not uncommon for an X workload to have more than 1024 pixmaps
>> live at the same time. Ideally, there wouldn't be any fixed limit like
>> this, but since we have one, increase it to 4096.
>> ---
>>  ui/spice-display.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/ui/spice-display.h b/ui/spice-display.h
>> index 12e50b6..e8d01a5 100644
>> --- a/ui/spice-display.h
>> +++ b/ui/spice-display.h
>> @@ -32,7 +32,7 @@
>>  #define MEMSLOT_GROUP_GUEST 1
>>  #define NUM_MEMSLOTS_GROUPS 2
>>  
>> -#define NUM_SURFACES 1024
>> +#define NUM_SURFACES 4096
>
> Breaks live migration.

Live migcation always on the middle :-()

> Second the vmstate must be adapted to handle this.  The number of
> surfaces is in the migration data stream, so this should be doable
> without too much trouble.  Right now it looks like this:
>
>         [ ... ]
>         VMSTATE_INT32_EQUAL(num_surfaces, PCIQXLDevice),
>         VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
>                       vmstate_info_uint64, uint64_t),
>         [ ... ]
>
> Juan?  Suggestions how to handle this?  There seems to be no direct way
> to make the array size depend on num_surfaces.  I think we could have
> two VMSTATE_ARRAY_TEST() entries, one for 1024 and one for 4096.

I would left things as they are, and just add a new section for the
rest of the surfaces.  If we are always going to have _more_ than 1024
surfaces, the easier solution that I can think of is:

       * move guest_surfaces.cmds to a pointer (now, it is runtime configurable)

        /* notice removal of _EQUAL */
        VMSTATE_INT32(num_surfaces, PCIQXLDevice),
        /* move from ARRAY to VARRAY with sive on num_surfaces */
        VMSTATE_VARRAY_INT32(guest_surfaces.cmds, PCIQXLDevice, num_surfaces, 0,
                       vmstate_info_uint64, uint64_t),

And thinking about it, no subsection is needed.  if num_surfaces is
1024, things can migrate to old qemu.  if it is bigger, it would break
migration with good reason (num_surfaces has changed).

The VMSTATE_INT32_EQUAL() will break (on the incoming side) of migration
if we are migrating with a numbef of surfaces != 1024.

What do you think?

Later, Juan.

Re: [Qemu-devel] [PATCH 1/2] spice: Change NUM_SURFACES to 4096

[Gerd Hoffmann]

  Hi,

>         /* move from ARRAY to VARRAY with sive on num_surfaces */
>         VMSTATE_VARRAY_INT32(guest_surfaces.cmds, PCIQXLDevice, num_surfaces, 0,
>                        vmstate_info_uint64, uint64_t),

Ah.  Yes, VARRAY will do, somehow I didn't find it.

> And thinking about it, no subsection is needed.  if num_surfaces is
> 1024, things can migrate to old qemu.  if it is bigger, it would break
> migration with good reason (num_surfaces has changed).

Indeed.  /me is glad that I was careful enougth to stick num_surfaces
into the migration data stream ;)

thanks,
  Gerd