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

[Andrii Anisov <andrii.anisov@gmail.com>]

Hello Julien

On 28.10.19 16:52, Julien Grall wrote:
> Hi,
> 
> On 11/09/2019 11:32, Andrii Anisov wrote:
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>> Extend XEN_SYSCTL_getcpuinfo interface with timing information
>> provided by introduced time accounting infrastructure.
>>
>> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
>> ---
>>   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?

Currently, idle time is the idle vcpu run time, what includes tasklets, scheduling, irq processing etc.
IMO it may confuse cpufreq and power management.

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

Yep.

> 
>>   /*
>> 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.

Maybe.

> 
>>               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,
> 

-- 
Sincerely,
Andrii Anisov.

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