[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 for Xen 4.6 3/4] libxl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler
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:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |