Re: [Xen-devel] [PATCH v1 1/4] xen: add real time scheduler rt

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Sun, Aug 24, 2014 at 11:58 PM, Meng Xu <mengxu@xxxxxxxxxxxxx> wrote:
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 5b11bbf..27d01c1 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -339,6 +339,19 @@ struct xen_domctl_max_vcpus {
>  typedef struct xen_domctl_max_vcpus xen_domctl_max_vcpus_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
>
> +/*
> + * This structure is used to pass to rt scheduler from a
> + * privileged domain to Xen
> + */
> +struct xen_domctl_sched_rt_params {
> +    /* get vcpus' info */
> +    uint64_t period; /* s_time_t type */
> +    uint64_t budget;
> +    uint16_t index;
> +    uint16_t padding[3];

Why the padding?

> +};
> +typedef struct xen_domctl_sched_rt_params xen_domctl_sched_rt_params_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_sched_rt_params_t);
>
>  /* XEN_DOMCTL_scheduler_op */
>  /* Scheduler types. */
> @@ -346,9 +359,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
>  #define XEN_SCHEDULER_CREDIT   5
>  #define XEN_SCHEDULER_CREDIT2  6
>  #define XEN_SCHEDULER_ARINC653 7
> +#define XEN_SCHEDULER_RT_DS    8
> +
>  /* Set or get info? */
> -#define XEN_DOMCTL_SCHEDOP_putinfo 0
> -#define XEN_DOMCTL_SCHEDOP_getinfo 1
> +#define XEN_DOMCTL_SCHEDOP_putinfo      0
> +#define XEN_DOMCTL_SCHEDOP_getinfo      1
> +#define XEN_DOMCTL_SCHEDOP_getnumvcpus   2
>  struct xen_domctl_scheduler_op {
>      uint32_t sched_id;  /* XEN_SCHEDULER_* */
>      uint32_t cmd;       /* XEN_DOMCTL_SCHEDOP_* */
> @@ -367,6 +383,16 @@ struct xen_domctl_scheduler_op {
>          struct xen_domctl_sched_credit2 {
>              uint16_t weight;
>          } credit2;
> +        struct xen_domctl_sched_rt{
> +            /* get vcpus' params */
> +            XEN_GUEST_HANDLE_64(xen_domctl_sched_rt_params_t) vcpu;
> +            uint16_t nr_vcpus;
> +            /* set one vcpu's params */
> +            uint16_t vcpu_index;
> +            uint16_t padding[2];

And again, why the padding?  This isn't a performance-critical bit of
code: you can safely let the compiler deal with adding padding to the
structure or managing mis-aligned reads.  Or if it really matters to
you, you can re-order the elements of the array so that they're
aligned naturally (e.g., by putting period and budget before
nr_vcpus).

> +            uint64_t period;
> +            uint64_t budget;
> +        } rt;

So if I'm reading this right, you set the information for vcpus one
vcpu at a time, but you want to read the whole lot out all at once?

I don't like the inconsistency.  It would be better if you did the
same thing each direction:  Either pass in an array with info about
the vcpus, or just read the vcpu information one-by-one.

 -George

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