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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-15 at 18:06 +0100, Wei Liu wrote:
> 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' ].

Isn't there an explicit value for any?

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

I think that's true regardless of how the stack is overflowed, be it an
allocation, an oversized array or a deep call chain. But I don't think
we are talking about quantities of data which are unreasonable to put on
the stack, even with thousands of guest CPUs?

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

The it is too late, you've already run off the end of kvl. (I'm talking
about the bounds of i*2, not the bounds of the resulting key BTW).

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

is that something to worry about in the context of a userspace library
interface?


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