Re: [libvirt] [PATCH] libxl: add memory attach support

2016-09-02 Thread Martin Kletzander

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

2016-08-31 Thread Joao Martins
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

2016-08-31 Thread Martin Kletzander

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 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.



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

2016-08-31 Thread Joao Martins


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

2016-08-31 Thread Bob Liu

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

2016-08-30 Thread Joao Martins
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
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

2016-08-30 Thread Bob Liu
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