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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jan 28, 2015 at 04:27:29PM +0000, Ian Campbell wrote:
> On Fri, 2015-01-23 at 11:13 +0000, Wei Liu wrote:
> > +    /* 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];
> > +
> > +        GCREALLOC_ARRAY(v, i+1);
> 
> Can we not just allocate it outside the loop using
> b_info->num_vnuma_nodes?
> 

Yes.

> > +
> > +        v[i].start = next;
> > +        v[i].end = next + (p->memkb << 10);
> > +        v[i].flags = 0;
> > +        v[i].nid = i;
> > +
> > +        next = v[i].end;
> > +    }
> > +
> > +    state->vmemranges = v;
> > +    state->num_vmemranges = i;
> > +
> > +    return libxl__arch_vnuma_build_vmemrange(gc, domid, b_info, state);
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> > index d012b4d..4841fef 100644
> > --- a/tools/libxl/libxl_x86.c
> > +++ b/tools/libxl/libxl_x86.c
> > @@ -339,6 +339,82 @@ int 
> > libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
> >      return 0;
> >  }
> >  
> > +int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
> 
> This isn't so much "build" as "adjust" or "make it fit arch policy" or
> something?
> 

"adjust" is better.

> > +                                      uint32_t domid,
> > +                                      libxl_domain_build_info *b_info,
> > +                                      libxl__domain_build_state *state)
> > +{
> > +    int nid, nr_vmemrange, rc;
> > +    uint32_t nr_e820, e820_count;
> > +    struct e820entry map[E820MAX];
> > +    xen_vmemrange_t *vmemranges;
> > +
> > +    /* Only touch vmemranges if it's PV guest and e820_host is true */
> 
> I take it that obeying e820_host here is only WRT where the memory is
> placed, not also trying to match the underlying hosts numa layout?
> 

It's used to set aside memory holes according to host e820 map. It
doesn't take into account host numa layout.

> > +    if (!(b_info->type == LIBXL_DOMAIN_TYPE_PV &&
> > +          libxl_defbool_val(b_info->u.pv.e820_host))) {
> > +        rc = 0;
> > +        goto out;
> > +    }
> > +
> > +    rc = e820_host_sanitize(gc, b_info, map, &nr_e820);
> > +    if (rc) goto out;
> > +
> > +    /* Ditch old vmemranges and start with host E820 map. Note, memory
> > +     * was gc allocated.
> 
> This makes me thing something is backwards in the layering here.
> 
> Wouldn't it make more sense for libxl__arch_vnuma_build_vmemrange to be
> the entry point which starts with:
>     if (nothing unusual like e820_host going on)
>         return libxl__vnuma_build_pv_generic(...)
> 
>     ...the special stuff.
> 

OK. That's fine by me.

Wei.

> > +    e820_count = 0;
> > +    nr_vmemrange = 0;
> > +    vmemranges = NULL;
> > +    for (nid = 0; nid < b_info->num_vnuma_nodes; nid++) {
> 
> This fill-loop looked ok to me.
> 

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