On 1/29/20 4:05 AM, Dario Faggioli wrote:
> With Credit2 being Xen default scheduler, it's definitely the case to
> allow Credit2's scheduling parameters to be get and set via libvirt.
Indeed. Thanks for the patch!
> This is easy, as Credit and Credit2 have (at least as of now) the very
> same parameters ('weight' and 'cap'). So we can just let credit2 pass
> the scheduler-type check and the same code will work for both.
>
> Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> ---
> Cc: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> src/libxl/libxl_driver.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index e0a8ec5c24..78ca77d0f6 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -75,6 +75,7 @@ VIR_LOG_INIT("libxl.libxl_driver");
>
> /* Number of Xen scheduler parameters */
> #define XEN_SCHED_CREDIT_NPARAM 2
> +#define XEN_SCHED_CREDIT2_NPARAM 2
Since the number of parameters supported is the same, I would prefer using
XEN_SCHED_CREDIT_NPARAM and delay introducing the new define until credit2
supports more parameters.
> #define LIBXL_CHECK_DOM0_GOTO(name, label) \
> do { \
> @@ -4586,6 +4587,8 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
> break;
> case LIBXL_SCHEDULER_CREDIT2:
> name = "credit2";
> + if (nparams)
> + *nparams = XEN_SCHED_CREDIT2_NPARAM;
> break;
> case LIBXL_SCHEDULER_ARINC653:
> name = "arinc653";
> @@ -4632,11 +4635,11 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
> if (virDomainObjCheckActive(vm) < 0)
> goto cleanup;
>
> + /* Only credit and credit2 are supported for now. */
> sched_id = libxl_get_scheduler(cfg->ctx);
> -
> - if (sched_id != LIBXL_SCHEDULER_CREDIT) {
> + if (sched_id != LIBXL_SCHEDULER_CREDIT && sched_id != LIBXL_SCHEDULER_CREDIT2) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Only 'credit' scheduler is supported"));
> + _("Only 'credit' and 'credit2' schedulers are supported"));
> goto cleanup;
> }
>
> @@ -4657,6 +4660,9 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
> goto cleanup;
> }
>
> + /* Credit and Credit2 have the same number (two) of parameters,
> + * so this is ok for both, at least as long as that stays true. */
> + G_STATIC_ASSERT(XEN_SCHED_CREDIT_NPARAM == XEN_SCHED2_CREDIT_NPARAM);
s/XEN_SCHED2_CREDIT_NPARAM/XEN_SCHED_CREDIT2_NPARAM/
But this hunk can be dropped if using XEN_SCHED_CREDIT_NPARAM for credit and
credit2.
> if (*nparams > XEN_SCHED_CREDIT_NPARAM)
> *nparams = XEN_SCHED_CREDIT_NPARAM;
> ret = 0;
> @@ -4711,9 +4717,11 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom,
>
> sched_id = libxl_get_scheduler(cfg->ctx);
>
> - if (sched_id != LIBXL_SCHEDULER_CREDIT) {
> + /* Only credit and credit2 are supported for now. */
> + sched_id = libxl_get_scheduler(cfg->ctx);
We just retrieved sched_id a few lines above, no need to fetch it again.
> + if (sched_id != LIBXL_SCHEDULER_CREDIT && sched_id != LIBXL_SCHEDULER_CREDIT2) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Only 'credit' scheduler is supported"));
> + _("Only 'credit' and 'credit2' schedulers are supported"));
> goto endjob;
> > }
No need to respin this trivial patch
Reviewed-by: Jim Fehlig<jfehlig@xxxxxxxx>
I've fixed up the minor issues and pushed it.
Regards,
Jim
