Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-13 15:55 GMT+08:00 Paolo Bonzini: > > > On 13/06/2016 05:38, Wanpeng Li wrote: >> + delta_jiffies = jiffies_to_cputime(now - tsk->vtime_snap); >> + steal_jiffies = >> jiffies_to_cputime(steal_account_process_tick(delta_jiffies)); > > Without jiffies_to_cputime here. Apart from this, yes, this is what I > meant. You are right, I miss that. :) Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-13 15:55 GMT+08:00 Paolo Bonzini : > > > On 13/06/2016 05:38, Wanpeng Li wrote: >> + delta_jiffies = jiffies_to_cputime(now - tsk->vtime_snap); >> + steal_jiffies = >> jiffies_to_cputime(steal_account_process_tick(delta_jiffies)); > > Without jiffies_to_cputime here. Apart from this, yes, this is what I > meant. You are right, I miss that. :) Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On 13/06/2016 05:38, Wanpeng Li wrote: > + delta_jiffies = jiffies_to_cputime(now - tsk->vtime_snap); > + steal_jiffies = > jiffies_to_cputime(steal_account_process_tick(delta_jiffies)); Without jiffies_to_cputime here. Apart from this, yes, this is what I meant. Paolo > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > tsk->vtime_snap = now; > > - return jiffies_to_cputime(delta); > + return jiffies_to_cputime(delta_jiffies - steal_jiffies);
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On 13/06/2016 05:38, Wanpeng Li wrote: > + delta_jiffies = jiffies_to_cputime(now - tsk->vtime_snap); > + steal_jiffies = > jiffies_to_cputime(steal_account_process_tick(delta_jiffies)); Without jiffies_to_cputime here. Apart from this, yes, this is what I meant. Paolo > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > tsk->vtime_snap = now; > > - return jiffies_to_cputime(delta); > + return jiffies_to_cputime(delta_jiffies - steal_jiffies);
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 18:14 GMT+08:00 Paolo Bonzini: > > > On 08/06/2016 05:05, Wanpeng Li wrote: >> From: Wanpeng Li >> >> 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. >> >> Suggested-by: Rik van Riel >> Cc: Ingo Molnar >> Cc: Peter Zijlstra (Intel) >> Cc: Rik van Riel >> Cc: Thomas Gleixner >> Cc: Frederic Weisbecker >> Cc: Paolo Bonzini >> Cc: Radim Krčmář >> Signed-off-by: Wanpeng Li >> --- >> v4 -> v5: >> * apply same logic to account_idle_time, so change get_vtime_delta instead >> v3 -> v4: >> * fix grammar errors, thanks Ingo >> * cleanup fragile codes, thanks Ingo >> v2 -> v3: >> * convert steal time jiffies to cputime >> v1 -> v2: >> * fix divide zero bug, thanks Rik >> >> kernel/sched/cputime.c | 13 + >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >> index 75f98c5..b62f9f8 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(_steal_enabled)) { >> @@ -279,7 +279,7 @@ static __always_inline bool >> steal_account_process_tick(void) >> return steal_jiffies; >> } >> #endif >> - return false; >> + return 0; >> } >> >> /* >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; > > I think this is wrong. If you get more steal time than delta time > (which as Rik noticed can happen due to partial jiffies), you will end > up accounting things twice, once in steal_account_process_tick and once > here. In other words you'll get the exact bug you're trying to fix. > > The right thing is to add a max_jiffies argument to > steal_account_process_tick. steal_account_process_tick will not attempt > to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now > - tsk->vtime_snap) to steal_account_process_tick, existing callers can > pass ULONG_MAX. You can then > > return jiffies_to_cputime(delta_jiffies - steal_jiffies); > > in get_vtime_delta and not worry about underflow. Do you mean something like below, actually I see delta_jiffies < steal_jiffies sometimes. diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 75f98c5..a7606a9 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(unsigned long max_jiffies) { #ifdef CONFIG_PARAVIRT if (static_key_false(_steal_enabled)) { @@ -272,14 +272,14 @@ static __always_inline bool steal_account_process_tick(void) * time in jiffies. Lets cast the result to jiffies * granularity and account the rest on the next rounds. */ - steal_jiffies = nsecs_to_jiffies(steal); + steal_jiffies = min(nsecs_to_jiffies(steal), max_jiffies); this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies); account_steal_time(jiffies_to_cputime(steal_jiffies)); return steal_jiffies; } #endif - return false; + return 0; } /* @@ -346,7 +346,7 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick, u64 cputime = (__force u64) cputime_one_jiffy; u64 *cpustat = kcpustat_this_cpu->cpustat; - if (steal_account_process_tick()) + if (steal_account_process_tick(ULONG_MAX)) return; cputime *= ticks; @@ -477,7 +477,7 @@ void account_process_tick(struct task_struct *p, int user_tick)
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 18:14 GMT+08:00 Paolo Bonzini : > > > On 08/06/2016 05:05, Wanpeng Li wrote: >> From: Wanpeng Li >> >> 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. >> >> Suggested-by: Rik van Riel >> Cc: Ingo Molnar >> Cc: Peter Zijlstra (Intel) >> Cc: Rik van Riel >> Cc: Thomas Gleixner >> Cc: Frederic Weisbecker >> Cc: Paolo Bonzini >> Cc: Radim Krčmář >> Signed-off-by: Wanpeng Li >> --- >> v4 -> v5: >> * apply same logic to account_idle_time, so change get_vtime_delta instead >> v3 -> v4: >> * fix grammar errors, thanks Ingo >> * cleanup fragile codes, thanks Ingo >> v2 -> v3: >> * convert steal time jiffies to cputime >> v1 -> v2: >> * fix divide zero bug, thanks Rik >> >> kernel/sched/cputime.c | 13 + >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >> index 75f98c5..b62f9f8 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(_steal_enabled)) { >> @@ -279,7 +279,7 @@ static __always_inline bool >> steal_account_process_tick(void) >> return steal_jiffies; >> } >> #endif >> - return false; >> + return 0; >> } >> >> /* >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; > > I think this is wrong. If you get more steal time than delta time > (which as Rik noticed can happen due to partial jiffies), you will end > up accounting things twice, once in steal_account_process_tick and once > here. In other words you'll get the exact bug you're trying to fix. > > The right thing is to add a max_jiffies argument to > steal_account_process_tick. steal_account_process_tick will not attempt > to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now > - tsk->vtime_snap) to steal_account_process_tick, existing callers can > pass ULONG_MAX. You can then > > return jiffies_to_cputime(delta_jiffies - steal_jiffies); > > in get_vtime_delta and not worry about underflow. Do you mean something like below, actually I see delta_jiffies < steal_jiffies sometimes. diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 75f98c5..a7606a9 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(unsigned long max_jiffies) { #ifdef CONFIG_PARAVIRT if (static_key_false(_steal_enabled)) { @@ -272,14 +272,14 @@ static __always_inline bool steal_account_process_tick(void) * time in jiffies. Lets cast the result to jiffies * granularity and account the rest on the next rounds. */ - steal_jiffies = nsecs_to_jiffies(steal); + steal_jiffies = min(nsecs_to_jiffies(steal), max_jiffies); this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies); account_steal_time(jiffies_to_cputime(steal_jiffies)); return steal_jiffies; } #endif - return false; + return 0; } /* @@ -346,7 +346,7 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick, u64 cputime = (__force u64) cputime_one_jiffy; u64 *cpustat = kcpustat_this_cpu->cpustat; - if (steal_account_process_tick()) + if (steal_account_process_tick(ULONG_MAX)) return; cputime *= ticks; @@ -477,7 +477,7 @@ void account_process_tick(struct task_struct *p, int user_tick) return; } - if (steal_account_process_tick()) + if (steal_account_process_tick(ULONG_MAX)) return; if (user_tick) @@ -681,12 +681,14 @@ static cputime_t vtime_delta(struct task_struct *tsk) static cputime_t
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On Thu, 2016-06-09 at 07:57 +0800, Wanpeng Li wrote: > 2016-06-09 3:05 GMT+08:00 Rik van Riel: > > > > On Wed, 2016-06-08 at 11:05 +0800, Wanpeng Li wrote: > > > > > > > > > @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct > > > task_struct > > > *tsk) > > > static cputime_t get_vtime_delta(struct task_struct *tsk) > > > { > > > unsigned long now = READ_ONCE(jiffies); > > > - unsigned long delta = now - tsk->vtime_snap; > > > + cputime_t delta_time, steal_time; > > > > > > + steal_time = > > > jiffies_to_cputime(steal_account_process_tick()); > > > + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); > > > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > > > tsk->vtime_snap = now; > > > > > > - return jiffies_to_cputime(delta); > > > + if (steal_time < delta_time) > > > + delta_time -= steal_time; > > > + > > > + return delta_time; > > > } > > This isn't right. > > > > If steal_time is equal to or larger than delta_time, > > get_vtime_delta needs to return 0, not delta_time. > > > > Otherwise the same time will be counted twice. > Paolo also pointed out this yesterday, so his proposal looks good to > you, right? > Yes it does. I can build the irqtime rework on top of your patches, taking irq and softirq time out of the vtime delta as well. With Paolo's proposal, no time will ever be accounted double, which is a good thing. -- All Rights Reversed. signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On Thu, 2016-06-09 at 07:57 +0800, Wanpeng Li wrote: > 2016-06-09 3:05 GMT+08:00 Rik van Riel : > > > > On Wed, 2016-06-08 at 11:05 +0800, Wanpeng Li wrote: > > > > > > > > > @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct > > > task_struct > > > *tsk) > > > static cputime_t get_vtime_delta(struct task_struct *tsk) > > > { > > > unsigned long now = READ_ONCE(jiffies); > > > - unsigned long delta = now - tsk->vtime_snap; > > > + cputime_t delta_time, steal_time; > > > > > > + steal_time = > > > jiffies_to_cputime(steal_account_process_tick()); > > > + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); > > > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > > > tsk->vtime_snap = now; > > > > > > - return jiffies_to_cputime(delta); > > > + if (steal_time < delta_time) > > > + delta_time -= steal_time; > > > + > > > + return delta_time; > > > } > > This isn't right. > > > > If steal_time is equal to or larger than delta_time, > > get_vtime_delta needs to return 0, not delta_time. > > > > Otherwise the same time will be counted twice. > Paolo also pointed out this yesterday, so his proposal looks good to > you, right? > Yes it does. I can build the irqtime rework on top of your patches, taking irq and softirq time out of the vtime delta as well. With Paolo's proposal, no time will ever be accounted double, which is a good thing. -- All Rights Reversed. signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-09 3:05 GMT+08:00 Rik van Riel: > On Wed, 2016-06-08 at 11:05 +0800, Wanpeng Li wrote: >> >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct >> *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = >> jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; >> } > > This isn't right. > > If steal_time is equal to or larger than delta_time, > get_vtime_delta needs to return 0, not delta_time. > > Otherwise the same time will be counted twice. Paolo also pointed out this yesterday, so his proposal looks good to you, right? Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-09 3:05 GMT+08:00 Rik van Riel : > On Wed, 2016-06-08 at 11:05 +0800, Wanpeng Li wrote: >> >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct >> *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = >> jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; >> } > > This isn't right. > > If steal_time is equal to or larger than delta_time, > get_vtime_delta needs to return 0, not delta_time. > > Otherwise the same time will be counted twice. Paolo also pointed out this yesterday, so his proposal looks good to you, right? Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On Wed, 2016-06-08 at 11:05 +0800, Wanpeng Li wrote: > > @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct > *tsk) > static cputime_t get_vtime_delta(struct task_struct *tsk) > { > unsigned long now = READ_ONCE(jiffies); > - unsigned long delta = now - tsk->vtime_snap; > + cputime_t delta_time, steal_time; > > + steal_time = > jiffies_to_cputime(steal_account_process_tick()); > + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > tsk->vtime_snap = now; > > - return jiffies_to_cputime(delta); > + if (steal_time < delta_time) > + delta_time -= steal_time; > + > + return delta_time; > } This isn't right. If steal_time is equal to or larger than delta_time, get_vtime_delta needs to return 0, not delta_time. Otherwise the same time will be counted twice. -- All rights reversed signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On Wed, 2016-06-08 at 11:05 +0800, Wanpeng Li wrote: > > @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct > *tsk) > static cputime_t get_vtime_delta(struct task_struct *tsk) > { > unsigned long now = READ_ONCE(jiffies); > - unsigned long delta = now - tsk->vtime_snap; > + cputime_t delta_time, steal_time; > > + steal_time = > jiffies_to_cputime(steal_account_process_tick()); > + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > tsk->vtime_snap = now; > > - return jiffies_to_cputime(delta); > + if (steal_time < delta_time) > + delta_time -= steal_time; > + > + return delta_time; > } This isn't right. If steal_time is equal to or larger than delta_time, get_vtime_delta needs to return 0, not delta_time. Otherwise the same time will be counted twice. -- All rights reversed signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 18:14 GMT+08:00 Paolo Bonzini: > > > On 08/06/2016 05:05, Wanpeng Li wrote: >> From: Wanpeng Li >> >> 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. >> >> Suggested-by: Rik van Riel >> Cc: Ingo Molnar >> Cc: Peter Zijlstra (Intel) >> Cc: Rik van Riel >> Cc: Thomas Gleixner >> Cc: Frederic Weisbecker >> Cc: Paolo Bonzini >> Cc: Radim Krčmář >> Signed-off-by: Wanpeng Li >> --- >> v4 -> v5: >> * apply same logic to account_idle_time, so change get_vtime_delta instead >> v3 -> v4: >> * fix grammar errors, thanks Ingo >> * cleanup fragile codes, thanks Ingo >> v2 -> v3: >> * convert steal time jiffies to cputime >> v1 -> v2: >> * fix divide zero bug, thanks Rik >> >> kernel/sched/cputime.c | 13 + >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >> index 75f98c5..b62f9f8 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(_steal_enabled)) { >> @@ -279,7 +279,7 @@ static __always_inline bool >> steal_account_process_tick(void) >> return steal_jiffies; >> } >> #endif >> - return false; >> + return 0; >> } >> >> /* >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; > > I think this is wrong. If you get more steal time than delta time > (which as Rik noticed can happen due to partial jiffies), you will end > up accounting things twice, once in steal_account_process_tick and once > here. In other words you'll get the exact bug you're trying to fix. > > The right thing is to add a max_jiffies argument to > steal_account_process_tick. steal_account_process_tick will not attempt > to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now > - tsk->vtime_snap) to steal_account_process_tick, existing callers can > pass ULONG_MAX. You can then > > return jiffies_to_cputime(delta_jiffies - steal_jiffies); > > in get_vtime_delta and not worry about underflow. I see, I will do it in next version. Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 18:14 GMT+08:00 Paolo Bonzini : > > > On 08/06/2016 05:05, Wanpeng Li wrote: >> From: Wanpeng Li >> >> 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. >> >> Suggested-by: Rik van Riel >> Cc: Ingo Molnar >> Cc: Peter Zijlstra (Intel) >> Cc: Rik van Riel >> Cc: Thomas Gleixner >> Cc: Frederic Weisbecker >> Cc: Paolo Bonzini >> Cc: Radim Krčmář >> Signed-off-by: Wanpeng Li >> --- >> v4 -> v5: >> * apply same logic to account_idle_time, so change get_vtime_delta instead >> v3 -> v4: >> * fix grammar errors, thanks Ingo >> * cleanup fragile codes, thanks Ingo >> v2 -> v3: >> * convert steal time jiffies to cputime >> v1 -> v2: >> * fix divide zero bug, thanks Rik >> >> kernel/sched/cputime.c | 13 + >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >> index 75f98c5..b62f9f8 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(_steal_enabled)) { >> @@ -279,7 +279,7 @@ static __always_inline bool >> steal_account_process_tick(void) >> return steal_jiffies; >> } >> #endif >> - return false; >> + return 0; >> } >> >> /* >> @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) >> static cputime_t get_vtime_delta(struct task_struct *tsk) >> { >> unsigned long now = READ_ONCE(jiffies); >> - unsigned long delta = now - tsk->vtime_snap; >> + cputime_t delta_time, steal_time; >> >> + steal_time = jiffies_to_cputime(steal_account_process_tick()); >> + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); >> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); >> tsk->vtime_snap = now; >> >> - return jiffies_to_cputime(delta); >> + if (steal_time < delta_time) >> + delta_time -= steal_time; >> + >> + return delta_time; > > I think this is wrong. If you get more steal time than delta time > (which as Rik noticed can happen due to partial jiffies), you will end > up accounting things twice, once in steal_account_process_tick and once > here. In other words you'll get the exact bug you're trying to fix. > > The right thing is to add a max_jiffies argument to > steal_account_process_tick. steal_account_process_tick will not attempt > to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now > - tsk->vtime_snap) to steal_account_process_tick, existing callers can > pass ULONG_MAX. You can then > > return jiffies_to_cputime(delta_jiffies - steal_jiffies); > > in get_vtime_delta and not worry about underflow. I see, I will do it in next version. Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On 08/06/2016 05:05, Wanpeng Li wrote: > From: Wanpeng Li> > 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. > > Suggested-by: Rik van Riel > Cc: Ingo Molnar > Cc: Peter Zijlstra (Intel) > Cc: Rik van Riel > Cc: Thomas Gleixner > Cc: Frederic Weisbecker > Cc: Paolo Bonzini > Cc: Radim Krčmář > Signed-off-by: Wanpeng Li > --- > v4 -> v5: > * apply same logic to account_idle_time, so change get_vtime_delta instead > v3 -> v4: > * fix grammar errors, thanks Ingo > * cleanup fragile codes, thanks Ingo > v2 -> v3: > * convert steal time jiffies to cputime > v1 -> v2: > * fix divide zero bug, thanks Rik > > kernel/sched/cputime.c | 13 + > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > index 75f98c5..b62f9f8 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(_steal_enabled)) { > @@ -279,7 +279,7 @@ static __always_inline bool > steal_account_process_tick(void) > return steal_jiffies; > } > #endif > - return false; > + return 0; > } > > /* > @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) > static cputime_t get_vtime_delta(struct task_struct *tsk) > { > unsigned long now = READ_ONCE(jiffies); > - unsigned long delta = now - tsk->vtime_snap; > + cputime_t delta_time, steal_time; > > + steal_time = jiffies_to_cputime(steal_account_process_tick()); > + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > tsk->vtime_snap = now; > > - return jiffies_to_cputime(delta); > + if (steal_time < delta_time) > + delta_time -= steal_time; > + > + return delta_time; I think this is wrong. If you get more steal time than delta time (which as Rik noticed can happen due to partial jiffies), you will end up accounting things twice, once in steal_account_process_tick and once here. In other words you'll get the exact bug you're trying to fix. The right thing is to add a max_jiffies argument to steal_account_process_tick. steal_account_process_tick will not attempt to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now - tsk->vtime_snap) to steal_account_process_tick, existing callers can pass ULONG_MAX. You can then return jiffies_to_cputime(delta_jiffies - steal_jiffies); in get_vtime_delta and not worry about underflow. Paolo > } > > static void __vtime_account_system(struct task_struct *tsk) >
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
On 08/06/2016 05:05, Wanpeng Li wrote: > From: Wanpeng Li > > 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. > > Suggested-by: Rik van Riel > Cc: Ingo Molnar > Cc: Peter Zijlstra (Intel) > Cc: Rik van Riel > Cc: Thomas Gleixner > Cc: Frederic Weisbecker > Cc: Paolo Bonzini > Cc: Radim Krčmář > Signed-off-by: Wanpeng Li > --- > v4 -> v5: > * apply same logic to account_idle_time, so change get_vtime_delta instead > v3 -> v4: > * fix grammar errors, thanks Ingo > * cleanup fragile codes, thanks Ingo > v2 -> v3: > * convert steal time jiffies to cputime > v1 -> v2: > * fix divide zero bug, thanks Rik > > kernel/sched/cputime.c | 13 + > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > index 75f98c5..b62f9f8 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(_steal_enabled)) { > @@ -279,7 +279,7 @@ static __always_inline bool > steal_account_process_tick(void) > return steal_jiffies; > } > #endif > - return false; > + return 0; > } > > /* > @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) > static cputime_t get_vtime_delta(struct task_struct *tsk) > { > unsigned long now = READ_ONCE(jiffies); > - unsigned long delta = now - tsk->vtime_snap; > + cputime_t delta_time, steal_time; > > + steal_time = jiffies_to_cputime(steal_account_process_tick()); > + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); > WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); > tsk->vtime_snap = now; > > - return jiffies_to_cputime(delta); > + if (steal_time < delta_time) > + delta_time -= steal_time; > + > + return delta_time; I think this is wrong. If you get more steal time than delta time (which as Rik noticed can happen due to partial jiffies), you will end up accounting things twice, once in steal_account_process_tick and once here. In other words you'll get the exact bug you're trying to fix. The right thing is to add a max_jiffies argument to steal_account_process_tick. steal_account_process_tick will not attempt to remove more than max_jiffies. Here you pass delta_jiffies (i.e. now - tsk->vtime_snap) to steal_account_process_tick, existing callers can pass ULONG_MAX. You can then return jiffies_to_cputime(delta_jiffies - steal_jiffies); in get_vtime_delta and not worry about underflow. Paolo > } > > static void __vtime_account_system(struct task_struct *tsk) >
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 16:04 GMT+08:00 Wanpeng Li: > 2016-06-08 15:52 GMT+08:00 Ingo Molnar : >> >> * Wanpeng Li wrote: >> >>> 2016-06-08 15:22 GMT+08:00 Ingo Molnar : >>> > >>> > * Wanpeng Li wrote: >>> > >>> >> From: Wanpeng Li >>> >> >>> >> 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. >>> > >>> > So the 'ring boundary' part still doesn't parse (neither grammatically nor >>> > logically) - please rephrase it because I have no idea what you want to >>> > say here. >>> >>> It is original from this slides. >>> http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, >>> slide 28. >> >> Yes, I now understand that this is meant as 'context tracking is active', >> but I >> don't understand the way you use it in this changelog's context. >> >> Btw., the grammatically correct way to add that phrase would have been: >> >> ... time sampling became jiffy based, even if it's still listening to ring >> boundaries, so steal_account_process_tick() is reused to account how many >> 'ticks' are stolen-time, after the last accumulation. > > Thanks, Ingo! > >> >> But I still don't understand it, nor did Paolo understand it. >> >> Nor is there any 0/3 boilerplace description that gives some context about >> what >> these changes are about. Exactly what do you mean by 'add steal-time >> support' - we >> clearly had that before. So is your patch lifting some limitation? Or was >> steal-time accounting totally inactive with certain dynticks configurations? >> The >> changelog does not tell us anything about that... > > Now I understand why you said "write-only code". vtime(depends on > context tracking) which is just used in full dynamic doesn't account s/dynamic/dynticks > steal time, however, periodic/nohz idle which not use vtime have codes > account steal time in cputime.c, this patch add the steal time > acccount support in vtime which will be used in full dynamic guest. s/dynamic/dynticks > > Regards, > Wanpeng Li -- Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 16:04 GMT+08:00 Wanpeng Li : > 2016-06-08 15:52 GMT+08:00 Ingo Molnar : >> >> * Wanpeng Li wrote: >> >>> 2016-06-08 15:22 GMT+08:00 Ingo Molnar : >>> > >>> > * Wanpeng Li wrote: >>> > >>> >> From: Wanpeng Li >>> >> >>> >> 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. >>> > >>> > So the 'ring boundary' part still doesn't parse (neither grammatically nor >>> > logically) - please rephrase it because I have no idea what you want to >>> > say here. >>> >>> It is original from this slides. >>> http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, >>> slide 28. >> >> Yes, I now understand that this is meant as 'context tracking is active', >> but I >> don't understand the way you use it in this changelog's context. >> >> Btw., the grammatically correct way to add that phrase would have been: >> >> ... time sampling became jiffy based, even if it's still listening to ring >> boundaries, so steal_account_process_tick() is reused to account how many >> 'ticks' are stolen-time, after the last accumulation. > > Thanks, Ingo! > >> >> But I still don't understand it, nor did Paolo understand it. >> >> Nor is there any 0/3 boilerplace description that gives some context about >> what >> these changes are about. Exactly what do you mean by 'add steal-time >> support' - we >> clearly had that before. So is your patch lifting some limitation? Or was >> steal-time accounting totally inactive with certain dynticks configurations? >> The >> changelog does not tell us anything about that... > > Now I understand why you said "write-only code". vtime(depends on > context tracking) which is just used in full dynamic doesn't account s/dynamic/dynticks > steal time, however, periodic/nohz idle which not use vtime have codes > account steal time in cputime.c, this patch add the steal time > acccount support in vtime which will be used in full dynamic guest. s/dynamic/dynticks > > Regards, > Wanpeng Li -- Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 15:52 GMT+08:00 Ingo Molnar: > > * Wanpeng Li wrote: > >> 2016-06-08 15:22 GMT+08:00 Ingo Molnar : >> > >> > * Wanpeng Li wrote: >> > >> >> From: Wanpeng Li >> >> >> >> 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. >> > >> > So the 'ring boundary' part still doesn't parse (neither grammatically nor >> > logically) - please rephrase it because I have no idea what you want to >> > say here. >> >> It is original from this slides. >> http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, >> slide 28. > > Yes, I now understand that this is meant as 'context tracking is active', but > I > don't understand the way you use it in this changelog's context. > > Btw., the grammatically correct way to add that phrase would have been: > > ... time sampling became jiffy based, even if it's still listening to ring > boundaries, so steal_account_process_tick() is reused to account how many > 'ticks' are stolen-time, after the last accumulation. Thanks, Ingo! > > But I still don't understand it, nor did Paolo understand it. > > Nor is there any 0/3 boilerplace description that gives some context about > what > these changes are about. Exactly what do you mean by 'add steal-time support' > - we > clearly had that before. So is your patch lifting some limitation? Or was > steal-time accounting totally inactive with certain dynticks configurations? > The > changelog does not tell us anything about that... Now I understand why you said "write-only code". vtime(depends on context tracking) which is just used in full dynamic doesn't account steal time, however, periodic/nohz idle which not use vtime have codes account steal time in cputime.c, this patch add the steal time acccount support in vtime which will be used in full dynamic guest. Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 15:52 GMT+08:00 Ingo Molnar : > > * Wanpeng Li wrote: > >> 2016-06-08 15:22 GMT+08:00 Ingo Molnar : >> > >> > * Wanpeng Li wrote: >> > >> >> From: Wanpeng Li >> >> >> >> 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. >> > >> > So the 'ring boundary' part still doesn't parse (neither grammatically nor >> > logically) - please rephrase it because I have no idea what you want to >> > say here. >> >> It is original from this slides. >> http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, >> slide 28. > > Yes, I now understand that this is meant as 'context tracking is active', but > I > don't understand the way you use it in this changelog's context. > > Btw., the grammatically correct way to add that phrase would have been: > > ... time sampling became jiffy based, even if it's still listening to ring > boundaries, so steal_account_process_tick() is reused to account how many > 'ticks' are stolen-time, after the last accumulation. Thanks, Ingo! > > But I still don't understand it, nor did Paolo understand it. > > Nor is there any 0/3 boilerplace description that gives some context about > what > these changes are about. Exactly what do you mean by 'add steal-time support' > - we > clearly had that before. So is your patch lifting some limitation? Or was > steal-time accounting totally inactive with certain dynticks configurations? > The > changelog does not tell us anything about that... Now I understand why you said "write-only code". vtime(depends on context tracking) which is just used in full dynamic doesn't account steal time, however, periodic/nohz idle which not use vtime have codes account steal time in cputime.c, this patch add the steal time acccount support in vtime which will be used in full dynamic guest. Regards, Wanpeng Li
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
* Wanpeng Liwrote: > 2016-06-08 15:22 GMT+08:00 Ingo Molnar : > > > > * Wanpeng Li wrote: > > > >> From: Wanpeng Li > >> > >> 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. > > > > So the 'ring boundary' part still doesn't parse (neither grammatically nor > > logically) - please rephrase it because I have no idea what you want to say > > here. > > It is original from this slides. > http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, > slide 28. Yes, I now understand that this is meant as 'context tracking is active', but I don't understand the way you use it in this changelog's context. Btw., the grammatically correct way to add that phrase would have been: ... time sampling became jiffy based, even if it's still listening to ring boundaries, so steal_account_process_tick() is reused to account how many 'ticks' are stolen-time, after the last accumulation. But I still don't understand it, nor did Paolo understand it. Nor is there any 0/3 boilerplace description that gives some context about what these changes are about. Exactly what do you mean by 'add steal-time support' - we clearly had that before. So is your patch lifting some limitation? Or was steal-time accounting totally inactive with certain dynticks configurations? The changelog does not tell us anything about that... I'd like to quote from a mail of Andrew Morton: "Please update the changelog to describe the current behavior. Please also describe why you think that behavior should be changed. ie: what's the reason for this patch." Thanks, Ingo
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
* Wanpeng Li wrote: > 2016-06-08 15:22 GMT+08:00 Ingo Molnar : > > > > * Wanpeng Li wrote: > > > >> From: Wanpeng Li > >> > >> 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. > > > > So the 'ring boundary' part still doesn't parse (neither grammatically nor > > logically) - please rephrase it because I have no idea what you want to say > > here. > > It is original from this slides. > http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, > slide 28. Yes, I now understand that this is meant as 'context tracking is active', but I don't understand the way you use it in this changelog's context. Btw., the grammatically correct way to add that phrase would have been: ... time sampling became jiffy based, even if it's still listening to ring boundaries, so steal_account_process_tick() is reused to account how many 'ticks' are stolen-time, after the last accumulation. But I still don't understand it, nor did Paolo understand it. Nor is there any 0/3 boilerplace description that gives some context about what these changes are about. Exactly what do you mean by 'add steal-time support' - we clearly had that before. So is your patch lifting some limitation? Or was steal-time accounting totally inactive with certain dynticks configurations? The changelog does not tell us anything about that... I'd like to quote from a mail of Andrew Morton: "Please update the changelog to describe the current behavior. Please also describe why you think that behavior should be changed. ie: what's the reason for this patch." Thanks, Ingo
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 15:22 GMT+08:00 Ingo Molnar: > > * Wanpeng Li wrote: > >> From: Wanpeng Li >> >> 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. > > So the 'ring boundary' part still doesn't parse (neither grammatically nor > logically) - please rephrase it because I have no idea what you want to say > here. It is original from this slides. http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, slide 28. > > Did you want to say: > >> ... time sampling became jiffy based, even if it's still being context >> tracked, >> so steal_account_process_tick() is reused to account how many 'ticks' are >> stolen-time, after the last accumulation. > > ... which makes sense grammatically but does not make sense to me logically. > :-/ > > Rik, Frederic, could you please help out? > > Thanks, > > Ingo
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
2016-06-08 15:22 GMT+08:00 Ingo Molnar : > > * Wanpeng Li wrote: > >> From: Wanpeng Li >> >> 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. > > So the 'ring boundary' part still doesn't parse (neither grammatically nor > logically) - please rephrase it because I have no idea what you want to say > here. It is original from this slides. http://ertl.jp/~shinpei/conf/ospert13/slides/FredericWeisbecker.pdf, slide 28. > > Did you want to say: > >> ... time sampling became jiffy based, even if it's still being context >> tracked, >> so steal_account_process_tick() is reused to account how many 'ticks' are >> stolen-time, after the last accumulation. > > ... which makes sense grammatically but does not make sense to me logically. > :-/ > > Rik, Frederic, could you please help out? > > Thanks, > > Ingo
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
* Wanpeng Liwrote: > From: Wanpeng Li > > 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. So the 'ring boundary' part still doesn't parse (neither grammatically nor logically) - please rephrase it because I have no idea what you want to say here. Did you want to say: > ... time sampling became jiffy based, even if it's still being context > tracked, > so steal_account_process_tick() is reused to account how many 'ticks' are > stolen-time, after the last accumulation. ... which makes sense grammatically but does not make sense to me logically. :-/ Rik, Frederic, could you please help out? Thanks, Ingo
Re: [PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
* Wanpeng Li wrote: > From: Wanpeng Li > > 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. So the 'ring boundary' part still doesn't parse (neither grammatically nor logically) - please rephrase it because I have no idea what you want to say here. Did you want to say: > ... time sampling became jiffy based, even if it's still being context > tracked, > so steal_account_process_tick() is reused to account how many 'ticks' are > stolen-time, after the last accumulation. ... which makes sense grammatically but does not make sense to me logically. :-/ Rik, Frederic, could you please help out? Thanks, Ingo
[PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
From: Wanpeng LiThis 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. Suggested-by: Rik van Riel Cc: Ingo Molnar Cc: Peter Zijlstra (Intel) Cc: Rik van Riel Cc: Thomas Gleixner Cc: Frederic Weisbecker Cc: Paolo Bonzini Cc: Radim Krčmář Signed-off-by: Wanpeng Li --- v4 -> v5: * apply same logic to account_idle_time, so change get_vtime_delta instead v3 -> v4: * fix grammar errors, thanks Ingo * cleanup fragile codes, thanks Ingo v2 -> v3: * convert steal time jiffies to cputime v1 -> v2: * fix divide zero bug, thanks Rik kernel/sched/cputime.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 75f98c5..b62f9f8 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(_steal_enabled)) { @@ -279,7 +279,7 @@ static __always_inline bool steal_account_process_tick(void) return steal_jiffies; } #endif - return false; + return 0; } /* @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) static cputime_t get_vtime_delta(struct task_struct *tsk) { unsigned long now = READ_ONCE(jiffies); - unsigned long delta = now - tsk->vtime_snap; + cputime_t delta_time, steal_time; + steal_time = jiffies_to_cputime(steal_account_process_tick()); + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); tsk->vtime_snap = now; - return jiffies_to_cputime(delta); + if (steal_time < delta_time) + delta_time -= steal_time; + + return delta_time; } static void __vtime_account_system(struct task_struct *tsk) -- 1.9.1
[PATCH v5 3/3] sched/cputime: Add steal time support to full dynticks CPU time accounting
From: Wanpeng Li 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. Suggested-by: Rik van Riel Cc: Ingo Molnar Cc: Peter Zijlstra (Intel) Cc: Rik van Riel Cc: Thomas Gleixner Cc: Frederic Weisbecker Cc: Paolo Bonzini Cc: Radim Krčmář Signed-off-by: Wanpeng Li --- v4 -> v5: * apply same logic to account_idle_time, so change get_vtime_delta instead v3 -> v4: * fix grammar errors, thanks Ingo * cleanup fragile codes, thanks Ingo v2 -> v3: * convert steal time jiffies to cputime v1 -> v2: * fix divide zero bug, thanks Rik kernel/sched/cputime.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 75f98c5..b62f9f8 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(_steal_enabled)) { @@ -279,7 +279,7 @@ static __always_inline bool steal_account_process_tick(void) return steal_jiffies; } #endif - return false; + return 0; } /* @@ -681,12 +681,17 @@ static cputime_t vtime_delta(struct task_struct *tsk) static cputime_t get_vtime_delta(struct task_struct *tsk) { unsigned long now = READ_ONCE(jiffies); - unsigned long delta = now - tsk->vtime_snap; + cputime_t delta_time, steal_time; + steal_time = jiffies_to_cputime(steal_account_process_tick()); + delta_time = jiffies_to_cputime(now - tsk->vtime_snap); WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE); tsk->vtime_snap = now; - return jiffies_to_cputime(delta); + if (steal_time < delta_time) + delta_time -= steal_time; + + return delta_time; } static void __vtime_account_system(struct task_struct *tsk) -- 1.9.1