[PATCH] libxl: support getting and setting parameters for the Credit2

[Jim Fehlig]

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 at suse.com>
> ---
> Cc: Jim Fehlig <jfehlig at suse.com>
> ---
>   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 at suse.com>

I've fixed up the minor issues and pushed it.

Regards,
Jim