Re: [Xen-devel] [PATCH V5 03/32] xl / libxl: push VCPU affinity pinning down to libxl

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, May 15, 2014 at 05:45:19PM +0100, Ian Campbell wrote:
> On Tue, 2014-05-13 at 22:53 +0100, Wei Liu wrote:
> 
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 0dfafe7..7b0901c 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -305,6 +305,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> >      ("avail_vcpus",     libxl_bitmap),
> >      ("cpumap",          libxl_bitmap),
> >      ("nodemap",         libxl_bitmap),
> > +    ("vcpu_affinity",   libxl_key_value_list),
> > 
> Is a key value list really the best way to represent this? At first
> glance it seems like an array would be more suitable?
> 
> I've glanced through the rest on the assumption you have a convincing
> reason why it should be a kvp list.
> 

How can you effectively skip pinning a VCPU if it's an array? I can have
[ '0': '1', '3': '3' ] in KVL, but not able to represent it in an array
[ '1', ?, ?, '3' ].

> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 661999c..b818815 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -263,6 +263,54 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
> >      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus, &info->cpumap);
> >  
> > +    /* If we have vcpu affinity list, pin vcpu to pcpu. */
> > +    if (d_config->b_info.vcpu_affinity) {
> > +        int i;
> > +        libxl_bitmap vcpu_cpumap;
> > +        int *vcpu_to_pcpu, sz = sizeof(int) * d_config->b_info.max_vcpus;
> > +
> > +        vcpu_to_pcpu = libxl__zalloc(gc, sz);
> 
> In theory this could be a stack allocation, either with alloca or just
>       int vcpu_to_pcpu[sz];
> 

I would rather avoid dynamic-size stack allocation, bacause

"The alloca() function returns a pointer to the beginning of the
allocated space.  If  the  allocation  causes  stack  overflow,
program behavior is undefined."

> > +        memset(vcpu_to_pcpu, -1, sz);
> > +
> > +        for (i = 0; i < d_config->b_info.max_vcpus; i++) {
> > +            libxl_key_value_list kvl = d_config->b_info.vcpu_affinity;
> > +            const char *key, *val;
> > +            int k, v;
> > +
> > +            key = kvl[i * 2];
> 
> Need to bounds check kvl here. I think you might be better off iterating
> over the kvl and validating the k against max_vcpus.
> 

The next line is "bound-checking".

> >      ("numa_placement",  libxl_defbool),
> >      ("tsc_mode",        libxl_tsc_mode),
> >      ("max_memkb",       MemKB),
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index a1cb5b8..3200d40 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -88,9 +88,6 @@ xlchild children[child_max];
> >  static const char *common_domname;
> >  static int fd_lock = -1;
> >  
> > -/* Stash for specific vcpu to pcpu mappping */
> 
> mappppppping.

I'm deleting this line. :-)

> > @@ -833,11 +823,30 @@ static void parse_config_data(const char 
> > *config_source,
> >                  exit(1);
> >              }
> >              libxl_bitmap_set(&b_info->cpumap, i);
> > -            if (n_cpus < b_info->max_vcpus)
> > -                vcpu_to_pcpu[n_cpus] = i;
> > +            if (n_cpus < b_info->max_vcpus) {
> > +                kvl = xrealloc(kvl, sizeof(char *) * (len * 2 + 2));
> 
> Can't you get the length of the input list and just allocate the right
> size to start with?
> 

That introduces extra overhead. Say if you have 32 VCPU but only want to
pin a few.

Wei.

> Ian.

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