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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-08-13 at 11:28 +0100, Wei Liu wrote:
> 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.

I disagree that xl should be dumb. It is perfectly fine for xl to do
convenient things automatically while still detecting things which are
likely to be incorrect or accidental or not handling all the strange edge
cases automatically (so long as they are possible manually).

Inferring #vcpus from vnuma config is a useful thing to do automatically,
but that doesn't preclude erroring out on edge cases.


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


I did say "not always true" rather than "not true".

>  There is
> nothing preventing user from setting #vcpus > #pcpus and no warning
> woudl be given.

A bug IMHO.

>  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