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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list