Re: [libvirt] [PATCH] libxl: add memory attach support
On Wed, Aug 31, 2016 at 12:12:55PM +0100, Joao Martins wrote: On 08/31/2016 10:18 AM, Martin Kletzander wrote: On Wed, Aug 31, 2016 at 03:56:02PM +0800, Bob Liu wrote: On 08/30/2016 11:20 PM, Joao Martins wrote: Hey! On 08/30/2016 11:00 AM, Bob Liu wrote: Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl driver, using libxl_set_memory_target in xen libxl. With "virsh attach-device" command we can then hotplug memory to a domain: 128 0 $ virsh attach-device domain_name this.xml --live Actually, attaching a memory device should do something else than calling libxl_set_memory_target(). It should add a guest-visible memory device into the DIMM slot (especially when it is model='dimm'). I'm no libxl expert, but the function you are using is just setting the memory IMHO and it is the same as if you would remove the check for: newmem > virDomainDefGetMemoryTotal in libxlDomainSetMemoryFlags() As a guest visible DIMM slot, doesn't appear to be supported at least as far as goes my reading of libxl on staging. In which case you probably suggest dropping this patch as it's not meant to be doing this? I misconcepted AttachDevice with VIR_DOMAIN_DEVICE_MEMORY (thinking this could another way of increasing the balloon other than libxlDomainSetMemory*), despite seeing now the XML/docs being obvious about its sole purpose. I don't see the point of making this available if there already is a way of doing this. We can think of something if this allows doing something new, though. Joao signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: add memory attach support
On 08/31/2016 10:18 AM, Martin Kletzander wrote: > On Wed, Aug 31, 2016 at 03:56:02PM +0800, Bob Liu wrote: >> >> On 08/30/2016 11:20 PM, Joao Martins wrote: >>> Hey! >>> >>> On 08/30/2016 11:00 AM, Bob Liu wrote: Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl driver, using libxl_set_memory_target in xen libxl. With "virsh attach-device" command we can then hotplug memory to a domain: 128 0 $ virsh attach-device domain_name this.xml --live > > Actually, attaching a memory device should do something else than > calling libxl_set_memory_target(). It should add a guest-visible memory > device into the DIMM slot (especially when it is model='dimm'). I'm no > libxl expert, but the function you are using is just setting the memory > IMHO and it is the same as if you would remove the check for: > newmem > virDomainDefGetMemoryTotal > in libxlDomainSetMemoryFlags() As a guest visible DIMM slot, doesn't appear to be supported at least as far as goes my reading of libxl on staging. In which case you probably suggest dropping this patch as it's not meant to be doing this? I misconcepted AttachDevice with VIR_DOMAIN_DEVICE_MEMORY (thinking this could another way of increasing the balloon other than libxlDomainSetMemory*), despite seeing now the XML/docs being obvious about its sole purpose. Joao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: add memory attach support
On Wed, Aug 31, 2016 at 03:56:02PM +0800, Bob Liu wrote: On 08/30/2016 11:20 PM, Joao Martins wrote: Hey! On 08/30/2016 11:00 AM, Bob Liu wrote: Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl driver, using libxl_set_memory_target in xen libxl. With "virsh attach-device" command we can then hotplug memory to a domain: 128 0 $ virsh attach-device domain_name this.xml --live Actually, attaching a memory device should do something else than calling libxl_set_memory_target(). It should add a guest-visible memory device into the DIMM slot (especially when it is model='dimm'). I'm no libxl expert, but the function you are using is just setting the memory IMHO and it is the same as if you would remove the check for: newmem > virDomainDefGetMemoryTotal in libxlDomainSetMemoryFlags() Signed-off-by: Bob LiuSee few very minor comments below, but overall LGTM. --- src/libxl/libxl_domain.c | 1 + src/libxl/libxl_driver.c | 29 + 2 files changed, 30 insertions(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index f529a2e..3924ba0 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = { .macPrefix = { 0x00, 0x16, 0x3e }, .devicesPostParseCallback = libxlDomainDeviceDefPostParse, .domainPostParseCallback = libxlDomainDefPostParse, +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG }; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a34eb02..541ea3b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, #endif static int +libxlDomainAttachMemory(libxlDriverPrivatePtr driver, +virDomainObjPtr vm, +virDomainMemoryDefPtr mem) +{ +int res = 0; I think you don't need to initialize the variable. +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + +if (mem->targetNode != 0) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupport non-zero target node for memory device")); Probably changing the message to "non-zero target node not supported" instead? The messages sounds like you are removing support for it. But english is not my mothertongue so may be it's just my wrong reading :) Should we fail with other error as this patch, or VIR_WARN the user and ignore Agree to use VIR_WARN(), will be updated. I think definitely fail. Otherwise you succeed with something that the user clearly did not want. targetNode. AFAIK libxl doesn't yet support ballooning for a specific node. What do others think? And it's said here as well, you are changing ballooning, which memory hot-plug should not do. +return -1; +} + +res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1); Should we unlock virDomainObj while ballooning, as similarly done in libxlDomainSetMemory* ? Yes, that's necessary. +if (res < 0) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to attach %lluKB memory for domain %d"), + mem->size, vm->def->id); +return -1; +} +return 0; +} + +static int libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver, dev->data.hostdev = NULL; break; +case VIR_DOMAIN_DEVICE_MEMORY: +ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); +dev->data.memory = NULL; This should probably be: ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); if (!ret) dev->data.memory = NULL; Right? This code was from qemu: 7408 /* note that qemuDomainAttachMemory always consumes dev->data.memory 7409 * and dispatches DeviceAdded event on success */ 7410 ret = qemuDomainAttachMemory(driver, vm, 7411 dev->data.memory); 7412 dev->data.memory = NULL; But I also forgot to free mem in libxlDomainAttachMemory(), thank you for the reminding. qemu uses it because it calls virDomainMemoryInsert() which steals the pointer data. You are leaking the memory here. Yes, you can free the data in libxlDomainAttachMemory(), but in case it is not needed (like in qemu), it is just creating ugly code. Also looking at qemu's implementation, there are bunch of safety checks and domain definition updates which are definitely not done in this patch. +break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("device type '%s' cannot be attached"), -- Regards,
Re: [libvirt] [PATCH] libxl: add memory attach support
On 08/31/2016 08:56 AM, Bob Liu wrote: > > On 08/30/2016 11:20 PM, Joao Martins wrote: >> Hey! >> >> On 08/30/2016 11:00 AM, Bob Liu wrote: >>> Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl >>> driver, using libxl_set_memory_target in xen libxl. >>> >>> With "virsh attach-device" command we can then hotplug memory to a domain: >>> >>> >>> 128 >>> 0 >>> >>> >>> >>> $ virsh attach-device domain_name this.xml --live >>> >>> Signed-off-by: Bob Liu>> See few very minor comments below, but overall LGTM. >> >>> --- >>> src/libxl/libxl_domain.c | 1 + >>> src/libxl/libxl_driver.c | 29 + >>> 2 files changed, 30 insertions(+) >>> >>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c >>> index f529a2e..3924ba0 100644 >>> --- a/src/libxl/libxl_domain.c >>> +++ b/src/libxl/libxl_domain.c >>> @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = { >>> .macPrefix = { 0x00, 0x16, 0x3e }, >>> .devicesPostParseCallback = libxlDomainDeviceDefPostParse, >>> .domainPostParseCallback = libxlDomainDefPostParse, >>> +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG >>> }; >>> >>> >>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >>> index a34eb02..541ea3b 100644 >>> --- a/src/libxl/libxl_driver.c >>> +++ b/src/libxl/libxl_driver.c >>> @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr >>> driver, >>> #endif >>> >>> static int >>> +libxlDomainAttachMemory(libxlDriverPrivatePtr driver, >>> +virDomainObjPtr vm, >>> +virDomainMemoryDefPtr mem) >>> +{ >>> +int res = 0; >> I think you don't need to initialize the variable. >> >>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); >>> + >>> +if (mem->targetNode != 0) { >>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >>> + _("unsupport non-zero target node for memory >>> device")); >> Probably changing the message to "non-zero target node not supported" >> instead? The >> messages sounds like you are removing support for it. But english is not my >> mothertongue so may be it's just my wrong reading :) >> >> Should we fail with other error as this patch, or VIR_WARN the user and >> ignore > > Agree to use VIR_WARN(), will be updated. Ah sorry, I forgot to add a "?" in this sentence, and I am not sure what would the correct behavior here. I think the current is correct, and I assume user wouldn't try to memory balloon to a node other than 0 as it can't create a guest with multiple nodes either. So probably it's reasonable to left it as is, unless someone raises a flag to loose the restriction? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: add memory attach support
On 08/30/2016 11:20 PM, Joao Martins wrote: > Hey! > > On 08/30/2016 11:00 AM, Bob Liu wrote: >> Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl >> driver, using libxl_set_memory_target in xen libxl. >> >> With "virsh attach-device" command we can then hotplug memory to a domain: >> >> >> 128 >> 0 >> >> >> >> $ virsh attach-device domain_name this.xml --live >> >> Signed-off-by: Bob Liu> See few very minor comments below, but overall LGTM. > >> --- >> src/libxl/libxl_domain.c | 1 + >> src/libxl/libxl_driver.c | 29 + >> 2 files changed, 30 insertions(+) >> >> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c >> index f529a2e..3924ba0 100644 >> --- a/src/libxl/libxl_domain.c >> +++ b/src/libxl/libxl_domain.c >> @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = { >> .macPrefix = { 0x00, 0x16, 0x3e }, >> .devicesPostParseCallback = libxlDomainDeviceDefPostParse, >> .domainPostParseCallback = libxlDomainDefPostParse, >> +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG >> }; >> >> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >> index a34eb02..541ea3b 100644 >> --- a/src/libxl/libxl_driver.c >> +++ b/src/libxl/libxl_driver.c >> @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr >> driver, >> #endif >> >> static int >> +libxlDomainAttachMemory(libxlDriverPrivatePtr driver, >> +virDomainObjPtr vm, >> +virDomainMemoryDefPtr mem) >> +{ >> +int res = 0; > I think you don't need to initialize the variable. > >> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); >> + >> +if (mem->targetNode != 0) { >> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("unsupport non-zero target node for memory >> device")); > Probably changing the message to "non-zero target node not supported" > instead? The > messages sounds like you are removing support for it. But english is not my > mothertongue so may be it's just my wrong reading :) > > Should we fail with other error as this patch, or VIR_WARN the user and ignore Agree to use VIR_WARN(), will be updated. > targetNode. AFAIK libxl doesn't yet support ballooning for a specific node. > What do > others think? > >> +return -1; >> +} >> + >> +res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1); > Should we unlock virDomainObj while ballooning, as similarly done in > libxlDomainSetMemory* ? > Yes, that's necessary. >> +if (res < 0) { >> +virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Failed to attach %lluKB memory for domain %d"), >> + mem->size, vm->def->id); >> +return -1; >> +} >> +return 0; >> +} >> + >> +static int >> libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, >> virDomainObjPtr vm, >> virDomainHostdevDefPtr hostdev) >> @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr >> driver, >> dev->data.hostdev = NULL; >> break; >> >> +case VIR_DOMAIN_DEVICE_MEMORY: >> +ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); >> +dev->data.memory = NULL; > This should probably be: > > ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); > if (!ret) > dev->data.memory = NULL; > > Right? > This code was from qemu: 7408 /* note that qemuDomainAttachMemory always consumes dev->data.memory 7409 * and dispatches DeviceAdded event on success */ 7410 ret = qemuDomainAttachMemory(driver, vm, 7411 dev->data.memory); 7412 dev->data.memory = NULL; But I also forgot to free mem in libxlDomainAttachMemory(), thank you for the reminding. >> +break; >> + >> default: >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("device type '%s' cannot be attached"), >> -- Regards, -Bob -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: add memory attach support
Hey! On 08/30/2016 11:00 AM, Bob Liu wrote: > Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl > driver, using libxl_set_memory_target in xen libxl. > > With "virsh attach-device" command we can then hotplug memory to a domain: > > > 128 > 0 > > > > $ virsh attach-device domain_name this.xml --live > > Signed-off-by: Bob LiuSee few very minor comments below, but overall LGTM. > --- > src/libxl/libxl_domain.c | 1 + > src/libxl/libxl_driver.c | 29 + > 2 files changed, 30 insertions(+) > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index f529a2e..3924ba0 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = { > .macPrefix = { 0x00, 0x16, 0x3e }, > .devicesPostParseCallback = libxlDomainDeviceDefPostParse, > .domainPostParseCallback = libxlDomainDefPostParse, > +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG > }; > > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index a34eb02..541ea3b 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr > driver, > #endif > > static int > +libxlDomainAttachMemory(libxlDriverPrivatePtr driver, > +virDomainObjPtr vm, > +virDomainMemoryDefPtr mem) > +{ > +int res = 0; I think you don't need to initialize the variable. > +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); > + > +if (mem->targetNode != 0) { > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("unsupport non-zero target node for memory > device")); Probably changing the message to "non-zero target node not supported" instead? The messages sounds like you are removing support for it. But english is not my mothertongue so may be it's just my wrong reading :) Should we fail with other error as this patch, or VIR_WARN the user and ignore targetNode. AFAIK libxl doesn't yet support ballooning for a specific node. What do others think? > +return -1; > +} > + > +res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1); Should we unlock virDomainObj while ballooning, as similarly done in libxlDomainSetMemory* ? > +if (res < 0) { > +virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to attach %lluKB memory for domain %d"), > + mem->size, vm->def->id); > +return -1; > +} > +return 0; > +} > + > +static int > libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, > virDomainObjPtr vm, > virDomainHostdevDefPtr hostdev) > @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr > driver, > dev->data.hostdev = NULL; > break; > > +case VIR_DOMAIN_DEVICE_MEMORY: > +ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); > +dev->data.memory = NULL; This should probably be: ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); if (!ret) dev->data.memory = NULL; Right? > +break; > + > default: > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("device type '%s' cannot be attached"), > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libxl: add memory attach support
Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl driver, using libxl_set_memory_target in xen libxl. With "virsh attach-device" command we can then hotplug memory to a domain: 128 0 $ virsh attach-device domain_name this.xml --live Signed-off-by: Bob Liu--- src/libxl/libxl_domain.c | 1 + src/libxl/libxl_driver.c | 29 + 2 files changed, 30 insertions(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index f529a2e..3924ba0 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = { .macPrefix = { 0x00, 0x16, 0x3e }, .devicesPostParseCallback = libxlDomainDeviceDefPostParse, .domainPostParseCallback = libxlDomainDefPostParse, +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG }; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a34eb02..541ea3b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, #endif static int +libxlDomainAttachMemory(libxlDriverPrivatePtr driver, +virDomainObjPtr vm, +virDomainMemoryDefPtr mem) +{ +int res = 0; +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + +if (mem->targetNode != 0) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unsupport non-zero target node for memory device")); +return -1; +} + +res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1); +if (res < 0) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to attach %lluKB memory for domain %d"), + mem->size, vm->def->id); +return -1; +} +return 0; +} + +static int libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver, dev->data.hostdev = NULL; break; +case VIR_DOMAIN_DEVICE_MEMORY: +ret = libxlDomainAttachMemory(driver, vm, dev->data.memory); +dev->data.memory = NULL; +break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("device type '%s' cannot be attached"), -- 2.6.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list