Re: [Xen-devel] [PATCH v3 08/19] libxl: functions to build vmemranges for PV guest

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH v3 08/19] libxl: functions to build vmemranges for PV 
guest"):
> Introduce a arch-independent routine to generate one vmemrange per
> vnode. Also introduce arch-dependent routines for different
> architectures because part of the process is arch-specific -- ARM has
> yet have NUMA support and E820 is x86 only.
> 
> For those x86 guests who care about machine E820 map (i.e. with
> e820_host=1), vnode is further split into several vmemranges to
> accommodate memory holes.  A few stubs for libxl_arm.c are created.
...
> +    /* Generate one vmemrange for each virtual node. */
> +    next = 0;
> +    for (i = 0; i < b_info->num_vnuma_nodes; i++) {
> +        libxl_vnode_info *p = &b_info->vnuma_nodes[i];
> +
> +        v = libxl__realloc(gc, v, sizeof(*v) * (i+1));

Please use GCREALLOC_ARRAY.

> +        v[i].start = next;
> +        v[i].end = next + (p->mem << 20); /* mem is in MiB */

Why are all these values in different units ?

Also, it would be best if the units were in the field and variable
names.  Then you wouldn't have to write an explanatory comment.

> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index e959e37..2018afc 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -338,6 +338,80 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc 
> *gc,
...
> +int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
> +                                      uint32_t domid,
> +                                      libxl_domain_build_info *b_info,
> +                                      libxl__domain_build_state *state)
> +{
...
> +    n = 0; /* E820 counter */

How about putting this information in the variable name rather than
dropping it into a comment ?  Likewise i.

> +        while (remaining > 0) {
> +            if (n >= nr_e820) {
> +                rc = ERROR_FAIL;

ERROR_NOMEM, surely ?

> +            if (map[n].size >= remaining) {
> +                v[x].start = map[n].addr;
> +                v[x].end = map[n].addr + remaining;
> +                map[n].addr += remaining;
> +                map[n].size -= remaining;
> +                remaining = 0;
> +            } else {
> +                v[x].start = map[n].addr;
> +                v[x].end = map[n].addr + map[n].size;
> +                remaining -= map[n].size;
> +                n++;
> +            }

It might be possible to write this more compactly with something like

   use = map[n].size < remaining ? map[n].size : remaining;

Ian.

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