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

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi George,

2014-09-03 10:20 GMT-04:00 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?

I did this because of Jan's comment "Also, you need to pad the structure to a multiple of 8 bytes, or
its layout will differ between 32- and 64-bit (tool stack) callers." I think what he said make sense so I added the padding here. :-)

Here is the link:â http://marc.info/?l=xen-devel&m=140661680931179&w=2â

Â

> +};
> +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).

I agree that this is not a performance-critical bit of code. âI can do either way as you suggested.Â

âIf I remove the padding, it means the structure won't be a multiply of 8 bytes and its layout will differ between 32- and 64-bit (tool stack) callers. If that's fine, I'm totally fine with it. Just need consensus. ;-)Â

Â

> +Â Â Â Â Â Â 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?

âYes. â

Â

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.

âI think it's a better idea to

Âpass in an array with information about vcpus to get/set vcpus' information.

âÂ

I only need to change the code related to setting a vcpu's information.

âI have a question:â

âWhen we set a vcpu's information by using an array, we have two choices:

a) just create an array with one vcpu element, and specify the index of the vcpu to modify; The concern to this method is that we only uses one element of this array, so is it a good idea to use an array with only one element?

âb) create an array with all vcpus of this domain, modify the parameters of the vcpu users want to change, and then bounce the array to hypervisor to reset these vcpus' parameters.â The concern to this method is that we don't need any other vcpus' information to set a specific vcpu's parameters. Bouncing the whole array with all vcpus information seems expensive and unnecessary?

âDo you have any suggestion/advice/preference on this?â

âI don't really like about the idea of reading the vcpu's information one-by-oneâ. :-) If a domain has many vcpus, say 12 vcpus, we will issue 12 hypercalls to get all vcpus' information of this domain. Because we only need to issue one hypercall to get all information we want, the extra hypercalls causes more overhead. This did simplify the implementation, but may cause more overhead.Â

Thank you very much!

â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