Re: [PATCH 09/12] tools/xl: add support for setting generic per-cpupool parameters

[Juergen Gross <jgross@suse.com>]

On 26/09/18 19:17, Dario Faggioli wrote:
> On Tue, 2018-09-18 at 08:03 +0200, Juergen Gross wrote:
>> Add a new xl command "cpupool-set-parameters" and cpupool config file
>> support for setting per-cpupool generic parameters.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> Seems good to me. Couple of questions.
> 
> Question one: what about getting (and displaying, I guess in
> cpupoolinfo) the cpupool parameters?
> 
>> --- a/tools/xl/xl_cpupool.c
>> +++ b/tools/xl/xl_cpupool.c
>> @@ -615,6 +625,35 @@ out:
>>      return rc;
>>  }
>>  
>> +int main_cpupoolsetparameters(int argc, char **argv)
>> +{
>> +    int opt;
>> +    const char *pool;
>> +    char *params;
>> +    uint32_t poolid;
>> +
>> +    SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-set-parameters", 2) {
>> +        /* No options */
>> +    }
>> +
>> +    pool = argv[optind++];
>> +    if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid,
>> NULL) ||
>> +        !libxl_cpupoolid_is_valid(ctx, poolid)) {
>> +        fprintf(stderr, "unknown cpupool '%s'\n", pool);
>> +        return EXIT_FAILURE;
>> +    }
>> +
> Since we know that we, AFAIUI, never allow changing the parameters for
> a cpupool with domains in it already, shall we test this here, and bail
> early, with a specific error message, without even trying going down in
> Xen?
> 
> I know it's sort-of duplicating checks with what's in the hypervisor,
> but I think it would be a common mistake, that it's worth trying to
> prevent/address specifically.

That's exactly what the PARAM_FLAG_RUNTIME is meant for. I could think
of parameters which might be changeable even with active domains in the
cpupool. So I wouldn't like to test that in the tools as we would need
to add the knowledge of each parameter to the tools.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel