Message ID | 1346084480-7994-1-git-send-email-sandmann@cs.au.dk |
---|---|
State | New |
Headers | show |
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
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.
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
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
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(-)