Re: [PATCH v4 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting

[Wanpeng Li <kernellwp@gmail.com>]

2016-06-07 18:47 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/06/2016 10:00, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> This patch adds guest steal-time support to full dynticks CPU
>> time accounting. After the following commit:
>>
>> ff9a9b4c4334 ("sched, time: Switch VIRT_CPU_ACCOUNTING_GEN to jiffy granularity")
>>
>> ... time sampling became jiffy based, even if it's still listened
>> to ring boundaries, so steal_account_process_tick() is reused
>> to account how many 'ticks' are stolen-time, after the last accumulation.
>
> I still have no idea how to parse this.  What are "ring boundaries"?
> Rik, can you suggest a better commit message?
>
>> Suggested-and-Reviewed-by: Rik van Riel <riel@redhat.com>
>
> Please split Suggested-by and Reviewed-by.
>
>> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
>> index 75f98c5..9ff036b 100644
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -257,7 +257,7 @@ void account_idle_time(cputime_t cputime)
>>               cpustat[CPUTIME_IDLE] += (__force u64) cputime;
>>  }
>>
>> -static __always_inline bool steal_account_process_tick(void)
>> +static __always_inline unsigned long steal_account_process_tick(void)
>>  {
>>  #ifdef CONFIG_PARAVIRT
>>       if (static_key_false(&paravirt_steal_enabled)) {
>> @@ -279,7 +279,7 @@ static __always_inline bool steal_account_process_tick(void)
>>               return steal_jiffies;
>>       }
>>  #endif
>> -     return false;
>> +     return 0;
>>  }
>>
>>  /*
>> @@ -691,9 +691,13 @@ static cputime_t get_vtime_delta(struct task_struct *tsk)
>>
>>  static void __vtime_account_system(struct task_struct *tsk)
>>  {
>> -     cputime_t delta_cpu = get_vtime_delta(tsk);
>> +     cputime_t delta_time = get_vtime_delta(tsk);
>> +     cputime_t steal_time = jiffies_to_cputime(steal_account_process_tick());
>>
>> -     account_system_time(tsk, irq_count(), delta_cpu, cputime_to_scaled(delta_cpu));
>> +     if (steal_time < delta_time) {
>> +             delta_time -= steal_time;
>> +             account_system_time(tsk, irq_count(), delta_time, cputime_to_scaled(delta_time));
>> +     }
>>  }
>>
>>  void vtime_account_system(struct task_struct *tsk)
>> @@ -718,13 +722,18 @@ void vtime_gen_account_irq_exit(struct task_struct *tsk)
>>
>>  void vtime_account_user(struct task_struct *tsk)
>>  {
>> -     cputime_t delta_cpu;
>> +     cputime_t delta_time, steal_time;
>>
>>       write_seqcount_begin(&tsk->vtime_seqcount);
>>       tsk->vtime_snap_whence = VTIME_SYS;
>>       if (vtime_delta(tsk)) {
>> -             delta_cpu = get_vtime_delta(tsk);
>> -             account_user_time(tsk, delta_cpu, cputime_to_scaled(delta_cpu));
>> +             delta_time = get_vtime_delta(tsk);
>> +             steal_time = jiffies_to_cputime(steal_account_process_tick());
>> +
>> +             if (steal_time < delta_time) {
>> +                     delta_time -= steal_time;
>> +                     account_user_time(tsk, delta_time, cputime_to_scaled(delta_time));
>> +             }
>>       }
>>       write_seqcount_end(&tsk->vtime_seqcount);
>>  }
>>
>
> You're adding almost the same code to two callers of get_vtime_delta out
> of three.  I don't know the vtime accounting code very well, but why
> doesn't the same apply to account_idle_time?

St stuff is accounted when vCPUs(tasks on host) are enqueued in rb
trees of pCPUs, which means that they are ready to run until they
finially reach CPUs. However, when vCPUs are idle, they will be
dequeued from rb trees and the time will be not accounted as st.

>
> If it does, you should instead change get_vtime_delta to process steal
> time and subtract it from the result.
>
> Secondarily, when can it happen that steal_time > delta_time?

Rik explanation it when he reply to v1.
http://www.gossamer-threads.com/lists/linux/kernel/2441175?do=post_view_threaded#2441175

Regards,
Wanpeng Li