-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Thursday, July 24, 2014 5:00 PM To: Chen, Hanxiao/陈 晗霄; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2] LXC: add support for --config in setmaxmem command
On 16.07.2014 11:51, Chen Hanxiao wrote:
In lxc, we could not use setmaxmem command with --config options. This patch will add support for this.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/lxc/lxc_driver.c | 69 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 23 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b7b4b02..be6ee19 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -721,10 +721,10 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virLXCDomainObjPrivatePtr priv; virLXCDriverPtr driver = dom->conn->privateData; virLXCDriverConfigPtr cfg = NULL; - unsigned long oldmax = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_MEM_MAXIMUM, -1);
if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -743,32 +743,55 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, &persistentDef) < 0) goto cleanup;
- if (flags & VIR_DOMAIN_AFFECT_LIVE) - oldmax = vm->def->mem.max_balloon; - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!oldmax || oldmax > persistentDef->mem.max_balloon) - oldmax = persistentDef->mem.max_balloon; - } + if (flags & VIR_DOMAIN_MEM_MAXIMUM) { + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (newmem < vm->def->mem.cur_balloon) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Cannot resize the max memory less than current" + " memory for an active domain")); + goto cleanup; + } + vm->def->mem.max_balloon = newmem;
Are things that easy? Don't we need to communicate this with the lxc_controler somehow? Even though you allow only extending, I think unless we are 100% sure guest will see the resize, we shouldn't allow this.
I focused on '--config', so I kept what the original lxcDomainSetMaxMemory did. It looks that guest could not see the resize, and we shouldn't allow this.
+ }
- if (newmem > oldmax) { - virReportError(VIR_ERR_INVALID_ARG, - "%s", _("Cannot set memory higher than max memory")); - goto cleanup; - } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef);
Is this assert needed? Did clang complain or is this just a pure lefover from copying from qemu_driver.c?
+ persistentDef->mem.max_balloon = newmem; + if (persistentDef->mem.cur_balloon > newmem) + persistentDef->mem.cur_balloon = newmem; + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto cleanup; + } + } else { + unsigned long oldmax = 0;
- if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virCgroupSetMemory(priv->cgroup, newmem) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Failed to set memory for domain")); - goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_LIVE) + oldmax = vm->def->mem.max_balloon; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (!oldmax || oldmax > persistentDef->mem.max_balloon) + oldmax = persistentDef->mem.max_balloon; } - }
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - sa_assert(persistentDef);
Well, since it has been here already, I think we can leave it in your patch too.
- persistentDef->mem.cur_balloon = newmem; - if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + if (newmem > oldmax) { + virReportError(VIR_ERR_INVALID_ARG, + "%s", _("Cannot set memory higher than max memory")); goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (virCgroupSetMemory(priv->cgroup, newmem) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Failed to set memory for domain")); + goto cleanup; + } + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef); + persistentDef->mem.cur_balloon = newmem; + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto cleanup; + } }
ret = 0;
But what I miss in this patch is, what would:
virDomainSetMaxMemoryFlags(dom, newmem, VIR_DOMAIN_AFFECT_CURRENT | VIR_DOMAIN_MEM_MAXIMUM)
do? I mean, the _CURRENT is not translated to _LIVE or _CONFIG anywhere in the function.
I put that section in 2/2 patch. I'll add a description in v2. Thanks, - Chen