[PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[George Dunlap]

This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.

The original commit fixes a bug when assigning a large number of
devices which require option roms to a guest.  (One known
configuration that needs extra memory is having more than 4 emulated
NICs assigned.  Three or fewer NICs seems to work without this
functionality.)

However, by unilaterally increasing maxmem, it introduces two
problems.

First, now libxl's calculation of the required maxmem during migration
is broken -- any guest which exercised this functionality will fail on
migration.  (Guests which have the default number of devices are not
affected.)

Secondly, it makes it impossible for a higher-level toolstack or
administer to predict how much memory a VM will actually use, making
it much more difficult to effectively use all of the memory on a
machine.

The right solution to the original problem is to figure out a way for
qemu to take pages from the existing pool of guest memory, rather than
allocating more pages.

That fix will take more time to develop than we have until the feature
freeze.  In the mean time, the simplest way to fix the migration issue
is to revert this change.  That will re-introduce the original bug,
but it's an unusual corner case; and without migration it isn't fully
functional yet anyway.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
I do think this is the right approach, but I'm mainly sending this is
mainly to open up discussion.

CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen-hvm.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/xen-hvm.c b/xen-hvm.c
index 42356b8..0408462 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -87,12 +87,6 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
 #endif
 
 #define BUFFER_IO_MAX_DELAY  100
-/* Leave some slack so that hvmloader does not complain about lack of
- * memory at boot time ("Could not allocate order=0 extent").
- * Once hvmloader is modified to cope with that situation without
- * printing warning messages, QEMU_SPARE_PAGES can be removed.
- */
-#define QEMU_SPARE_PAGES 16
 
 typedef struct XenPhysmap {
     hwaddr start_addr;
@@ -250,8 +244,6 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
     unsigned long nr_pfn;
     xen_pfn_t *pfn_list;
     int i;
-    xc_domaininfo_t info;
-    unsigned long free_pages;
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         /* RAM already populated in Xen */
@@ -274,22 +266,6 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
         pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i;
     }
 
-    if ((xc_domain_getinfolist(xen_xc, xen_domid, 1, &info) != 1) ||
-        (info.domain != xen_domid)) {
-        hw_error("xc_domain_getinfolist failed");
-    }
-    free_pages = info.max_pages - info.tot_pages;
-    if (free_pages > QEMU_SPARE_PAGES) {
-        free_pages -= QEMU_SPARE_PAGES;
-    } else {
-        free_pages = 0;
-    }
-    if ((free_pages < nr_pfn) &&
-        (xc_domain_setmaxmem(xen_xc, xen_domid,
-                             ((info.max_pages + nr_pfn - free_pages)
-                              << (XC_PAGE_SHIFT - 10))) < 0)) {
-        hw_error("xc_domain_setmaxmem failed");
-    }
     if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
         hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr);
     }
-- 
1.9.1

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[George Dunlap]

On 06/10/2015 01:55 PM, George Dunlap wrote:
> This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> 
> The original commit fixes a bug when assigning a large number of
> devices which require option roms to a guest.  (One known
> configuration that needs extra memory is having more than 4 emulated
> NICs assigned.  Three or fewer NICs seems to work without this
> functionality.)
> 
> However, by unilaterally increasing maxmem, it introduces two
> problems.
> 
> First, now libxl's calculation of the required maxmem during migration
> is broken -- any guest which exercised this functionality will fail on
> migration.  (Guests which have the default number of devices are not
> affected.)

Just to make it clear what the situation is (to the best of my knowledge):

QEMU 2.2 and before:
 * A VM assigned more than 3 NICs would fail during qemu start-up
 * A VM assigned 3 or fewer NICs can be created and migrated successfully.

QEMU 2.3 (most recent release):
 * A VM assigned more than 3 NICs can be created successfully, but not
migrated afterwards
 * A VM assigned 3 or fewer NICs can be both created and migrated.
(Stefano has done a few tests to verify this and it seems to be accurate.)

It's unlikely that the "proper fix" descibed in this mail will be ready
for 2.4, so if this patch is accepted, 2.4 will look like 2.2.

 -George

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Ian Campbell]

On Wed, 2015-06-10 at 14:21 +0100, George Dunlap wrote:
> On 06/10/2015 01:55 PM, George Dunlap wrote:
> > This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> > 
> > The original commit fixes a bug when assigning a large number of
> > devices which require option roms to a guest.  (One known
> > configuration that needs extra memory is having more than 4 emulated
> > NICs assigned.  Three or fewer NICs seems to work without this
> > functionality.)
> > 
> > However, by unilaterally increasing maxmem, it introduces two
> > problems.
> > 
> > First, now libxl's calculation of the required maxmem during migration
> > is broken -- any guest which exercised this functionality will fail on
> > migration.  (Guests which have the default number of devices are not
> > affected.)
> 
> Just to make it clear what the situation is (to the best of my knowledge):
> 
> QEMU 2.2 and before:
>  * A VM assigned more than 3 NICs would fail during qemu start-up
>  * A VM assigned 3 or fewer NICs can be created and migrated successfully.
> 
> QEMU 2.3 (most recent release):
>  * A VM assigned more than 3 NICs can be created successfully, but not
> migrated afterwards
>  * A VM assigned 3 or fewer NICs can be both created and migrated.
> (Stefano has done a few tests to verify this and it seems to be accurate.)
> 
> It's unlikely that the "proper fix" descibed in this mail will be ready
> for 2.4, so if this patch is accepted, 2.4 will look like 2.2.

FWIW I think reverting would be the right thing to do.

I think we should also revert some of the changes to libxl which tried
to cope with the qemu 2.2 behaviour.

Ian.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Wei Liu]

On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> 
> The original commit fixes a bug when assigning a large number of
> devices which require option roms to a guest.  (One known
> configuration that needs extra memory is having more than 4 emulated
> NICs assigned.  Three or fewer NICs seems to work without this
> functionality.)
> 
> However, by unilaterally increasing maxmem, it introduces two
> problems.
> 
> First, now libxl's calculation of the required maxmem during migration
> is broken -- any guest which exercised this functionality will fail on
> migration.  (Guests which have the default number of devices are not
> affected.)
> 
> Secondly, it makes it impossible for a higher-level toolstack or
> administer to predict how much memory a VM will actually use, making
> it much more difficult to effectively use all of the memory on a
> machine.
> 
> The right solution to the original problem is to figure out a way for
> qemu to take pages from the existing pool of guest memory, rather than
> allocating more pages.
> 
> That fix will take more time to develop than we have until the feature
> freeze.  In the mean time, the simplest way to fix the migration issue
> is to revert this change.  That will re-introduce the original bug,
> but it's an unusual corner case; and without migration it isn't fully
> functional yet anyway.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> I do think this is the right approach, but I'm mainly sending this is
> mainly to open up discussion.
> 
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>

Stefano, Andrew, any comments?

If we're to do this we need to do it now.

I think reverting this change in QEMU and relevant changes in libxl
would be the most viable solution to solve this for this release.

Wei.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Stefano Stabellini]

On Tue, 16 Jun 2015, Wei Liu wrote:
> On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> > This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> > 
> > The original commit fixes a bug when assigning a large number of
> > devices which require option roms to a guest.  (One known
> > configuration that needs extra memory is having more than 4 emulated
> > NICs assigned.  Three or fewer NICs seems to work without this
> > functionality.)
> > 
> > However, by unilaterally increasing maxmem, it introduces two
> > problems.
> > 
> > First, now libxl's calculation of the required maxmem during migration
> > is broken -- any guest which exercised this functionality will fail on
> > migration.  (Guests which have the default number of devices are not
> > affected.)
> > 
> > Secondly, it makes it impossible for a higher-level toolstack or
> > administer to predict how much memory a VM will actually use, making
> > it much more difficult to effectively use all of the memory on a
> > machine.
> > 
> > The right solution to the original problem is to figure out a way for
> > qemu to take pages from the existing pool of guest memory, rather than
> > allocating more pages.
> > 
> > That fix will take more time to develop than we have until the feature
> > freeze.  In the mean time, the simplest way to fix the migration issue
> > is to revert this change.  That will re-introduce the original bug,
> > but it's an unusual corner case; and without migration it isn't fully
> > functional yet anyway.
> > 
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > ---
> > I do think this is the right approach, but I'm mainly sending this is
> > mainly to open up discussion.
> > 
> > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Ian Campbell <ian.campbell@citrix.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Stefano, Andrew, any comments?
> 
> If we're to do this we need to do it now.
> 
> I think reverting this change in QEMU and relevant changes in libxl
> would be the most viable solution to solve this for this release.

Reverting this patch doesn't really solve the problem: instead of
breaking on migration when the VM has more than 3 emulated NICs, the VM
simply refuses to start in that case. I guess it can be considered a
small improvement but certainly not a fix.

Given that the migration issue only happens in an "unusual corner case",
are we really in a hurry to revert this commit and go back to the
failure to start, even before we actually figure out what the proper fix
is?

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Stefano Stabellini]

On Tue, 16 Jun 2015, Stefano Stabellini wrote:
> On Tue, 16 Jun 2015, Wei Liu wrote:
> > On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> > > This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> > > 
> > > The original commit fixes a bug when assigning a large number of
> > > devices which require option roms to a guest.  (One known
> > > configuration that needs extra memory is having more than 4 emulated
> > > NICs assigned.  Three or fewer NICs seems to work without this
> > > functionality.)
> > > 
> > > However, by unilaterally increasing maxmem, it introduces two
> > > problems.
> > > 
> > > First, now libxl's calculation of the required maxmem during migration
> > > is broken -- any guest which exercised this functionality will fail on
> > > migration.  (Guests which have the default number of devices are not
> > > affected.)
> > > 
> > > Secondly, it makes it impossible for a higher-level toolstack or
> > > administer to predict how much memory a VM will actually use, making
> > > it much more difficult to effectively use all of the memory on a
> > > machine.
> > > 
> > > The right solution to the original problem is to figure out a way for
> > > qemu to take pages from the existing pool of guest memory, rather than
> > > allocating more pages.
> > > 
> > > That fix will take more time to develop than we have until the feature
> > > freeze.  In the mean time, the simplest way to fix the migration issue
> > > is to revert this change.  That will re-introduce the original bug,
> > > but it's an unusual corner case; and without migration it isn't fully
> > > functional yet anyway.
> > > 
> > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > ---
> > > I do think this is the right approach, but I'm mainly sending this is
> > > mainly to open up discussion.
> > > 
> > > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > > CC: Wei Liu <wei.liu2@citrix.com>
> > > CC: Ian Campbell <ian.campbell@citrix.com>
> > > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > 
> > Stefano, Andrew, any comments?
> > 
> > If we're to do this we need to do it now.
> > 
> > I think reverting this change in QEMU and relevant changes in libxl
> > would be the most viable solution to solve this for this release.
> 
> Reverting this patch doesn't really solve the problem: instead of
> breaking on migration when the VM has more than 3 emulated NICs, the VM
> simply refuses to start in that case. I guess it can be considered a
> small improvement but certainly not a fix.
> 
> Given that the migration issue only happens in an "unusual corner case",
> are we really in a hurry to revert this commit and go back to the
> failure to start, even before we actually figure out what the proper fix
> is?
 
I think it would be more useful to add a check to libxl to explicitly
refuse to create any guests with more than 3 emulated nics.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[George Dunlap]

On 06/16/2015 04:51 PM, Stefano Stabellini wrote:
> On Tue, 16 Jun 2015, Wei Liu wrote:
>> On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
>>> This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
>>>
>>> The original commit fixes a bug when assigning a large number of
>>> devices which require option roms to a guest.  (One known
>>> configuration that needs extra memory is having more than 4 emulated
>>> NICs assigned.  Three or fewer NICs seems to work without this
>>> functionality.)
>>>
>>> However, by unilaterally increasing maxmem, it introduces two
>>> problems.
>>>
>>> First, now libxl's calculation of the required maxmem during migration
>>> is broken -- any guest which exercised this functionality will fail on
>>> migration.  (Guests which have the default number of devices are not
>>> affected.)
>>>
>>> Secondly, it makes it impossible for a higher-level toolstack or
>>> administer to predict how much memory a VM will actually use, making
>>> it much more difficult to effectively use all of the memory on a
>>> machine.
>>>
>>> The right solution to the original problem is to figure out a way for
>>> qemu to take pages from the existing pool of guest memory, rather than
>>> allocating more pages.
>>>
>>> That fix will take more time to develop than we have until the feature
>>> freeze.  In the mean time, the simplest way to fix the migration issue
>>> is to revert this change.  That will re-introduce the original bug,
>>> but it's an unusual corner case; and without migration it isn't fully
>>> functional yet anyway.
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> ---
>>> I do think this is the right approach, but I'm mainly sending this is
>>> mainly to open up discussion.
>>>
>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Ian Campbell <ian.campbell@citrix.com>
>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> Stefano, Andrew, any comments?
>>
>> If we're to do this we need to do it now.
>>
>> I think reverting this change in QEMU and relevant changes in libxl
>> would be the most viable solution to solve this for this release.
> 
> Reverting this patch doesn't really solve the problem: instead of
> breaking on migration when the VM has more than 3 emulated NICs, the VM
> simply refuses to start in that case. I guess it can be considered a
> small improvement but certainly not a fix.
> 
> Given that the migration issue only happens in an "unusual corner case",
> are we really in a hurry to revert this commit and go back to the
> failure to start, even before we actually figure out what the proper fix
> is?

I'm in a hurry to go back to a world where qemu doesn't unexpectedly
allocate more RAM to a guest.  If the real problem with the original
patch was that it broke migration, we could fix that pretty easily; but
the real problem (to me) with the original patch is that it
unpredicatably allocates more memory to a guest that the toolstack
doesn't know about.

 -George

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Stefano Stabellini]

On Tue, 16 Jun 2015, George Dunlap wrote:
> On 06/16/2015 04:51 PM, Stefano Stabellini wrote:
> > On Tue, 16 Jun 2015, Wei Liu wrote:
> >> On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> >>> This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> >>>
> >>> The original commit fixes a bug when assigning a large number of
> >>> devices which require option roms to a guest.  (One known
> >>> configuration that needs extra memory is having more than 4 emulated
> >>> NICs assigned.  Three or fewer NICs seems to work without this
> >>> functionality.)
> >>>
> >>> However, by unilaterally increasing maxmem, it introduces two
> >>> problems.
> >>>
> >>> First, now libxl's calculation of the required maxmem during migration
> >>> is broken -- any guest which exercised this functionality will fail on
> >>> migration.  (Guests which have the default number of devices are not
> >>> affected.)
> >>>
> >>> Secondly, it makes it impossible for a higher-level toolstack or
> >>> administer to predict how much memory a VM will actually use, making
> >>> it much more difficult to effectively use all of the memory on a
> >>> machine.
> >>>
> >>> The right solution to the original problem is to figure out a way for
> >>> qemu to take pages from the existing pool of guest memory, rather than
> >>> allocating more pages.
> >>>
> >>> That fix will take more time to develop than we have until the feature
> >>> freeze.  In the mean time, the simplest way to fix the migration issue
> >>> is to revert this change.  That will re-introduce the original bug,
> >>> but it's an unusual corner case; and without migration it isn't fully
> >>> functional yet anyway.
> >>>
> >>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >>> ---
> >>> I do think this is the right approach, but I'm mainly sending this is
> >>> mainly to open up discussion.
> >>>
> >>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Ian Campbell <ian.campbell@citrix.com>
> >>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >>
> >> Stefano, Andrew, any comments?
> >>
> >> If we're to do this we need to do it now.
> >>
> >> I think reverting this change in QEMU and relevant changes in libxl
> >> would be the most viable solution to solve this for this release.
> > 
> > Reverting this patch doesn't really solve the problem: instead of
> > breaking on migration when the VM has more than 3 emulated NICs, the VM
> > simply refuses to start in that case. I guess it can be considered a
> > small improvement but certainly not a fix.
> > 
> > Given that the migration issue only happens in an "unusual corner case",
> > are we really in a hurry to revert this commit and go back to the
> > failure to start, even before we actually figure out what the proper fix
> > is?
> 
> I'm in a hurry to go back to a world where qemu doesn't unexpectedly
> allocate more RAM to a guest.  If the real problem with the original
> patch was that it broke migration, we could fix that pretty easily; but
> the real problem (to me) with the original patch is that it
> unpredicatably allocates more memory to a guest that the toolstack
> doesn't know about.

I don't think that this is a big problem. But I'll proceed assuming it
is. I would like to have a plan, a design doc, to solve the underlying
issue. Once we have it, if it involves reverting this commit, we'll do
it, even before any new lines of code get written.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Ian Campbell]

On Tue, 2015-06-16 at 16:56 +0100, George Dunlap wrote:
> On 06/16/2015 04:51 PM, Stefano Stabellini wrote:
> > On Tue, 16 Jun 2015, Wei Liu wrote:
> >> On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> >>> This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> >>>
> >>> The original commit fixes a bug when assigning a large number of
> >>> devices which require option roms to a guest.  (One known
> >>> configuration that needs extra memory is having more than 4 emulated
> >>> NICs assigned.  Three or fewer NICs seems to work without this
> >>> functionality.)
> >>>
> >>> However, by unilaterally increasing maxmem, it introduces two
> >>> problems.
> >>>
> >>> First, now libxl's calculation of the required maxmem during migration
> >>> is broken -- any guest which exercised this functionality will fail on
> >>> migration.  (Guests which have the default number of devices are not
> >>> affected.)
> >>>
> >>> Secondly, it makes it impossible for a higher-level toolstack or
> >>> administer to predict how much memory a VM will actually use, making
> >>> it much more difficult to effectively use all of the memory on a
> >>> machine.
> >>>
> >>> The right solution to the original problem is to figure out a way for
> >>> qemu to take pages from the existing pool of guest memory, rather than
> >>> allocating more pages.
> >>>
> >>> That fix will take more time to develop than we have until the feature
> >>> freeze.  In the mean time, the simplest way to fix the migration issue
> >>> is to revert this change.  That will re-introduce the original bug,
> >>> but it's an unusual corner case; and without migration it isn't fully
> >>> functional yet anyway.
> >>>
> >>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >>> ---
> >>> I do think this is the right approach, but I'm mainly sending this is
> >>> mainly to open up discussion.
> >>>
> >>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Ian Campbell <ian.campbell@citrix.com>
> >>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >>
> >> Stefano, Andrew, any comments?
> >>
> >> If we're to do this we need to do it now.
> >>
> >> I think reverting this change in QEMU and relevant changes in libxl
> >> would be the most viable solution to solve this for this release.
> > 
> > Reverting this patch doesn't really solve the problem: instead of
> > breaking on migration when the VM has more than 3 emulated NICs, the VM
> > simply refuses to start in that case. I guess it can be considered a
> > small improvement but certainly not a fix.
> > 
> > Given that the migration issue only happens in an "unusual corner case",
> > are we really in a hurry to revert this commit and go back to the
> > failure to start, even before we actually figure out what the proper fix
> > is?
> 
> I'm in a hurry to go back to a world where qemu doesn't unexpectedly
> allocate more RAM to a guest.  If the real problem with the original
> patch was that it broke migration, we could fix that pretty easily; but
> the real problem (to me) with the original patch is that it
> unpredicatably allocates more memory to a guest that the toolstack
> doesn't know about.

Not only that but in trying to deal with this at least one race/bug has
been added in the libxl code. I suspect there are more.

Ian.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Ian Campbell]

On Tue, 2015-06-16 at 17:49 +0100, Stefano Stabellini wrote:
> On Tue, 16 Jun 2015, George Dunlap wrote:
> > On 06/16/2015 04:51 PM, Stefano Stabellini wrote:
> > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > >> On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> > >>> This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> > >>>
> > >>> The original commit fixes a bug when assigning a large number of
> > >>> devices which require option roms to a guest.  (One known
> > >>> configuration that needs extra memory is having more than 4 emulated
> > >>> NICs assigned.  Three or fewer NICs seems to work without this
> > >>> functionality.)
> > >>>
> > >>> However, by unilaterally increasing maxmem, it introduces two
> > >>> problems.
> > >>>
> > >>> First, now libxl's calculation of the required maxmem during migration
> > >>> is broken -- any guest which exercised this functionality will fail on
> > >>> migration.  (Guests which have the default number of devices are not
> > >>> affected.)
> > >>>
> > >>> Secondly, it makes it impossible for a higher-level toolstack or
> > >>> administer to predict how much memory a VM will actually use, making
> > >>> it much more difficult to effectively use all of the memory on a
> > >>> machine.
> > >>>
> > >>> The right solution to the original problem is to figure out a way for
> > >>> qemu to take pages from the existing pool of guest memory, rather than
> > >>> allocating more pages.
> > >>>
> > >>> That fix will take more time to develop than we have until the feature
> > >>> freeze.  In the mean time, the simplest way to fix the migration issue
> > >>> is to revert this change.  That will re-introduce the original bug,
> > >>> but it's an unusual corner case; and without migration it isn't fully
> > >>> functional yet anyway.
> > >>>
> > >>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > >>> ---
> > >>> I do think this is the right approach, but I'm mainly sending this is
> > >>> mainly to open up discussion.
> > >>>
> > >>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > >>> CC: Wei Liu <wei.liu2@citrix.com>
> > >>> CC: Ian Campbell <ian.campbell@citrix.com>
> > >>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > >>
> > >> Stefano, Andrew, any comments?
> > >>
> > >> If we're to do this we need to do it now.
> > >>
> > >> I think reverting this change in QEMU and relevant changes in libxl
> > >> would be the most viable solution to solve this for this release.
> > > 
> > > Reverting this patch doesn't really solve the problem: instead of
> > > breaking on migration when the VM has more than 3 emulated NICs, the VM
> > > simply refuses to start in that case. I guess it can be considered a
> > > small improvement but certainly not a fix.
> > > 
> > > Given that the migration issue only happens in an "unusual corner case",
> > > are we really in a hurry to revert this commit and go back to the
> > > failure to start, even before we actually figure out what the proper fix
> > > is?
> > 
> > I'm in a hurry to go back to a world where qemu doesn't unexpectedly
> > allocate more RAM to a guest.  If the real problem with the original
> > patch was that it broke migration, we could fix that pretty easily; but
> > the real problem (to me) with the original patch is that it
> > unpredicatably allocates more memory to a guest that the toolstack
> > doesn't know about.
> 
> I don't think that this is a big problem. But I'll proceed assuming it
> is. I would like to have a plan, a design doc, to solve the underlying
> issue. Once we have it, if it involves reverting this commit, we'll do
> it, even before any new lines of code get written.

I'm not inclined to wait for that to accept reverts to the libxl stuff
which built on top of this qemu change and added races to
libxl_set_memory.

Ian.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Wei Liu]

On Tue, Jun 16, 2015 at 04:54:45PM +0100, Stefano Stabellini wrote:
> On Tue, 16 Jun 2015, Stefano Stabellini wrote:
> > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > On Wed, Jun 10, 2015 at 01:55:13PM +0100, George Dunlap wrote:
> > > > This reverts commit c1d322e6048796296555dd36fdd102d7fa2f50bf.
> > > > 
> > > > The original commit fixes a bug when assigning a large number of
> > > > devices which require option roms to a guest.  (One known
> > > > configuration that needs extra memory is having more than 4 emulated
> > > > NICs assigned.  Three or fewer NICs seems to work without this
> > > > functionality.)
> > > > 
> > > > However, by unilaterally increasing maxmem, it introduces two
> > > > problems.
> > > > 
> > > > First, now libxl's calculation of the required maxmem during migration
> > > > is broken -- any guest which exercised this functionality will fail on
> > > > migration.  (Guests which have the default number of devices are not
> > > > affected.)
> > > > 
> > > > Secondly, it makes it impossible for a higher-level toolstack or
> > > > administer to predict how much memory a VM will actually use, making
> > > > it much more difficult to effectively use all of the memory on a
> > > > machine.
> > > > 
> > > > The right solution to the original problem is to figure out a way for
> > > > qemu to take pages from the existing pool of guest memory, rather than
> > > > allocating more pages.
> > > > 
> > > > That fix will take more time to develop than we have until the feature
> > > > freeze.  In the mean time, the simplest way to fix the migration issue
> > > > is to revert this change.  That will re-introduce the original bug,
> > > > but it's an unusual corner case; and without migration it isn't fully
> > > > functional yet anyway.
> > > > 
> > > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > > ---
> > > > I do think this is the right approach, but I'm mainly sending this is
> > > > mainly to open up discussion.
> > > > 
> > > > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > > > CC: Wei Liu <wei.liu2@citrix.com>
> > > > CC: Ian Campbell <ian.campbell@citrix.com>
> > > > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > > 
> > > Stefano, Andrew, any comments?
> > > 
> > > If we're to do this we need to do it now.
> > > 
> > > I think reverting this change in QEMU and relevant changes in libxl
> > > would be the most viable solution to solve this for this release.
> > 
> > Reverting this patch doesn't really solve the problem: instead of
> > breaking on migration when the VM has more than 3 emulated NICs, the VM
> > simply refuses to start in that case. I guess it can be considered a
> > small improvement but certainly not a fix.
> > 
> > Given that the migration issue only happens in an "unusual corner case",
> > are we really in a hurry to revert this commit and go back to the
> > failure to start, even before we actually figure out what the proper fix
> > is?
>  
> I think it would be more useful to add a check to libxl to explicitly
> refuse to create any guests with more than 3 emulated nics.

Providing proper error message in libxl instead of failing at start is
indeed an improvement. I can send a patch to do that.

This is orthogonal to the underlying issue though. The underlying issue
is how we would model guest memory allocation, that we (various
maintainers) haven't come to an agreement on this.

Since we have applied some libxl side patch for this currently
controversial model, we also need to determine whether we want to
release that piece code. That is something we can't wait.

In light of Ian's reasoning that libxl side itself has race, I would be
in favour of reverting that libxl patch. And since we need to revert
libxl side patch, we should really revert QEMU side patch as well,
otherwise we create trouble for our future selves when integrating libxl
and QEMU.

Wei.

Re: [PATCH] Revert "xen-hvm: increase maxmem before calling xc_domain_populate_physmap"

[Ian Campbell]

On Tue, 2015-06-16 at 16:51 +0100, Stefano Stabellini wrote:
> > I think reverting this change in QEMU and relevant changes in libxl
> > would be the most viable solution to solve this for this release.
> 
> Reverting this patch doesn't really solve the problem: instead of
> breaking on migration when the VM has more than 3 emulated NICs, the VM
> simply refuses to start in that case. I guess it can be considered a
> small improvement but certainly not a fix.
> 
> Given that the migration issue only happens in an "unusual corner case",
> are we really in a hurry to revert this commit and go back to the
> failure to start, even before we actually figure out what the proper fix
> is?

I don't care about migration (particularly). I care about the brokeness
in libxl which has been introduced while trying to cope with this QEMU
change.

We should revert now so that we can revert those libxl changes too and
get ourselves to a known state which doesn't have races in the memory
setting code.

It will also be much easier to move forward from a state where QEMU is
not messing with the memory limit for a domain in an uncoordinated way
and only libxl is doing so. Every QEMU release which we delay this
revert will just make it harder to find a path safe forward towards
whatever design we finally settle on.

Ian.