Re: [PATCH v1 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

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

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

[Adjusting the Cc list:
  - removing hypervisor only people
  - adding more tools maintainers
  - adding George]

On Thu, 2015-05-07 at 12:05 -0500, Chong Li wrote:
> Change sched_rtds_domain_get/set functions to support per-VCPU settings for RTDS scheduler.
> 
More on this patch (I had to run yesterday).

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 44bd8e2..8284ce1 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h

> +/* Consistent with XEN_LEGACY_MAX_VCPUS xen/arch-x86/xen.h*/
> +#define LIBXL_XEN_LEGACY_MAX_VCPUS                  32
> +
Kill this. Nothing good can possibly come from relying on things like
this (i.e., copying constants --and even worse arch specific ones, in
this case!-- around).

>  int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
>                                    libxl_domain_sched_params *params);
>  int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 117b61d..806316a 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -347,6 +347,16 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
>      ("checkpointed_stream", integer),
>      ])
>  
> +libxl_rtds_vcpu = Struct("vcpu",[
>
                     ^Struct("rtds_vcpu",[

> +    ("period",       uint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> +    ("budget",       uint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +    ("index",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
>
Call this last member vcpuid, if you want to be able to pass a
sparse/incomplete array, and hence you need to know to what vcpu each
element refers to, or just get rid of it, it you always pass all the
elements.

I'd go with the former.

> +    ])
> +
> +libxl_domain_sched_rtds_params = Struct("domain_sched_rtds_params",[
>
I'd call this something like "vcpus_sched_rtds_params". Well, actually,
I don't think this is needed at all (see below).

> +    ("vcpus",        Array(libxl_rtds_vcpu, "num_vcpus")),
> +    ])
> +
>  libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("sched",        libxl_scheduler),
>      ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
> @@ -356,6 +366,7 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("latency",      integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
>      ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>      ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +    ("rtds",             libxl_domain_sched_rtds_params),  
>      ])
>  
I've got the same concerns I've already expressed about the hypervisor
interface. Doing things like this, we will have
libxl_domain_sched_params that hosts both per-domain and per-vcpu
parameters, without a clear way to know what happens if one specifies
both.

Also, if at some point other schedulers want to support per-vcpu
parameters, the said information duplication (and ambiguity) will apply
to them too!

Finally, I don't think I would call the way the result returned by a
call to libxl_domain_sched_params_{get,set}() changes backward
compatible, which OTOH is something we want to retain in libxl.

So, if API compatibility/stability wasn't an issue, fiddling with
libxl_domain_sched_params would probably be the best solution. Since it
is, I'd leave that alone as much as possible, and introduce something
completely new, for dealing with per-vcpu parameters. Something like
this:

typedef struct libxl_vcpu_sched_params {
    libxl_domain_sched_params vcpus[];
    int num_vcpus;
} libxl_vcpu_sched_params;

[*]

And I'll deal with this in new API functions:

int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
                                libxl_domain_vcpu_params *params);
int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
                                const libxl_vcpu_sched_params *params);

At this point, if, say, Credit2 wants to start supporting per-vcpu
scheduling parameters, we're all set for that!

What do the tools' maintainers think?

The only thing I'm not sure about, in the described scenario, is what a
call to libxl_domain_sched_params_get() should return for a scheduler
which supports per-vcpu parameters, and actually has different
scheduling parameters for the various vcpus...

I mean, a call to libxl_domain_sched_params_set() can be interpreted
like 'set the passed params for all the vcpus', _get(), I don't know...
perhaps it should return some sort of ERROR_INVAL, to inform the caller
that it is libxl_vcpu_sched_params_get() that should be used?

Again, tools maintainers? :-)

Oh, and in any case, you need to mark that something is being added to
the libxl API using the '#define LIBXL_HAVE_*' mechanism. Look for
examples in libxl.h, there are several. If going for my proposal, I'd
call the constant LIBXL_HAVE_VCPU_SCHED_PARAMS.

Regards,
Dario

[*] I don't like the fact that the vcpus[] array is of
'libxl_domain_sched_params' type, especially the *domain* part of the
type name, but I don't think we can change that, for the said API
stability reasons.

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