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

[Juan Quintela <quintela@redhat.com>]

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.