Re: [Xen-devel] [PATCH v1 3/4] libxl: add rt scheduler

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi George,

2014-09-03 11:33 GMT-04:00 George Dunlap <George.Dunlap@xxxxxxxxxxxxx>:

On Sun, Aug 24, 2014 at 11:58 PM, Meng Xu <mengxu@xxxxxxxxxxxxx> wrote:
> Add libxl functions to set/get domain's parameters for rt scheduler
>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> ---
> tools/libxl/libxl.c    Â| 139 +++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl.h    Â|  7 +++
>Â tools/libxl/libxl_types.idl |Â Â15 +++++
>Â 3 files changed, 161 insertions(+)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 3526539..440e8df31 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5154,6 +5154,139 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
>Â Â Â return 0;
>Â }
>
> +static int sched_rt_domain_get(libxl__gc *gc, uint32_t domid,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlibxl_domain_sched_params *scinfo)
> +{
> +Â Â struct xen_domctl_sched_rt_params* sdom;
> +Â Â uint16_t num_vcpus;
> +Â Â int rc, i;
> +
> +Â Â rc = xc_sched_rt_domain_get_num_vcpus(CTX->xch, domid, &num_vcpus);
> +Â Â if (rc != 0) {
> +Â Â Â Â LOGE(ERROR, "getting num_vcpus of domain sched rt");
> +Â Â Â Â return ERROR_FAIL;
> +Â Â }
> +
> +Â Â /* FIXME: can malloc be used in libxl? seems it was used in the file */
> +Â Â sdom = (struct xen_domctl_sched_rt_params *)
> +Â Â Â Â Â Â malloc( sizeof(struct xen_domctl_sched_rt_params) * num_vcpus );

There are two ways to do allocation; one which will be garbage
collected automatically (so you don't need to call free on it), and
one which returns something which the caller has to remember to free.

You can call libxl__calloc(gc, num_vcpus, sizeof(struct
xen_domctl_sched_rt_params)) with the garbage collector for this one;
then you won't need to free it below, as it will be freed by the
GC_FREE at the end of libxl_domain_params_get(). Then...

âThank you very much for your suggestion! I modified the code as you said after Wei pointed this out. In the next patch, it will use the GC mechanisms to handle the memory allocation.

â

Â

> +Â Â if ( !sdom ){
> +Â Â Â Â LOGE(ERROR, "Allocate sdom array fails\n");
> +Â Â Â Â return ERROR_INVAL;
> +Â Â }
> +
> +Â Â rc = xc_sched_rt_domain_get(CTX->xch, domid, sdom, num_vcpus);
> +Â Â if (rc != 0) {
> +Â Â Â Â LOGE(ERROR, "getting domain sched rt");
> +Â Â Â Â return ERROR_FAIL;
> +Â Â }
> +
> +Â Â /* FIXME: how to guarantee libxl_*_dispose be called exactly once? */
> +Â Â libxl_domain_sched_params_init(scinfo);
> +
> +Â Â scinfo->rt.num_vcpus = num_vcpus;
> +Â Â scinfo->sched = LIBXL_SCHEDULER_RT;
> +Â Â /* FIXME: can malloc be used in libxl? seems it was used in the file */
> +Â Â scinfo->rt.vcpus = (libxl_vcpu *)
> +Â Â Â Â Â Â Â Â Â Â Â Âmalloc( sizeof(libxl_vcpu) * scinfo->rt.num_vcpus );

...for this one, call libxl__calloc(NOGC, scinfo->rt.num_vcpus,
sizeof(libxl_vcpu)). NOGC will cause the values not to be freed by
GC_FREE (so they should be freed by the caller).

And again, as Wei said, you don't need to check the return value for
either one, as libxl will handle memory allocation errors.

ââ

âYes, next release will change as you and Wei said. (I have actually changed it for the next release. :-)â )

Â

However, it looks like libxl_list_domain() managed to make the libxl
struct look exactly like the libxc struct, so that you don't need to
do this allocate-and-copy thing. Could you try to arrange for that to
be the case for libxl_rt_vcpu?

âDo you mean that I pass the libxl_rt_vcpu structure as the parameter for the function Âxc_sched_rt_domain_get(CTX->xch, domid, sdom, num_vcpus), i.e., replace the sdom with the libxl_rt_vcpu data. Then I can avoid copying from the libxc data sdom to the libxl_rt_vcpu?Â

âIf so, I think it's doable and will change it.â

Â

> +/*
> + * Sanity check of the scinfo parameters
> + * return 0 if all values are valid
> + * return 1 if one param is default value
> + * return 2 if the target vcpu's index, period or budget is out of range
> + */

These should be checked in the hypervisor, not the toolstack. The
hypervisor can return -EINVAL, and libxl can pass that error message
on to the caller.

âI saw the toolstack for credit and credit2 scheduler also check the validity of the new parameters. That's why I check the validity of the parameters at the toolstack.Â

For example, in function sched_credit_domain_set() @ file tools/libxl/libxl.c does the check as follows:

 ÂÂif (scinfo->weight != LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT) {

    if (scinfo->weight < 1 || scinfo->weight > 65535) {

      LOG(ERROR, "Cpu weight out of range, "

        "valid values are within range from 1 to 65535");

      return ERROR_INVAL;

    }

    sdom.weight = scinfo->weight;

  }

Just to confirm, should I not follow what credit does in the libxl.c and check the validity of the parameters in hypervisor as you suggested? Please bear with me about this question because I want to make sure I'm heading to the correct direction. :-)

So this function should go away; but in general, just as a style note,
you should avoid "magic constants" like this. Ideally you'd re-use
some of the LIBXL error codes; but if that won't work, you should make
#define's with a suitable descriptive name.

âGot it! Thank you very much! This is my bad and I will change it. :-( â

Â

> +static int sched_rt_domain_set_validate_params(libxl__gc *gc,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst libxl_domain_sched_params *scinfo,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst uint16_t num_vcpus)
> +{
> +Â Â int vcpu_index = scinfo->rt.vcpu_index;
> +
> +Â Â if (vcpu_index == LIBXL_DOMAIN_SCHED_PARAM_VCPU_DEFAULT ||
> +Â Â Â Â scinfo->rt.period == LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT ||
> +Â Â Â Â scinfo->rt.budget == LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
> +Â Â {
> +Â Â Â Â return 1;
> +Â Â }
> +
> +Â Â if (vcpu_index < 0 || vcpu_index > num_vcpus)
> +Â Â {
> +Â Â Â Â LOG(ERROR, "VCPU index is not set or out of range, "
> +Â Â Â Â Â Â Â Â Â Â "valid values are within range from 0 to %d", num_vcpus);
> +Â Â Â Â return 2;
> +Â Â }
> +
> +Â Â if (scinfo->rt.period < 1 ||
> +Â Â Â Â scinfo->rt.period > SCHED_RT_VCPU_PERIOD_MAX)
> +Â Â {
> +Â Â Â Â LOG(ERROR, "VCPU period is not set or out of range, "
> +Â Â Â Â Â Â Â Â Â Â "valid values are within range from 0 to %lu", SCHED_RT_VCPU_PERIOD_MAX);
> +Â Â Â Â return 2;
> +Â Â }
> +
> +Â Â if (scinfo->rt.budget < 1 ||
> +Â Â Â Â scinfo->rt.budget > SCHED_RT_VCPU_BUDGET_MAX)
> +Â Â {
> +Â Â Â Â LOG(ERROR, "VCPU budget is not set or out of range, "
> +Â Â Â Â Â Â Â Â Â Â "valid values are within range from 0 to %lu", SCHED_RT_VCPU_BUDGET_MAX);
> +Â Â Â Â return 2;
> +Â Â }
> +
> +Â Â return 0;
> +
> +}
> +
> +static int sched_rt_domain_set(libxl__gc *gc, uint32_t domid,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst libxl_domain_sched_params *scinfo)
> +{
> +Â Â struct xen_domctl_sched_rt_params sdom;
> +Â Â uint16_t num_vcpus;
> +Â Â int rc;
> +
> +Â Â rc = xc_sched_rt_domain_get_num_vcpus(CTX->xch, domid, &num_vcpus);
> +Â Â if (rc != 0) {
> +Â Â Â Â LOGE(ERROR, "getting domain sched rt");
> +Â Â Â Â return ERROR_FAIL;
> +Â Â }
> +
> +Â Â rc = sched_rt_domain_set_validate_params(gc, scinfo, num_vcpus);
> +Â Â if (rc == 2)
> +Â Â Â Â return ERROR_INVAL;
> +Â Â if (rc == 1)
> +Â Â Â Â return 0;

Er, one of the parameters is the default value, and so you don't set
any of them?

âHmm, yes, users should specify the vcpu index, the new period and the new budget to set a vcpu's parameter. They have to give us the index of the vcpu to set, otherwise, we don't know which vcpu the new parameters are set to. :-)Â

âWe can definitely allow users to only specify period or budget to just set the period or budget. If you think that's a better way to do it. I can modify it. â:-)

Â

> @@ -303,6 +304,19 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
>Â Â Â ("checkpointed_stream", integer),
>Â Â Â ])
>
> +libxl_rt_vcpu = Struct("vcpu",[
> +  ("period",   Âuint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> +  ("budget",   Âuint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +  ("index",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> +Â Â ])
> +
> +libxl_domain_sched_rt_params = Struct("domain_sched_rt_params",[
> +  ("vcpus",    Array(libxl_rt_vcpu, "num_vcpus")),
> +  ("period",   Âuint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
> +  ("budget",   Âuint64, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +  ("vcpu_index", Âinteger, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
> +Â Â ])
> +
>Â libxl_domain_sched_params = Struct("domain_sched_params",[
>   ("sched",    libxl_scheduler),
>   ("weight",   Âinteger, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
> @@ -311,6 +325,7 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
>   ("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'}),
> +  ("rt",     Âlibxl_domain_sched_rt_params),
>Â Â Â ])

While the domctl interface is not stable, the libxl interface *is*
stable, so we definitely need to think carefully about what we want
this to look like.

Let me give that a think. :-)

âSure! â I totally agree! When every one agrees with the interface, I can modify it accordingly very quickly. The d

ifficult thing is the consensus of how the interface should look like :-P

âThank you very much for your comments and suggestions! â

âMengâ

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel