Re: [Xen-devel] [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 13, 2015 at 11:07:47AM +0100, Ian Campbell wrote:
> On Thu, 2015-08-13 at 10:54 +0100, Wei Liu wrote:
> > On Thu, Aug 13, 2015 at 10:42:26AM +0100, Ian Campbell wrote:
> > > On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > > > Originally, if user didn't specify maxvcpus= in xl config file, the
> > > > maximum size of vcpu bitmap was always equal to maximum number of 
> > > > pcpus.
> > > > This might not be what user wants.
> > > 
> > > What are you suggesting they wanted instead? We are only talking about 
> > > the
> > > bitmap right, and the typical/sensible config will have #vcpus <= 
> > > #pcpus,
> > > so they will fit even if they "waste" some bits during parsing.
> > 
> > #vcpus > #pcpus, bitmap is too small.
> 
> Right. Which is trivial to detect as we go through the parsing and raise an
> appropriate error.
> 
> > > I'm almost inclined to suggest that if a user wants #vcpus > #pcpus 
> > > they
> > > should have to specify maxvcpus and not rely on the vnuma parsing code
> > > inferring this fact.
> > > 
> > 
> > I don't think we should prevent people from shooting themselves in the
> > foot.
> 
> If the cost of supporting that is this patch then I disagree. If you can
> find a way to do it simply and cleanly then fine, maybe.
> 

I would say let's make xl dumb. If the maximum number of vcpus specified
in vNUMA doesn't match maximum number of vcpus specified in
configuration file, error out. That would maybe lead to a smaller patch.

This was my first implementation of this parser routine but for some
reason we tried to make xl smart (to sum up vcpus in vNUMA
configuration), and now it's causing us trouble.

> In fact I even disagree in general, we can and should provide warnings or
> errors for things which we know are bad and which are most likely
> unintentional, but provide overrides.
> 
> > > IOW maybe this code could just error out (or print a warning) if this
> > > happens? + a doc update.
> > > 
> > 
> > Xl doesn't complain when you set vcpus > pcpus. I don't think vNUMA
> > should behave differently.
> 
> Not always true, e.g. from vcpuset:
> 
>        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
>             fprintf(stderr, "You are overcommmitting! You have %d physical" \
>                     " CPUs and want %d vCPUs! Aborting, use --ignore-host to" 
> \
>                     " continue\n", host_cpu, max_vcpus);
>             rc = 1;
>         }
> 

That is for vcpuset command, not xl configuration file parser. There is
nothing preventing user from setting #vcpus > #pcpus and no warning
woudl be given. The parser could use some improvement, but that's 4.7
material.

Wei.

> Ian.

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