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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-05-16 at 11:15 +0100, Wei Liu wrote:
> On Fri, May 16, 2014 at 10:57:59AM +0100, Ian Campbell wrote:
> > 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?
> > 
> 
> Yes, but then that's not very efficient if you only want to pin a few
> vcpu, say, if you have 128 vcpu but only want to pin several.

I'm not sure how worried I am about that in the context of libxl -- Ian,
what do you think?

> > > > > +        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).
> > 
> 
> We haven't run off the end. The last single element of a KV list is
> sentinel.

So it is, sorry I misread.



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