Re: [Xen-devel] [RFC 2/9] sysctl: extend XEN_SYSCTL_getcpuinfo interface

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 11/09/2019 11:32, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> Extend XEN_SYSCTL_getcpuinfo interface with timing information
> provided by introduced time accounting infrastructure.
>
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>   xen/common/schedule.c       | 33 ++++++++++++++++++++++++++++-----
>   xen/common/sysctl.c         |  4 ++++
>   xen/include/public/sysctl.h |  4 ++++
>   xen/include/xen/sched.h     |  4 ++++
>   4 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 6dd6603..2007034 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -208,13 +208,36 @@ void vcpu_runstate_get(struct vcpu *v, struct 
> vcpu_runstate_info *runstate)
>   
>   uint64_t get_cpu_idle_time(unsigned int cpu)
>   {
> -    struct vcpu_runstate_info state = { 0 };
> -    struct vcpu *v = idle_vcpu[cpu];
> +    struct tacc *tacc = &per_cpu(tacc, cpu);
>   
> -    if ( cpu_online(cpu) && v )
> -        vcpu_runstate_get(v, &state);
> +    return tacc->state_time[TACC_IDLE];
So what's the difference between the current idle time and the new version?

> +}
> +
> +uint64_t get_cpu_guest_time(unsigned int cpu)
> +{
> +    struct tacc *tacc = &per_cpu(tacc, cpu);
> +
> +    return tacc->state_time[TACC_GUEST];
> +}
> +
> +uint64_t get_cpu_hyp_time(unsigned int cpu)
> +{
> +    struct tacc *tacc = &per_cpu(tacc, cpu);
> +
> +    return tacc->state_time[TACC_HYP];
> +}
> +
> +uint64_t get_cpu_irq_time(unsigned int cpu)
> +{
> +    struct tacc *tacc = &per_cpu(tacc, cpu);
> +
> +    return tacc->state_time[TACC_IRQ];
> +}
> +uint64_t get_cpu_gsync_time(unsigned int cpu)
> +{
> +    struct tacc *tacc = &per_cpu(tacc, cpu);
>   
> -    return state.time[RUNSTATE_running];
> +    return tacc->state_time[TACC_GSYNC];
>   }
You may want to introduce an helper retrieving the time for a given state rather 
than duplicating it.
>   
>   /*
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 92b4ea0..b63083c 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -152,6 +152,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>           for ( i = 0; i < nr_cpus; i++ )
>           {
>               cpuinfo.idletime = get_cpu_idle_time(i);
> +            cpuinfo.guesttime = get_cpu_guest_time(i);
> +            cpuinfo.hyptime = get_cpu_hyp_time(i);
> +            cpuinfo.gsynctime = get_cpu_gsync_time(i);
> +            cpuinfo.irqtime = get_cpu_irq_time(i);
It feels to me we want a function that fills up the structure.

>   
>               if ( copy_to_guest_offset(op->u.getcpuinfo.info, i, &cpuinfo, 1) )
>                   goto out;
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 5401f9c..cdada1f 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
As a side note, SYSCTL version will need to be bumped if this wasn't done before 
during the current release.
> @@ -168,6 +168,10 @@ struct xen_sysctl_debug_keys {
>   /* XEN_SYSCTL_getcpuinfo */
>   struct xen_sysctl_cpuinfo {
>       uint64_aligned_t idletime;
> +    uint64_aligned_t hyptime;
> +    uint64_aligned_t guesttime;
> +    uint64_aligned_t irqtime;
> +    uint64_aligned_t gsynctime;
>   };
>   typedef struct xen_sysctl_cpuinfo xen_sysctl_cpuinfo_t;
>   DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpuinfo_t);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 04a8724..8167608 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -876,6 +876,10 @@ void restore_vcpu_affinity(struct domain *d);
>   
>   void vcpu_runstate_get(struct vcpu *v, struct vcpu_runstate_info *runstate);
>   uint64_t get_cpu_idle_time(unsigned int cpu);
> +uint64_t get_cpu_hyp_time(unsigned int cpu);
> +uint64_t get_cpu_guest_time(unsigned int cpu);
> +uint64_t get_cpu_gsync_time(unsigned int cpu);
> +uint64_t get_cpu_irq_time(unsigned int cpu);
>   
>   /*
>    * Used by idle loop to decide whether there is work to do:
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel