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

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi Wei,

Thank you very much for your quick review! I really appreciate it. :-)

In summary, I correct those code you pointed out.Â

Â

> +Â Â Â Â 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);
> +

libxl_*_dispose should be idempotent (as least we intent to make it so).
And it's caller's responsibility to ensure it's called once if it wishes
to.

âI see. Right now, it is disposed by the caller in xl.â

> +Â Â if (vcpu_index < 0 || vcpu_index > num_vcpus)
> +Â Â {

Libxl coding style normally has { in the same line of "if" "for" etc. Please
check and fix other occurences.

âA quick question:Â

Should I use the style "if () { " for all files in the tool stack or just in libxl files?

If I need to use this style in all tool stack files, I will check and modify them. (Right now, I just made the correction for this exact patch.)

Â

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index bfeb3bc..4657056 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1240,6 +1240,13 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
>Â #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULTÂ Â-1
>Â #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
>
> +#define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULTÂ Â Â-1
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_DEFAULTÂ Â Â Â-1
> +#define LIBXL_DOMAIN_SCHED_PARAM_NUM_VCPUS_DEFAULTÂ -1
> +#define LIBXL_DOMAIN_SCHED_PARAM_VCPU_INDEX_DEFAULT -1
> +/* Consistent with XEN_LEGACY_MAX_VCPUS xen/arch-x86/xen.h*/
> +#define LIBXL_XEN_LEGACY_MAX_VCPUSÂ Â Â Â Â Â Â Â Â 32
> +

Where is this macro used? I cannot find it in this one and following xl
patch.

âMy bad. This should exist any more since we dynamically allocate vcpus structure based on the number of vcpus in a domain to display vcpus' information. Â

Delete it now.Â

Thank you again for your useful comments, Wei!Â

Best,

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