Re: [PATCH v2 for Xen 4.6 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: 5683 bytes --]

On Fri, 2015-06-05 at 12:37 +0100, Ian Campbell wrote:
> On Mon, 2015-05-25 at 19:09 -0500, Chong Li wrote:

> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 117b61d..d28d274 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -347,6 +347,17 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
> >      ("checkpointed_stream", integer),
> >      ])
> >  
> > +libxl_rtds_vcpu = Struct("rtds_vcpu",[
> > +    ("period",       uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> > +    ("budget",       uint32, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> > +    ("vcpuid",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> > +    ])
> > +
> > +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> > +    ("sched",        libxl_scheduler),
> > +    ("vcpus",        Array(libxl_rtds_vcpu, "num_vcpus")),
> 
> How will we handle a second scheduler with per-vcpu parameters being
> added in the future without breaking the API?
> 
Yeah, this is an issue here, as it is at the hypercall level. In my own
review of this patch, I said "do something similar to the suggested
hypervisor interface".

Let me try to be a bit more specific.

Ideally, I'd like this to look sort of as follows (let's call this
'option 0'):

libxl_sched_params = Struct("sched_params",[
    ("weight",       integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
    ("cap",          integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
    ("period",       integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
    ("slice",        integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
    ("latency",      integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
    ("extratime",    integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
    ("budget",       integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
    ])

libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
    ("sched",        libxl_scheduler),
    ("vcpus",        Array(libxl_sched_params, "num_vcpus")),
    ])

libxl_domain_sched_params = Struct("domain_sched_params",[
    ("sched",        libxl_scheduler),
    ("schedparams",  libxl_sched_params),
    ])

But this would mean breaking the API too much, I guess (see
libxl_domain_sched_params being completely redefined, the LIBXL_*_PARAMS
constants being changed, etc.)

So, I think I'd be fine with something like this (let's call this
'option 1'):

libxl_domain_sched_params = Struct("domain_sched_params",[  //left right as it is now!!
    ("sched",        libxl_scheduler),
    ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
    ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
    ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
    ("slice",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
    ("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'}),
    ])

libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
    ("vcpus",        Array(libxl_domain_sched_params, "num_vcpus")),
    ])

The mix of domain and vcpu in the type (and hence) function names may
not appear ideal, but it's certainly not meaningless nor incorrect:
these are scheduling parameters for a domain (as opposed to parameters
global to a scheduler, like the ones in libxl_sched_credit_params), and
are (now) applied at a per-vcpu level for such domain.

There is some amount of redundancy, as the sched field, of
libxl_scheduler type, is repeated for all vcpus, but it's not possible
to use different schedulers for different vcpus, so it'll have to always
be the same, or just be unused, which is indeed rather ugly an API.

Alternatevily we can just add the per-vcpu stuff (as in 'option 0'), for
all schedulers, and really leave libxl_domain_sched_params alone (let's
call this 'option 2'):

libxl_sched_params = Struct("sched_params",[
    ("weight",       integer, {'init_val': 'LIBXL_PARAM_WEIGHT_DEFAULT'}),
    ("cap",          integer, {'init_val': 'LIBXL_PARAM_CAP_DEFAULT'}),
    ("period",       integer, {'init_val': 'LIBXL_PARAM_PERIOD_DEFAULT'}),
    ("slice",        integer, {'init_val': 'LIBXL_PARAM_SLICE_DEFAULT'}),
    ("latency",      integer, {'init_val': 'LIBXL_PARAM_LATENCY_DEFAULT'}),
    ("extratime",    integer, {'init_val': 'LIBXL_PARAM_EXTRATIME_DEFAULT'}),
    ("budget",       integer, {'init_val': 'LIBXL_PARAM_BUDGET_DEFAULT'}),
    ])

libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
    ("sched",        libxl_scheduler),
    ("vcpus",        Array(libxl_sched_params, "num_vcpus")),
    ])

In this case the redundancy comes from having libxl_domain_sched_params
and libxl_sched_params looking a lot similar, but not shared code in
declaring them.

Maybe we can also consider putting an union inside
libl_domain_sched_params... but again, quite a severe breakage, and I
wouldn't be sure about how to 'key it'...

So, Thoughts? What do you think the best way forward could be?

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