Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1415983 > unrolled thread

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

Started byPaolo Bonzini <pbonzini@redhat.com>
First post2016-06-07 12:50 +0200
Last post2016-06-07 14:40 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/3] sched/cputime: Add steal time support to full  dynticks CPU time accounting Paolo Bonzini <pbonzini@redhat.com> - 2016-06-07 12:50 +0200
    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 13:30 +0200
    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 14:20 +0200
      Re: [PATCH v4 3/3] sched/cputime: Add steal time support to full  dynticks CPU time accounting Paolo Bonzini <pbonzini@redhat.com> - 2016-06-07 14:40 +0200

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

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-07 12:50 +0200
SubjectRe: [PATCH v4 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
Message-ID<rHmK6-1z7-21@gated-at.bofh.it>

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?

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?

Thanks,

Paolo

[toc] | [next] | [standalone]

#1416044

FromWanpeng Li <kernellwp@gmail.com>
Date2016-06-07 13:30 +0200
Message-ID<rHnmO-229-15@gated-at.bofh.it>
In reply to#1415983
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?

It is original from this slides.
http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf,
slide 28.

>
>> 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?
>
> 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?
>
> Thanks,
>
> Paolo



-- 
Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1416111

FromWanpeng Li <kernellwp@gmail.com>
Date2016-06-07 14:20 +0200
Message-ID<rHo9c-2Ai-11@gated-at.bofh.it>
In reply to#1415983
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

[toc] | [prev] | [next] | [standalone]

#1416126

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-07 14:40 +0200
Message-ID<rHosx-2GV-23@gated-at.bofh.it>
In reply to#1416111

On 07/06/2016 14:15, Wanpeng Li wrote:
>> >
>> > 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.

Why not?  If idle=poll, for example, any time the guest is suspended
(and thus cannot poll) does count as stolen time.

In addition, you are going to account the stolen time anyway sooner or
later, and then it will be accounted wrong (subtracted to either user or
system time).  I really believe you should do the change directly in
get_vtime_delta.

Paolo

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web