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