Re: [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap

[Stefan Bader <stefan.bader@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
>>> 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@xxxxxxxxxxxxx>
>>>     
>>
>> The libxl calling logic looks Ok to me. So from the libxl perspective
>> you can tack on Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>
>>   
>>> ---
>>>  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

Attachment: 0001-libxl-Correctly-initialize-vcpu-bitmap.patch
Description: Text Data

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel