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

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.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> ---
>  tools/libxl/libxl.c         | 143 +++++++++++++++++++++++++++++++++-----------
>  tools/libxl/libxl.h         |   6 ++
>  tools/libxl/libxl_types.idl |  11 ++++
>  3 files changed, 126 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index feb3aa9..5f66753 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5800,10 +5800,17 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
>  static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
>                                 libxl_domain_sched_params *scinfo)
>  {
> -    struct xen_domctl_sched_rtds sdom;
> -    int rc;
> -
> -    rc = xc_sched_rtds_domain_get(CTX->xch, domid, &sdom);
> +    uint16_t num_vcpus;
> +    int rc, i;
> +    xc_dominfo_t info;
>
Coding style: we want a blank line between variable definitions and
actual code.

> +    rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +    if (rc < 0) {
> +        LOGE(ERROR, "getting domain info");
> +        return ERROR_FAIL;
> +    }
> +    num_vcpus = info.nr_online_vcpus;
>
It looks like the most of other places in libxl where this is necessary
use libxl_list_vcpu(), which, if you want to open code it, uses
info.max_vcpu_id. I'd do the same.

> +    struct xen_domctl_sched_rtds_params  sdom[num_vcpus];
>
And, please, all the variable definitions go together, at the top of the
function. Avoid having them scattered around the body.

I see why this is here, but no, that's not a good reason... For
allocating arrays in libxl, have a look at GCNEW_ARRAY, or
libxl__calloc, libxl__malloc, libxl__realloc, etc.

> +    rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, sdom, num_vcpus);
>      if (rc != 0) {
>          LOGE(ERROR, "getting domain sched rtds");
>          return ERROR_FAIL;

> @@ -5821,43 +5835,104 @@ static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
>  static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
>                                 const libxl_domain_sched_params *scinfo)
>  {
> -    struct xen_domctl_sched_rtds sdom;
>      int rc;
> +    int i;
>  
> -    rc = xc_sched_rtds_domain_get(CTX->xch, domid, &sdom);
> -    if (rc != 0) {
> -        LOGE(ERROR, "getting domain sched rtds");
> -        return ERROR_FAIL;
> -    }
> +    if(scinfo->rtds.num_vcpus <= 0) {/*set per-dom rtds parameters*/
>
The comment is better before or after, rather than inline.

But, really, can you define another helper function and call it, instead
than re-indenting everything and making this one much more long and
complex?

Oh, and the error checking and error reporting code should be factored
and shared, not duplicated.

I mean this:

> +        if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> +            if (scinfo->period < 1) {
> +                LOG(ERROR, "VCPU period is not set or out of range, "
> +                           "valid values are larger than 1");
> +                return ERROR_INVAL;
> +            }
> +            sdom.period = scinfo->period;
> +        }

And this:
> +            if (period < 1) {
> +                LOG(ERROR, "VCPU period is out of range, "
> +                           "valid values are larger than 1");
> +                return ERROR_INVAL;
> +            }
>
And the same for every other one.

Regards,
Dario