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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2840 bytes --]

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?

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

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)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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