Re: [libvirt] [PATCH 4/4] libxl: implement virDomainGetNumaParameters
On mar, 2013-07-02 at 14:22 -0600, Jim Fehlig wrote:
On 06/28/2013 08:33 AM, Dario Faggioli wrote:
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 53af609..9bd6d99 100644
>
> +/* NUMA node affinity information is available through libxl
> + * starting from Xen 4.3. */
> +#ifdef LIBXL_HAVE_DOMAIN_NODEAFFINITY
> +
> +/* Number of Xen NUMA parameters */
> +#define LIBXL_NUMA_NPARAM 2
Is there a similar definition in Xen? E.g. would future changes to libxl adding
more parameters, but neglecting to update here, cause problems?
There is nothing like this in Xen. I went for this #define based
approach because that is right what the QEMU driver does for NUMA
parameters and, also, what libxl driver does for scheduler parameters.
I therefore think that this could be considered safe, from that point of
view. What could happen is that more NUMA (and, perhaps, even more
scheduler) parameters can pop up in the future. However, given libxl
commitment for API stability, this will be advertised with something
like LIBXL_HAVE_THIS_NEW_NUMA_PARAM symbol.
That means we, as soon as we wan to support it, will end up with
something like:
#ifdef LIBXL_HAVE_THIS_NEW_NUMA_PARAM
# define LIBXL_NUMA_NPARAM 3
#else
# define LIBXL_NUMA_NPARAM 2
#endif
Is that reasonable?
> +
> +static int
> +libxlDomainGetNumaParameters(virDomainPtr dom,
> + virTypedParameterPtr params,
> + int *nparams,
> + unsigned int flags)
> +{
> + libxlDriverPrivatePtr driver = dom->conn->privateData;
> + libxlDomainObjPrivatePtr priv;
> + virDomainObjPtr vm;
> +
> + libxl_bitmap nodemap;
> + virBitmapPtr nodes = NULL;
> + char *nodeset = NULL;
> +
> + int rc, ret = -1;
> + int i, j;
No need for the extra whitespace between these local variables.
Ok, done.
> + case 1:
> + /* Node affinity */
> +
> + /* Let's allocate both libxl and libvirt bitmaps */
> + if (libxl_node_bitmap_alloc(priv->ctx, &nodemap, 0) ||
> + !(nodes = virBitmapNew(libxl_get_max_nodes(priv->ctx)))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + rc = libxl_domain_get_nodeaffinity(priv->ctx, vm->def->id,
> + &nodemap);
Fits on one line, albeit with nothing to spare :).
Does it? According to my editor here, if I put it in just one line that
ends at 82. :-(
I'm therefore changing it in
rc = libxl_domain_get_nodeaffinity(priv->ctx,
vm->def->id,
&nodemap);
As you mentioned in another e-mail.
> @@ -4741,6 +4876,9 @@ static virDriver libxlDriver = {
> .domainGetSchedulerParametersFlags = libxlDomainGetSchedulerParametersFlags,
/* 0.9.2 */
> .domainSetSchedulerParameters = libxlDomainSetSchedulerParameters, /* 0.9.0
*/
> .domainSetSchedulerParametersFlags = libxlDomainSetSchedulerParametersFlags,
/* 0.9.2 */
> +#ifdef LIBXL_HAVE_DOMAIN_NODEAFFINITY
> + .domainGetNumaParameters = libxlDomainGetNumaParameters, /* 1.1.1 */
> +#endif
> .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
> .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
Looks good with the exception of these minor nits, but needs to go along with an
updated 3/4.
Sure.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
Attachments:
- signature.asc (application/pgp-signature — 198 bytes)