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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jan 13, 2015 at 06:15:49PM +0000, Ian Jackson wrote:
> 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.

No problem.

> 
> > +        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.
> 

I will rework code / data structures that's unclear about their units.

> > 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.
> 

Sure. Done.

> > +        while (remaining > 0) {
> > +            if (n >= nr_e820) {
> > +                rc = ERROR_FAIL;
> 
> ERROR_NOMEM, surely ?
> 

Done.

> > +            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;
> 

I will see what I can do.

Wei.

> Ian.

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