Re: [Xen-devel] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 11/23/2015 03:35 PM, Jim Fehlig wrote:
> On 11/20/2015 05:40 PM, Joao Martins wrote:
>>
>> On 11/20/2015 07:05 PM, Jim Fehlig wrote:
>>> On 11/19/2015 04:45 PM, Joao Martins wrote:
>>>
>>> You're not going to be happy with me...
>>>
>>>> This new field in libxlDomainObjPrivate is named "config"
>>>> and is kept while the domain is active.
>>> While this sounded like a good idea when I mentioned it, I'm now worried 
>>> that
>>> the config will quickly become stale and cause problems if used elsewhere 
>>> (e.g.
>>> see my yet-to-be-written comment in 3/3).  IIUC correctly, 
>>> libxl_domain_config
>>> is only useful when creating the domain. Subsequently adding/removing 
>>> devices,
>>> memory, vcpus, etc. would not be reflected in the libxl_domain_config 
>>> object. I
>>> suppose it would useful (and still valid) in the start callback, but IMO
>>> including it in the libxlDomainPrivate struct fools us into believing it 
>>> could
>>> be used elsewhere throughout the life of the domain. I now have second 
>>> doubts
>>> about this. What do you think?
>> I agree with you, and since there a libxl_device_nic_list as you suggested, 
>> it
>> would actually be much cleaner and safer compared to libxl_domain_config
>> alternative (though with a small performance cost). And we would avoid end up
>> having config just lying there with no additional use (besides StartCallback)
>> and inconsistent info.
>>
>> The only thing that the libxlDomainObjPrivate approach is better than
>> libxl_device_nic_list() would be that we don't need to refetch the devid, 
>> since
>> the nics array has it correctly filled already when console callback is 
>> invoked.
>> Whereas libxl_device_nic_list will refetch the same info (in additiona to all
>> entries in the backend directory) from xenstore thus adding up overhead. But
>> given that this is only once and in domain create I think it's not a big 
>> deal.
> 
> Right. I think the extra overhead is in the noise relative to the other
> activities involved in starting a domain.
> 
>> Would you agree then to resend this series without this patch and using
>> libxl_device_nic_list, as the final approach? Thanks for pointing out this 
>> issue!
> 
> I think so. If you dislike the extra overhead of libxl_device_nic_list, 
> another
> option would be something like a libxlDomainStartCallbackInfo struct that
> contains the virDomainObj and libxl_domain_config, and is passed to the start
> callback via for_callback of libxl_asyncop_how. That would allow us to use the
> libxl_domain_config object in the callback, but still dispose it after the 
> start
> completes.
I did a quick measurement to double-check and have a rough idea of the
"libxl_device_nic_list" cost.

Each line is in the form of

<n> VIFs: <libxl_device_nic_list cost in us> / <libvirt dom create cost in secs>

1  VIF : 1066  us / 0.315 s
2  VIF : 1762  us / 0.375 s
4  VIF : 3528  us / 0.560 s
8  VIF : 6726  us / 0.953 s
16 VIF : 13378 us / 1.653 s

It almost grows linearly with the number of NICs having ~1ms per NIC. And given
the numbers above, I think the extra overhead is indeed small and neglible, so
I'll be sending with the libxl_device_nic_list approach as also agreed in your
previous comment.

Regards,
Joao

> 
> Regards,
> Jim
> 

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