| Message ID | 1465355110-21714-2-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/06/2016 05:05, Wanpeng Li wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > Commit e9532e69b8d1 ("sched/cputime: Fix steal time accounting vs. CPU hotplug") > set rq->prev_* to 0 after a cpu hotplug comes back in order to fix the scenario: > > | steal is smaller than rq->prev_steal_time we end up with an insane large > | value which then gets added to rq->prev_steal_time, resulting in a permanent > | wreckage of the accounting. > > However, it is still buggy. > > rq->prev_steal_time = 0: > > As Rik pointed out: > > | setting rq->prev_irq_time to 0 in the guest, and then getting a giant value from > | the host, could result in a very large of steal_jiffies. > > rq->prev_steal_time_rq = 0: > > | steal = paravirt_steal_clock(cpu_of(rq)); > | steal -= rq->prev_steal_time_rq; > | > | if (unlikely(steal > delta)) > | steal = delta; > | > | rq->prev_steal_time_rq += steal; > | delta -= steal; > | > | rq->clock_task += delta; > > steal is a giant value and rq->prev_steal_time_rq is 0, rq->prev_steal_time_rq > grows in delta granularity, rq->clock_task can't ramp up until rq->prev_steal_time_rq > catches up steal clock since delta value will be 0 after reducing steal time from > normal execution time. That's why I obersved that cpuhg/1-12 continue running > until rq->prev_steal_time_rq catches up steal clock timestamp. > > I believe rq->prev_irq_time has similar issue. So this patch fix it by reverting > commit e9532e69b8d1. This is still very hard to read. I think the justification for this patch is much simpler: ------- Commit e9532e69b8d1 ("sched/cputime: Fix steal time accounting vs. CPU hotplug") set rq->prev_* to 0 after a cpu hotplug comes back in order to fix the case where (after CPU hotplug) steal is smaller than rq->prev_steal_time. However, this should never happen. steal was only smaller because of the KVM-specific bug fixed by the previous patch. Worse, the previous patch triggers a bug on CPU hot-unplug/plug operation: because rq->prev_steal_time is cleared, all of the CPU's past steal time will be accounted again on hot-plug. Since the root cause has been fixed, we can just revert commit e9532e69b8d1. ------- Since things are currently pretty broken, I think it's fair to keep the two patches separate instead of squashing them. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-06-08 18:01 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>: > > > On 08/06/2016 05:05, Wanpeng Li wrote: >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> Commit e9532e69b8d1 ("sched/cputime: Fix steal time accounting vs. CPU hotplug") >> set rq->prev_* to 0 after a cpu hotplug comes back in order to fix the scenario: >> >> | steal is smaller than rq->prev_steal_time we end up with an insane large >> | value which then gets added to rq->prev_steal_time, resulting in a permanent >> | wreckage of the accounting. >> >> However, it is still buggy. >> >> rq->prev_steal_time = 0: >> >> As Rik pointed out: >> >> | setting rq->prev_irq_time to 0 in the guest, and then getting a giant value from >> | the host, could result in a very large of steal_jiffies. >> >> rq->prev_steal_time_rq = 0: >> >> | steal = paravirt_steal_clock(cpu_of(rq)); >> | steal -= rq->prev_steal_time_rq; >> | >> | if (unlikely(steal > delta)) >> | steal = delta; >> | >> | rq->prev_steal_time_rq += steal; >> | delta -= steal; >> | >> | rq->clock_task += delta; >> >> steal is a giant value and rq->prev_steal_time_rq is 0, rq->prev_steal_time_rq >> grows in delta granularity, rq->clock_task can't ramp up until rq->prev_steal_time_rq >> catches up steal clock since delta value will be 0 after reducing steal time from >> normal execution time. That's why I obersved that cpuhg/1-12 continue running >> until rq->prev_steal_time_rq catches up steal clock timestamp. >> >> I believe rq->prev_irq_time has similar issue. So this patch fix it by reverting >> commit e9532e69b8d1. > > This is still very hard to read. I think the justification for this > patch is much simpler: > > ------- > Commit e9532e69b8d1 ("sched/cputime: Fix steal time accounting vs. CPU > hotplug") set rq->prev_* to 0 after a cpu hotplug comes back in order to > fix the case where (after CPU hotplug) steal is smaller than > rq->prev_steal_time. > > However, this should never happen. steal was only smaller because of the > KVM-specific bug fixed by the previous patch. Worse, the previous patch > triggers a bug on CPU hot-unplug/plug operation: because > rq->prev_steal_time is cleared, all of the CPU's past steal time will be > accounted again on hot-plug. > > Since the root cause has been fixed, we can just revert commit e9532e69b8d1. > ------- Thanks Paolo! :) Regards, Wanpeng Li -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 7f2cae4..7d45bb3 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -7213,7 +7213,6 @@ static void sched_rq_cpu_starting(unsigned int cpu) struct rq *rq = cpu_rq(cpu); rq->calc_load_update = calc_load_update; - account_reset_rq(rq); update_max_interval(); } diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 72f1f30..de607e4 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1809,16 +1809,3 @@ static inline void cpufreq_trigger_update(u64 time) {} #else /* arch_scale_freq_capacity */ #define arch_scale_freq_invariant() (false) #endif - -static inline void account_reset_rq(struct rq *rq) -{ -#ifdef CONFIG_IRQ_TIME_ACCOUNTING - rq->prev_irq_time = 0; -#endif -#ifdef CONFIG_PARAVIRT - rq->prev_steal_time = 0; -#endif -#ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING - rq->prev_steal_time_rq = 0; -#endif -}