On 07/30/2014 11:41 PM, Chen Hanxiao wrote: > Currently, setmaxmem return success on an active > domain, but nothing happened. Not quite true... Prior to these changes... # virsh -c lxc:/// start vm1 # virsh -c lxc:/// dominfo vm1 | grep memory Max memory: 500000 KiB Used memory: 776 KiB # virsh -c lxc:/// dumpxml vm1 | grep mem <memory unit='KiB'>500000</memory> # cat /sys/fs/cgroup/memory/machine.slice/*/memory.limit_in_bytes 512000000 # virsh -c lxc:/// setmaxmem vm1 525000 # virsh -c lxc:/// dominfo vm1 | grep memory Max memory: 525000 KiB Used memory: 776 KiB # virsh -c lxc:/// dumpxml vm1 | grep mem <memory unit='KiB'>525000</memory> # cat /sys/fs/cgroup/memory/machine.slice/*/memory.limit_in_bytes 512000000 # virsh -c lxc:/// console vm1 Connected to domain vm1 Escape character is ^] sh-4.2# grep Mem /proc/meminfo MemTotal: 500000 kB MemFree: 499076 kB MemAvailable: 3629632 kB # Now I'll agree that technically what one expected to happen didn't since it's not supported, but something did happen :-)... And of course after a destroy the maxmem went back to 500000. > This patch will disable this behaviour, > also add support persistent config. > And it will be used in a later patch. FWIW: I agree with Peter's observation from your v1 - the two patches should be one as it is clearer that way what you are doing and leaves no doubt why the flag is used I do see that other drivers like qemu & libxl use the MEM_MAXIMUM flag. So perhaps said differently - "This patch changes the LXC driver setmaxmem function to support the '--live', '--config', and '--current' flags by revectoring the code through the setmem function using the VIR_DOMAIN_MEM_MAXIMUM flag. The setmem code is refactored to handle both cases depending on the flag. The changed maxmem code for the MEM_MAXIMUM path will not allow modification to the memory values of an active guest unless the --config switch is used." > > Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx> > --- > v2: disable changing max memory on an active domain > drop useless as_assert > > src/lxc/lxc_driver.c | 64 +++++++++++++++++++++++++++++++++------------------- > 1 file changed, 41 insertions(+), 23 deletions(-) > > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index 9e12ecc..d99ab3b 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,50 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, > &persistentDef) < 0) > goto cleanup; > I see the next set of changes is essentially cut-n-pasted from qemu, although the changes after the else are the same as the former setmem function - it's just the git diff that obfuscates that. The changes do work as I'd expect, so ACK from that point of view. I can merge the two patches for you and commit, but I'll give it a bit to make sure no one else has concerns John > - 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) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("Cannot resize the max memory " > + "on an active domain")); > + goto cleanup; > + } > > - if (newmem > oldmax) { > - virReportError(VIR_ERR_INVALID_ARG, > - "%s", _("Cannot set memory higher than max memory")); > - goto cleanup; > - } > + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > + 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); > - 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) { > + persistentDef->mem.cur_balloon = newmem; > + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) > + goto cleanup; > + } > } > > ret = 0; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list