On 06/28/2013 08:32 AM, Dario Faggioli wrote:
Hi Jim, Everyone,
First patch series, so maybe a small introduction is required: I'm Dario and I work for Citrix on improving the NUMA support of Xen.
Welcome Dario!
This patch series implements some of the missing bits and pieces, in the libxl driver, regarding obtaining per-host and per-domain NUMA related information. It's not the full libvirt NUMA interface, since we don't have all we would need for that in Xen yet (we will in the next version, 4.4), but it's certainly better than having nothing! :-)
Yep, incremental improvements are fine.
Basically, I'm enhancing capability reporting, to cover NUMA topology (patch 01), and I'm implementing nodeGetCellsFreeMemory (patch 02) and virDomainGetNumaParameters (patch 04) for the libxl driver. This last one requires at least Xen 4.3, so I put the implementation within the proper #ifdef-ery.
Ok, I'll comment in the individual patches too.
What I'm really not sure about is patch 03, which is something I need if I want patch 04 to function properly. Basically it is about advertising that the libxl driver supports VIR_TYPED_PARAM_STRING. I looked at how that is done in the qemu driver, but I'm not entirely sure I completely understood the logic behind it, so, please, tell me if I missed or misinterpreted anything!
I think we'll need another libvirt dev more familiar with this to verify, but afaict advertising that the driver supports VIR_TYPED_PARAM_STRING is required when returning a string in virTypedParam, for compatibility with older libvirt clients. Without it, strings wouldn't be returned to newer clients that support VIR_TYPED_PARAM_STRING.
In particular, should I have added more of those "flags &= ~VIR_TYPED_PARAM_STRING_OKAY;" statements, as it happens in the qemu driver? If yes, in which functions?
Once the driver advertises VIR_TYPED_PARAM_STRING, I think the functions taking virTypedParamPtr as a parameter must accommodate that, at least the getters. E.g. qemuDomainGetSchedulerParametersFlags() in the qemu driver has virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_TYPED_PARAM_STRING_OKAY, -1); /* We don't return strings, and thus trivially support this flag. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; I think the same applies for libxlDomainGetSchedulerParametersFlags(), which looks to be the only getter function in the libxl driver taking virTypedParam.
Finally, allow me to say that it was a while that I wanted start hacking a bit on libvirt. I'm really glad I've eventually been able to do so, and I definitely plan to continue (with particular focus on NUMA related stuff).
Great to hear and looking forward to your work! Regards, Jim