All of lore.kernel.org
 help / color / mirror / Atom feed
* [libvirt] [PATCH] libxl: Correctly initialize vcpu bitmap
@ 2013-07-22 10:51 Stefan Bader
  2013-07-22 19:39 ` [libvirt] [Xen-devel] " Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Bader @ 2013-07-22 10:51 UTC (permalink / raw)
  To: libvir-list, xen-devel; +Cc: Ian Campbell

This fixes the basic setup but there is likely more to do if things
like manual CPU hirarchy (nodes, cores, threads) to be working.

Cross-posting to xen-devel to make sure I am doing things correctly.

-Stefan


>From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Fri, 19 Jul 2013 15:20:00 +0200
Subject: [PATCH] libxl: Correctly initialize vcpu bitmap

The avai_vcpu bitmap has to be allocated before it can be used (using
the maximum allowed value for that). Then for each available VCPU the
bit in the mask has to be set (libxl_bitmap_set takes a bit position
as an argument, not the number of bits to set).

Without this, I would always only get one VCPU for guests created
through libvirt/libxl.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 src/libxl/libxl_conf.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4a0fba9..7592dd2 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -331,7 +331,8 @@ error:
 }
 
 static int
-libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
+libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
+                      libxl_domain_config *d_config)
 {
     libxl_domain_build_info *b_info = &d_config->b_info;
     int hvm = STREQ(def->os.type, "hvm");
@@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
         libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
     else
         libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
+
     b_info->max_vcpus = def->maxvcpus;
-    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
+    if (libxl_cpu_bitmap_alloc(driver->ctx, &b_info->avail_vcpus,
+                               def->maxvcpus))
+        goto error;
+    libxl_bitmap_set_none(&b_info->avail_vcpus);
+    for (i = 0; i < def->vcpus; i++)
+        libxl_bitmap_set((&b_info->avail_vcpus), i);
+
     if (def->clock.ntimers > 0 &&
         def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
         switch (def->clock.timers[0]->mode) {
@@ -802,7 +810,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
     if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
         return -1;
 
-    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
+    if (libxlMakeDomBuildInfo(driver, def, d_config) < 0) {
         return -1;
     }
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap
  2013-07-22 10:51 [libvirt] [PATCH] libxl: Correctly initialize vcpu bitmap Stefan Bader
@ 2013-07-22 19:39 ` Konrad Rzeszutek Wilk
  2013-07-23  7:08   ` Stefan Bader
  2013-07-23 21:20   ` Jim Fehlig
  0 siblings, 2 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-22 19:39 UTC (permalink / raw)
  To: Stefan Bader; +Cc: libvir-list, xen-devel, Ian Campbell

On Mon, Jul 22, 2013 at 12:51:05PM +0200, Stefan Bader wrote:
> This fixes the basic setup but there is likely more to do if things
> like manual CPU hirarchy (nodes, cores, threads) to be working.
> 
> Cross-posting to xen-devel to make sure I am doing things correctly.
> 
> -Stefan
> 
> 
> >From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Fri, 19 Jul 2013 15:20:00 +0200
> Subject: [PATCH] libxl: Correctly initialize vcpu bitmap
> 
> The avai_vcpu bitmap has to be allocated before it can be used (using

avail_vcpu ?

> the maximum allowed value for that). Then for each available VCPU the
> bit in the mask has to be set (libxl_bitmap_set takes a bit position
> as an argument, not the number of bits to set).
> 
> Without this, I would always only get one VCPU for guests created
> through libvirt/libxl.
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>

The libxl calling logic looks Ok to me. So from the libxl perspective
you can tack on Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
>  src/libxl/libxl_conf.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4a0fba9..7592dd2 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -331,7 +331,8 @@ error:
>  }
>  
>  static int
> -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
> +                      libxl_domain_config *d_config)
>  {
>      libxl_domain_build_info *b_info = &d_config->b_info;
>      int hvm = STREQ(def->os.type, "hvm");
> @@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
>      else
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
> +
>      b_info->max_vcpus = def->maxvcpus;
> -    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
> +    if (libxl_cpu_bitmap_alloc(driver->ctx, &b_info->avail_vcpus,
> +                               def->maxvcpus))
> +        goto error;
> +    libxl_bitmap_set_none(&b_info->avail_vcpus);
> +    for (i = 0; i < def->vcpus; i++)
> +        libxl_bitmap_set((&b_info->avail_vcpus), i);
> +
>      if (def->clock.ntimers > 0 &&
>          def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
>          switch (def->clock.timers[0]->mode) {
> @@ -802,7 +810,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
>      if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
>          return -1;
>  
> -    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
> +    if (libxlMakeDomBuildInfo(driver, def, d_config) < 0) {
>          return -1;
>      }
>  
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap
  2013-07-22 19:39 ` [libvirt] [Xen-devel] " Konrad Rzeszutek Wilk
@ 2013-07-23  7:08   ` Stefan Bader
  2013-07-23 21:20   ` Jim Fehlig
  1 sibling, 0 replies; 6+ messages in thread
From: Stefan Bader @ 2013-07-23  7:08 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: libvir-list, xen-devel, Ian Campbell


[-- Attachment #1.1: Type: text/plain, Size: 3436 bytes --]

On 22.07.2013 21:39, Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 22, 2013 at 12:51:05PM +0200, Stefan Bader wrote:
>> This fixes the basic setup but there is likely more to do if things
>> like manual CPU hirarchy (nodes, cores, threads) to be working.
>>
>> Cross-posting to xen-devel to make sure I am doing things correctly.
>>
>> -Stefan
>>
>>
>> >From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@canonical.com>
>> Date: Fri, 19 Jul 2013 15:20:00 +0200
>> Subject: [PATCH] libxl: Correctly initialize vcpu bitmap
>>
>> The avai_vcpu bitmap has to be allocated before it can be used (using
> 
> avail_vcpu ?

*sigh* Yeah, of course I cannot spell. :-P

> 
>> the maximum allowed value for that). Then for each available VCPU the
>> bit in the mask has to be set (libxl_bitmap_set takes a bit position
>> as an argument, not the number of bits to set).
>>
>> Without this, I would always only get one VCPU for guests created
>> through libvirt/libxl.
>>
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> 
> The libxl calling logic looks Ok to me. So from the libxl perspective
> you can tack on Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks, I will leave that (as well as the complimentary fixing of missing
lletters to the libvirt maintainers. :)

-Stefan

> 
>> ---
>>  src/libxl/libxl_conf.c |   14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 4a0fba9..7592dd2 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -331,7 +331,8 @@ error:
>>  }
>>  
>>  static int
>> -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>> +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
>> +                      libxl_domain_config *d_config)
>>  {
>>      libxl_domain_build_info *b_info = &d_config->b_info;
>>      int hvm = STREQ(def->os.type, "hvm");
>> @@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
>>      else
>>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
>> +
>>      b_info->max_vcpus = def->maxvcpus;
>> -    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
>> +    if (libxl_cpu_bitmap_alloc(driver->ctx, &b_info->avail_vcpus,
>> +                               def->maxvcpus))
>> +        goto error;
>> +    libxl_bitmap_set_none(&b_info->avail_vcpus);
>> +    for (i = 0; i < def->vcpus; i++)
>> +        libxl_bitmap_set((&b_info->avail_vcpus), i);
>> +
>>      if (def->clock.ntimers > 0 &&
>>          def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
>>          switch (def->clock.timers[0]->mode) {
>> @@ -802,7 +810,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
>>      if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
>>          return -1;
>>  
>> -    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
>> +    if (libxlMakeDomBuildInfo(driver, def, d_config) < 0) {
>>          return -1;
>>      }
>>  
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap
  2013-07-22 19:39 ` [libvirt] [Xen-devel] " Konrad Rzeszutek Wilk
  2013-07-23  7:08   ` Stefan Bader
@ 2013-07-23 21:20   ` Jim Fehlig
  2013-07-24 11:43     ` Stefan Bader
  1 sibling, 1 reply; 6+ messages in thread
From: Jim Fehlig @ 2013-07-23 21:20 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: libvir-list, xen-devel, Ian Campbell, Stefan Bader

One comment below in addition to Konrad's...

Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 22, 2013 at 12:51:05PM +0200, Stefan Bader wrote:
>   
>> This fixes the basic setup but there is likely more to do if things
>> like manual CPU hirarchy (nodes, cores, threads) to be working.
>>
>> Cross-posting to xen-devel to make sure I am doing things correctly.
>>
>> -Stefan
>>
>>
>> >From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@canonical.com>
>> Date: Fri, 19 Jul 2013 15:20:00 +0200
>> Subject: [PATCH] libxl: Correctly initialize vcpu bitmap
>>
>> The avai_vcpu bitmap has to be allocated before it can be used (using
>>     
>
> avail_vcpu ?
>
>   
>> the maximum allowed value for that). Then for each available VCPU the
>> bit in the mask has to be set (libxl_bitmap_set takes a bit position
>> as an argument, not the number of bits to set).
>>
>> Without this, I would always only get one VCPU for guests created
>> through libvirt/libxl.
>>
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>>     
>
> The libxl calling logic looks Ok to me. So from the libxl perspective
> you can tack on Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>   
>> ---
>>  src/libxl/libxl_conf.c |   14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 4a0fba9..7592dd2 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -331,7 +331,8 @@ error:
>>  }
>>  
>>  static int
>> -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>> +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
>> +                      libxl_domain_config *d_config)
>>  {
>>      libxl_domain_build_info *b_info = &d_config->b_info;
>>      int hvm = STREQ(def->os.type, "hvm");
>> @@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
>>      else
>>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
>> +
>>      b_info->max_vcpus = def->maxvcpus;
>> -    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
>> +    if (libxl_cpu_bitmap_alloc(driver->ctx, &b_info->avail_vcpus,
>> +                               def->maxvcpus))
>>     

You are using the driver-wide libxl_ctx in libxlDriverPrivate here, but
should be using the per-domain libxl_ctx in libxlDomainObjPrivate
structure IMO.

It looks like libxlBuildDomainConfig, which calls libxlMakeDomBuildInfo,
should have just taken virDomainObjPtr instead of virDomainDefPtr. 
libxlBuildDomainConfig would then have access to the per-domain
libxl_ctx, and no longer need the libxlDriverPrivatePtr parameter as well.

Regards,
Jim

>> +        goto error;
>> +    libxl_bitmap_set_none(&b_info->avail_vcpus);
>> +    for (i = 0; i < def->vcpus; i++)
>> +        libxl_bitmap_set((&b_info->avail_vcpus), i);
>> +
>>      if (def->clock.ntimers > 0 &&
>>          def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
>>          switch (def->clock.timers[0]->mode) {
>> @@ -802,7 +810,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
>>      if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
>>          return -1;
>>  
>> -    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
>> +    if (libxlMakeDomBuildInfo(driver, def, d_config) < 0) {
>>          return -1;
>>      }
>>  
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>     
>
>   

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap
  2013-07-23 21:20   ` Jim Fehlig
@ 2013-07-24 11:43     ` Stefan Bader
  2013-07-24 15:14       ` Jim Fehlig
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Bader @ 2013-07-24 11:43 UTC (permalink / raw)
  To: Jim Fehlig; +Cc: libvir-list, xen-devel, Ian Campbell, Konrad Rzeszutek Wilk


[-- Attachment #1.1.1: Type: text/plain, Size: 3397 bytes --]

On 23.07.2013 23:20, Jim Fehlig wrote:
> One comment below in addition to Konrad's...
> 
> Konrad Rzeszutek Wilk wrote:
>> On Mon, Jul 22, 2013 at 12:51:05PM +0200, Stefan Bader wrote:
>>   
>>> This fixes the basic setup but there is likely more to do if things
>>> like manual CPU hirarchy (nodes, cores, threads) to be working.
>>>
>>> Cross-posting to xen-devel to make sure I am doing things correctly.
>>>
>>> -Stefan
>>>
>>>
>>> >From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
>>> From: Stefan Bader <stefan.bader@canonical.com>
>>> Date: Fri, 19 Jul 2013 15:20:00 +0200
>>> Subject: [PATCH] libxl: Correctly initialize vcpu bitmap
>>>
>>> The avai_vcpu bitmap has to be allocated before it can be used (using
>>>     
>>
>> avail_vcpu ?
>>
>>   
>>> the maximum allowed value for that). Then for each available VCPU the
>>> bit in the mask has to be set (libxl_bitmap_set takes a bit position
>>> as an argument, not the number of bits to set).
>>>
>>> Without this, I would always only get one VCPU for guests created
>>> through libvirt/libxl.
>>>
>>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>>>     
>>
>> The libxl calling logic looks Ok to me. So from the libxl perspective
>> you can tack on Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>>   
>>> ---
>>>  src/libxl/libxl_conf.c |   14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 4a0fba9..7592dd2 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -331,7 +331,8 @@ error:
>>>  }
>>>  
>>>  static int
>>> -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>>> +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
>>> +                      libxl_domain_config *d_config)
>>>  {
>>>      libxl_domain_build_info *b_info = &d_config->b_info;
>>>      int hvm = STREQ(def->os.type, "hvm");
>>> @@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>>>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
>>>      else
>>>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
>>> +
>>>      b_info->max_vcpus = def->maxvcpus;
>>> -    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
>>> +    if (libxl_cpu_bitmap_alloc(driver->ctx, &b_info->avail_vcpus,
>>> +                               def->maxvcpus))
>>>     
> 
> You are using the driver-wide libxl_ctx in libxlDriverPrivate here, but
> should be using the per-domain libxl_ctx in libxlDomainObjPrivate
> structure IMO.
> 
> It looks like libxlBuildDomainConfig, which calls libxlMakeDomBuildInfo,
> should have just taken virDomainObjPtr instead of virDomainDefPtr. 
> libxlBuildDomainConfig would then have access to the per-domain
> libxl_ctx, and no longer need the libxlDriverPrivatePtr parameter as well.
> 

So more like attached v2. I am not sure libxlDriverPrivatePtr can really be
dropped (and maybe should not as part of a fix to this issue). Maybe calling
libxl_flask_context_to_sid also should use the per domain context. But at least
libxlMakeVfbList->libxlMakeVfb->virPortAllocatorAcquire sounds a bit like it
might need the driver context.

-Stefan


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-libxl-Correctly-initialize-vcpu-bitmap.patch --]
[-- Type: text/x-diff; name="0001-libxl-Correctly-initialize-vcpu-bitmap.patch", Size: 3847 bytes --]

From 2d4b7e5ae2644e6f7a2ea930d0899ea426cb9c84 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Fri, 19 Jul 2013 15:20:00 +0200
Subject: [PATCH] libxl: Correctly initialize vcpu bitmap

The avail_vcpu bitmap has to be allocated before it can be used (using
the maximum allowed value for that). Then for each available VCPU the
bit in the mask has to be set (libxl_bitmap_set takes a bit position
as an argument, not the number of bits to set).

Without this, I would always only get one VCPU for guests created
through libvirt/libxl.

[v2] Use private ctx from virDomainObjPtr->privateData

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 src/libxl/libxl_conf.c   |   19 +++++++++++++++----
 src/libxl/libxl_conf.h   |    2 +-
 src/libxl/libxl_driver.c |    2 +-
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4a0fba9..f732135 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -331,8 +331,10 @@ error:
 }
 
 static int
-libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
+libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
 {
+    virDomainDefPtr def = vm->def;
+    libxlDomainObjPrivatePtr priv = vm->privateData;
     libxl_domain_build_info *b_info = &d_config->b_info;
     int hvm = STREQ(def->os.type, "hvm");
     size_t i;
@@ -343,8 +345,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
         libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
     else
         libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
+
     b_info->max_vcpus = def->maxvcpus;
-    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
+    if (libxl_cpu_bitmap_alloc(priv->ctx, &b_info->avail_vcpus,
+                               def->maxvcpus))
+        goto error;
+    libxl_bitmap_set_none(&b_info->avail_vcpus);
+    for (i = 0; i < def->vcpus; i++)
+        libxl_bitmap_set((&b_info->avail_vcpus), i);
+
     if (def->clock.ntimers > 0 &&
         def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
         switch (def->clock.timers[0]->mode) {
@@ -795,14 +804,16 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
-                       virDomainDefPtr def, libxl_domain_config *d_config)
+                       virDomainObjPtr vm, libxl_domain_config *d_config)
 {
+    virDomainDefPtr def = vm->def;
+
     libxl_domain_config_init(d_config);
 
     if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
         return -1;
 
-    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
+    if (libxlMakeDomBuildInfo(vm, d_config) < 0) {
         return -1;
     }
 
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 2b4a281..942cdd5 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -126,6 +126,6 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
 
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
-                       virDomainDefPtr def, libxl_domain_config *d_config);
+                       virDomainObjPtr vm, libxl_domain_config *d_config);
 
 #endif /* LIBXL_CONF_H */
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 358d387..98b1985 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -968,7 +968,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
 
     libxl_domain_config_init(&d_config);
 
-    if (libxlBuildDomainConfig(driver, vm->def, &d_config) < 0)
+    if (libxlBuildDomainConfig(driver, vm, &d_config) < 0)
         goto error;
 
     if (driver->autoballoon && libxlFreeMem(priv, &d_config) < 0) {
-- 
1.7.9.5


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap
  2013-07-24 11:43     ` Stefan Bader
@ 2013-07-24 15:14       ` Jim Fehlig
  0 siblings, 0 replies; 6+ messages in thread
From: Jim Fehlig @ 2013-07-24 15:14 UTC (permalink / raw)
  To: Stefan Bader; +Cc: libvir-list, xen-devel, Ian Campbell

Stefan Bader wrote:
> On 23.07.2013 23:20, Jim Fehlig wrote:
>   
[...]
>> You are using the driver-wide libxl_ctx in libxlDriverPrivate here, but
>> should be using the per-domain libxl_ctx in libxlDomainObjPrivate
>> structure IMO.
>>
>> It looks like libxlBuildDomainConfig, which calls libxlMakeDomBuildInfo,
>> should have just taken virDomainObjPtr instead of virDomainDefPtr. 
>> libxlBuildDomainConfig would then have access to the per-domain
>> libxl_ctx, and no longer need the libxlDriverPrivatePtr parameter as well.
>>
>>     
>
> So more like attached v2. I am not sure libxlDriverPrivatePtr can really be
> dropped (and maybe should not as part of a fix to this issue). Maybe calling
> libxl_flask_context_to_sid also should use the per domain context.

Yeah, I think so, but as a separate patch.

>  But at least
> libxlMakeVfbList->libxlMakeVfb->virPortAllocatorAcquire sounds a bit like it
> might need the driver context.
>   

Ah, right, neglected that call chain when suggesting to remove
libxlDriverPrivatePtr.

> From 2d4b7e5ae2644e6f7a2ea930d0899ea426cb9c84 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Fri, 19 Jul 2013 15:20:00 +0200
> Subject: [PATCH] libxl: Correctly initialize vcpu bitmap
>
> The avail_vcpu bitmap has to be allocated before it can be used (using
> the maximum allowed value for that). Then for each available VCPU the
> bit in the mask has to be set (libxl_bitmap_set takes a bit position
> as an argument, not the number of bits to set).
>
> Without this, I would always only get one VCPU for guests created
> through libvirt/libxl.
>
> [v2] Use private ctx from virDomainObjPtr->privateData
>   

No need for history of patch revisions in the commit message.  They can
be added with 'git send-email --annotate ...'.

> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  src/libxl/libxl_conf.c   |   19 +++++++++++++++----
>  src/libxl/libxl_conf.h   |    2 +-
>  src/libxl/libxl_driver.c |    2 +-
>  3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4a0fba9..f732135 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -331,8 +331,10 @@ error:
>  }
>  
>  static int
> -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
>  {
> +    virDomainDefPtr def = vm->def;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
>      libxl_domain_build_info *b_info = &d_config->b_info;
>      int hvm = STREQ(def->os.type, "hvm");
>      size_t i;
> @@ -343,8 +345,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
>      else
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
> +
>      b_info->max_vcpus = def->maxvcpus;
> -    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
> +    if (libxl_cpu_bitmap_alloc(priv->ctx, &b_info->avail_vcpus,
> +                               def->maxvcpus))
>   

Fits on one line.

ACK to the patch though.  I fixed these minor nits and pushed.

Regards,
Jim

> +        goto error;
> +    libxl_bitmap_set_none(&b_info->avail_vcpus);
> +    for (i = 0; i < def->vcpus; i++)
> +        libxl_bitmap_set((&b_info->avail_vcpus), i);
> +
>      if (def->clock.ntimers > 0 &&
>          def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
>          switch (def->clock.timers[0]->mode) {
> @@ -795,14 +804,16 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>  
>  int
>  libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> -                       virDomainDefPtr def, libxl_domain_config *d_config)
> +                       virDomainObjPtr vm, libxl_domain_config *d_config)
>  {
> +    virDomainDefPtr def = vm->def;
> +
>      libxl_domain_config_init(d_config);
>  
>      if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
>          return -1;
>  
> -    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
> +    if (libxlMakeDomBuildInfo(vm, d_config) < 0) {
>          return -1;
>      }
>  
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 2b4a281..942cdd5 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -126,6 +126,6 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
>  
>  int
>  libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> -                       virDomainDefPtr def, libxl_domain_config *d_config);
> +                       virDomainObjPtr vm, libxl_domain_config *d_config);
>  
>  #endif /* LIBXL_CONF_H */
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 358d387..98b1985 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -968,7 +968,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>  
>      libxl_domain_config_init(&d_config);
>  
> -    if (libxlBuildDomainConfig(driver, vm->def, &d_config) < 0)
> +    if (libxlBuildDomainConfig(driver, vm, &d_config) < 0)
>          goto error;
>  
>      if (driver->autoballoon && libxlFreeMem(priv, &d_config) < 0) {
> -- 1.7.9.5

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-24 15:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-22 10:51 [libvirt] [PATCH] libxl: Correctly initialize vcpu bitmap Stefan Bader
2013-07-22 19:39 ` [libvirt] [Xen-devel] " Konrad Rzeszutek Wilk
2013-07-23  7:08   ` Stefan Bader
2013-07-23 21:20   ` Jim Fehlig
2013-07-24 11:43     ` Stefan Bader
2013-07-24 15:14       ` Jim Fehlig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.