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

[Meng Xu <xumengpanda@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2500 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 4673 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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