5 a.m.
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:
<memory model='dimm'>
<target>
<size unit='MiB'>128</size>
<node>0</node>
</target>
</memory>
$ virsh attach-device domain_name this.xml --live
Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
---
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
10:20 a.m.
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:
<memory model='dimm'>
<target>
<size unit='MiB'>128</size>
<node>0</node>
</target>
</memory>
$ virsh attach-device domain_name this.xml --live
Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
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
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"),
2:56 a.m.
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:
> <memory model='dimm'>
> <target>
> <size unit='MiB'>128</size>
> <node>0</node>
> </target>
> </memory>
>
> $ virsh attach-device domain_name this.xml --live
>
> Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
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
3:24 a.m.
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:
>> <memory model='dimm'>
>> <target>
>> <size unit='MiB'>128</size>
>> <node>0</node>
>> </target>
>> </memory>
>>
>> $ virsh attach-device domain_name this.xml --live
>>
>> Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
> 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?
4:18 a.m.
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:
>> <memory model='dimm'>
>> <target>
>> <size unit='MiB'>128</size>
>> <node>0</node>
>> </target>
>> </memory>
>>
>> $ 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 Liu <bob.liu(a)oracle.com>
> 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.
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,
-Bob
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:12 a.m.
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:
>>> <memory model='dimm'>
>>> <target>
>>> <size unit='MiB'>128</size>
>>> <node>0</node>
>>> </target>
>>> </memory>
>>>
>>> $ 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
9:08 a.m.
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:
>>>> <memory model='dimm'>
>>>> <target>
>>>> <size unit='MiB'>128</size>
>>>> <node>0</node>
>>>> </target>
>>>> </memory>
>>>>
>>>> $ 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
3003
days inactive
3006
days old
6 comments
3 participants
participants (3)
-
Bob Liu -
Joao Martins -
Martin Kletzander