Re: [Xen-devel] [PATCH for-4.6 v2 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Aug 14, 2015 at 01:25:45AM +0200, Dario Faggioli wrote:
> On Thu, 2015-08-13 at 16:41 +0100, Wei Liu wrote:
> > ... but allow user to override that check by specifying maxvcpus= in xl
> > configuration file.
> > 
> Ok, from the discussion on v1, and from the subject of this new
> submission, I now see that what you're after the "more vcpus (in a
> single guest) than pcpus" case.
> 
> This is so uncommon, IMO, that it did not even cross my mind while
> looking at v1.. Sorry for this. :-)
> 
> That being said, I agree with IanC's view, as he expressed it during v1
> review, and I find this new version of the patch much better!
> 
> However, although I like the idea, as I said...
> 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 078acd1..5fde8fa 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1202,11 +1202,27 @@ static void parse_vnuma_config(const XLU_Config 
> > *config,
> >      }
> >  
> >      /* User has specified maxvcpus= */
> > -    if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {
> > -        fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
> > -        exit(1);
> > -    } else
> > +    if (b_info->max_vcpus != 0) {
> > +        if (b_info->max_vcpus != max_vcpus) {
> > +            fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
> > +            exit(1);
> > +        }
> > +    } else {
> > +        int host_cpus = libxl_get_online_cpus(ctx);
> > +
> > +        if (host_cpus < 0) {
> > +            fprintf(stderr, "Failed to get online cpus\n");
> > +            exit(1);
> > +        }
> > +
> > +        if (host_cpus < max_vcpus) {
> > +            fprintf(stderr, "xl: vnuma specifies more vcpus than pcpus, "\
> > +                    "use maxvcpus= to override this check.\n");
> >
> ...isn't it too late, when we get to here?
> 
> In fact, if b_info->max_vcpus is 0, the elements of vcpu_parsed are
> sized against the host pcpus, and we risk to call libxl_bitmap_set() for
> vcpus beyond that limit, while parsing the "vcpus" subsection of the
> vnode specification (which happens _before_ this check).
> 
> Or am I missing something?
> 

That's fine because that function has no effect when you try to set a
bit beyond its size.

> Assuming I'm not, it seems to me that a solution could be to check for
> this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
> fact, if "maxvcpus" has not been specified, as soon as the end of one of
> the ranges --as returned by parse_range()-- is beyond host_cpus, we know
> we'd be going past the limit of the corresponding element of
> vcpu_parsed, and we can error out.
> 
> It'll most likely be a bit uglier than this patch, but probably still
> less complex than v1. :-)
> 

That doesn't make any difference in terms of functionality. I would
rather leave the parsing bit as it is and deal with fallout separately.
That would make code cleaner IMHO.

Wei.

> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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