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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On ven, 2014-09-05 at 11:45 -0400, Meng Xu wrote:
> 2014-09-05 6:21 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> > On dom, 2014-08-24 at 18:58 -0400, Meng Xu wrote:
> >> --- 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;
> >> +    }
> >>
> > As George pointed out already, you can get the num_vcpus via
> > xc_domain_getinfo().
> >
> > I agree with George's review about the rest of this function
> > (appropriately updated to take what we decided about the interface into
> > account :-) ).
> >
> >> +#define SCHED_RT_VCPU_PERIOD_MAX    31536000000000 /* one year in 
> >> microsecond*/
> >> +#define SCHED_RT_VCPU_BUDGET_MAX    SCHED_RT_VCPU_PERIOD_MAX
> >> +
> > This may be me not remembering correctly the outcome of a preceding
> > discussion... did we say we were going with _RT_ or with something like
> > _RTDS_ ?
> >
> > ISTR the latter...
> 
> So I also need to change all RT to RT_DS in the tool stack, except
> keeping the command 'xl sched-rt'? If so, I will do that.
> 
As I said in another email, the same reasons that called for renaming
inside Xen, applies to the other layers, I think.

I'm not sure about `xl sched-rt', but yes, I think that would be fine
like that. For one, xl interface/commands are not set in stone.
Moreover, when/if we'll have more policies, I think it could be fine to
add a new 'policy' parameter to `xl sched-rt', rather than adding
another `xl sched-xxx' thing.

But then again, we can decide this later.

> >
> > Also, macros like INT_MAX, UINT_MAX, etc., or << and ~ "tricks" are,
> > IMO, preferrable to the open coding of the value.
> 
> What does open coding of the value mean? Do you mean (2^32-1) instead
> of 4294967295?
> 
I mean use INT_MAX, UINT_MAX, or similar, if possible. :-)

> >
> > Finally, I wonder whether these would better live in some headers,
> > closer to the declaration of period and budget (where their type is also
> > visible) and, as a nice side effect of that, available to libxl callers
> > as well.
> 
> I actually considered the possibility of adding it to
> xen/include/public/domctl.h, but other schedulers do not have such
> range macro in the domctl.h, so I'm not sure if it will cause
> inconsistence?
> 
This is toolstack stuff, libxl stuff, to be more precise, so the public
header I'm mentioning is something like tools/libxl/libxl*.h.

> Yes. Now I didn't use any array in toolstack or kernel to set/get
> domain's parameters. So we don't use (budget == 0 && period == 0) to
> indicate the vcpus not changed. (Whenever user set a domain's vcpu's
> parameters, we set all vcpus' parameters of this domain, so we don't
> need such implication for 4.5 release. :-P) The array comes "only"
> when we allow users to set/get a specific vcpu's parameters and vcpus
> have different periods and budgets.
> 
True. I was thinking at something different, but hey, let's see the new
version with the new interface implemented and comment on that! :-)

regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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