* [RESEND PATCH] sched: consider missed ticks when updating global cpu load @ 2015-09-25 8:52 byungchul.park 2015-09-26 13:14 ` Frederic Weisbecker 0 siblings, 1 reply; 6+ messages in thread From: byungchul.park @ 2015-09-25 8:52 UTC (permalink / raw) To: mingo, peterz; +Cc: linux-kernel, fweisbec, tglx, Byungchul Park From: Byungchul Park <byungchul.park@lge.com> hello, i have already sent this patch about 1 month ago. (see https://lkml.org/lkml/2015/8/13/160) now, i am resending the same patch with adding some additional commit message. thank you, byungchul ----->8----- >From 8ece9a0482e74a39cd2e9165bf8eec1d04665fa9 Mon Sep 17 00:00:00 2001 From: Byungchul Park <byungchul.park@lge.com> Date: Fri, 25 Sep 2015 17:10:10 +0900 Subject: [RESEND PATCH] sched: consider missed ticks when updating global cpu load in hrtimer_interrupt(), the first tick_program_event() can be failed because the next timer could be already expired due to, (see the comment in hrtimer_interrupt()) - tracing - long lasting callbacks - being scheduled away when running in a VM in the case that the first tick_program_event() is failed, the second tick_program_event() set the expired time to more than one tick later. then next tick can happen after more than one tick, even though tick is not stopped by e.g. NOHZ. when the next tick occurs, update_process_times() -> scheduler_tick() -> update_cpu_load_active() is performed, assuming the distance between last tick and current tick is 1 tick! it's wrong in this case. thus, this abnormal case should be considered in update_cpu_load_active(). Signed-off-by: Byungchul Park <byungchul.park@lge.com> --- kernel/sched/fair.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 4d5f97b..829282f 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -4356,12 +4356,15 @@ void update_cpu_load_nohz(void) */ void update_cpu_load_active(struct rq *this_rq) { + unsigned long curr_jiffies = READ_ONCE(jiffies); + unsigned long pending_updates; unsigned long load = weighted_cpuload(cpu_of(this_rq)); /* * See the mess around update_idle_cpu_load() / update_cpu_load_nohz(). */ - this_rq->last_load_update_tick = jiffies; - __update_cpu_load(this_rq, load, 1); + pending_updates = curr_jiffies - this_rq->last_load_update_tick; + this_rq->last_load_update_tick = curr_jiffies; + __update_cpu_load(this_rq, load, pending_updates); } /* -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] sched: consider missed ticks when updating global cpu load 2015-09-25 8:52 [RESEND PATCH] sched: consider missed ticks when updating global cpu load byungchul.park @ 2015-09-26 13:14 ` Frederic Weisbecker 2015-09-30 10:43 ` Peter Zijlstra 2015-10-02 0:43 ` Byungchul Park 0 siblings, 2 replies; 6+ messages in thread From: Frederic Weisbecker @ 2015-09-26 13:14 UTC (permalink / raw) To: byungchul.park; +Cc: mingo, peterz, linux-kernel, tglx On Fri, Sep 25, 2015 at 05:52:37PM +0900, byungchul.park@lge.com wrote: > From: Byungchul Park <byungchul.park@lge.com> > > hello, > > i have already sent this patch about 1 month ago. > (see https://lkml.org/lkml/2015/8/13/160) > > now, i am resending the same patch with adding some additional commit > message. > > thank you, > byungchul > > ----->8----- > From 8ece9a0482e74a39cd2e9165bf8eec1d04665fa9 Mon Sep 17 00:00:00 2001 > From: Byungchul Park <byungchul.park@lge.com> > Date: Fri, 25 Sep 2015 17:10:10 +0900 > Subject: [RESEND PATCH] sched: consider missed ticks when updating global cpu > load > > in hrtimer_interrupt(), the first tick_program_event() can be failed > because the next timer could be already expired due to, > (see the comment in hrtimer_interrupt()) > > - tracing > - long lasting callbacks > - being scheduled away when running in a VM > > in the case that the first tick_program_event() is failed, the second > tick_program_event() set the expired time to more than one tick later. > then next tick can happen after more than one tick, even though tick is > not stopped by e.g. NOHZ. > > when the next tick occurs, update_process_times() -> scheduler_tick() > -> update_cpu_load_active() is performed, assuming the distance between > last tick and current tick is 1 tick! it's wrong in this case. thus, > this abnormal case should be considered in update_cpu_load_active(). > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > --- > kernel/sched/fair.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 4d5f97b..829282f 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -4356,12 +4356,15 @@ void update_cpu_load_nohz(void) > */ > void update_cpu_load_active(struct rq *this_rq) > { > + unsigned long curr_jiffies = READ_ONCE(jiffies); > + unsigned long pending_updates; > unsigned long load = weighted_cpuload(cpu_of(this_rq)); > /* > * See the mess around update_idle_cpu_load() / update_cpu_load_nohz(). > */ > - this_rq->last_load_update_tick = jiffies; > - __update_cpu_load(this_rq, load, 1); > + pending_updates = curr_jiffies - this_rq->last_load_update_tick; > + this_rq->last_load_update_tick = curr_jiffies; > + __update_cpu_load(this_rq, load, pending_updates); > } That's right but __update_cpu_load() doesn't handle correctly pending updates with non-zero loads. Currently, pending updates are wheeled through decay_load_missed() that assume it's all about idle load. But in the cases you've enumerated, as well as in the nohz full case, missed pending updates can be about buzy loads. I think we need to fix update_cpu_load() to handle that first, or your fix is going to make things worse. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] sched: consider missed ticks when updating global cpu load 2015-09-26 13:14 ` Frederic Weisbecker @ 2015-09-30 10:43 ` Peter Zijlstra 2015-10-02 1:04 ` Byungchul Park 2015-10-02 0:43 ` Byungchul Park 1 sibling, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2015-09-30 10:43 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: byungchul.park, mingo, linux-kernel, tglx On Sat, Sep 26, 2015 at 03:14:45PM +0200, Frederic Weisbecker wrote: > > when the next tick occurs, update_process_times() -> scheduler_tick() > > -> update_cpu_load_active() is performed, assuming the distance between > > last tick and current tick is 1 tick! it's wrong in this case. thus, > > this abnormal case should be considered in update_cpu_load_active(). > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > > --- > > kernel/sched/fair.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 4d5f97b..829282f 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -4356,12 +4356,15 @@ void update_cpu_load_nohz(void) > > */ > > void update_cpu_load_active(struct rq *this_rq) > > { > > + unsigned long curr_jiffies = READ_ONCE(jiffies); > > + unsigned long pending_updates; > > unsigned long load = weighted_cpuload(cpu_of(this_rq)); > > /* > > * See the mess around update_idle_cpu_load() / update_cpu_load_nohz(). > > */ > > - this_rq->last_load_update_tick = jiffies; > > - __update_cpu_load(this_rq, load, 1); > > + pending_updates = curr_jiffies - this_rq->last_load_update_tick; > > + this_rq->last_load_update_tick = curr_jiffies; > > + __update_cpu_load(this_rq, load, pending_updates); > > } > > That's right but __update_cpu_load() doesn't handle correctly pending updates > with non-zero loads. Currently, pending updates are wheeled through decay_load_missed() > that assume it's all about idle load. > > But in the cases you've enumerated, as well as in the nohz full case, missed pending > updates can be about buzy loads. > > I think we need to fix update_cpu_load() to handle that first, or your fix is > going to make things worse. Its worse than that, the whole call chain of update_process_times() fully assumes a single tick, fixing just the one function deep down to handle more than 1 tick is ass backwards. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] sched: consider missed ticks when updating global cpu load 2015-09-30 10:43 ` Peter Zijlstra @ 2015-10-02 1:04 ` Byungchul Park 0 siblings, 0 replies; 6+ messages in thread From: Byungchul Park @ 2015-10-02 1:04 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Frederic Weisbecker, mingo, linux-kernel, tglx On Wed, Sep 30, 2015 at 12:43:43PM +0200, Peter Zijlstra wrote: > On Sat, Sep 26, 2015 at 03:14:45PM +0200, Frederic Weisbecker wrote: > > > > when the next tick occurs, update_process_times() -> scheduler_tick() > > > -> update_cpu_load_active() is performed, assuming the distance between > > > last tick and current tick is 1 tick! it's wrong in this case. thus, > > > this abnormal case should be considered in update_cpu_load_active(). > > > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > > > --- > > > kernel/sched/fair.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 4d5f97b..829282f 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -4356,12 +4356,15 @@ void update_cpu_load_nohz(void) > > > */ > > > void update_cpu_load_active(struct rq *this_rq) > > > { > > > + unsigned long curr_jiffies = READ_ONCE(jiffies); > > > + unsigned long pending_updates; > > > unsigned long load = weighted_cpuload(cpu_of(this_rq)); > > > /* > > > * See the mess around update_idle_cpu_load() / update_cpu_load_nohz(). > > > */ > > > - this_rq->last_load_update_tick = jiffies; > > > - __update_cpu_load(this_rq, load, 1); > > > + pending_updates = curr_jiffies - this_rq->last_load_update_tick; > > > + this_rq->last_load_update_tick = curr_jiffies; > > > + __update_cpu_load(this_rq, load, pending_updates); > > > } > > > > That's right but __update_cpu_load() doesn't handle correctly pending updates > > with non-zero loads. Currently, pending updates are wheeled through decay_load_missed() > > that assume it's all about idle load. > > > > But in the cases you've enumerated, as well as in the nohz full case, missed pending > > updates can be about buzy loads. > > > > I think we need to fix update_cpu_load() to handle that first, or your fix is > > going to make things worse. > > Its worse than that, the whole call chain of update_process_times() > fully assumes a single tick, fixing just the one function deep down to yes update_process_times() currently assumes a single tick which is not good. i think this assuming should be removed one by one so that full NOHZ works completely. > handle more than 1 tick is ass backwards. why do you think fixing this funciton to handle more than 1 tick is assbackwards? i don't think so because there's no dependancy between this function and any other tick handling functions. that kind of bugs should be fixed carefully one by one until all is done, though it could be assbackwards if fixing something effects another tick handling. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] sched: consider missed ticks when updating global cpu load 2015-09-26 13:14 ` Frederic Weisbecker 2015-09-30 10:43 ` Peter Zijlstra @ 2015-10-02 0:43 ` Byungchul Park 2015-10-02 1:25 ` Byungchul Park 1 sibling, 1 reply; 6+ messages in thread From: Byungchul Park @ 2015-10-02 0:43 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: mingo, peterz, linux-kernel, tglx On Sat, Sep 26, 2015 at 03:14:45PM +0200, Frederic Weisbecker wrote: > On Fri, Sep 25, 2015 at 05:52:37PM +0900, byungchul.park@lge.com wrote: > > From: Byungchul Park <byungchul.park@lge.com> > > > > hello, > > > > i have already sent this patch about 1 month ago. > > (see https://lkml.org/lkml/2015/8/13/160) > > > > now, i am resending the same patch with adding some additional commit > > message. > > > > thank you, > > byungchul > > > > ----->8----- > > From 8ece9a0482e74a39cd2e9165bf8eec1d04665fa9 Mon Sep 17 00:00:00 2001 > > From: Byungchul Park <byungchul.park@lge.com> > > Date: Fri, 25 Sep 2015 17:10:10 +0900 > > Subject: [RESEND PATCH] sched: consider missed ticks when updating global cpu > > load > > > > in hrtimer_interrupt(), the first tick_program_event() can be failed > > because the next timer could be already expired due to, > > (see the comment in hrtimer_interrupt()) > > > > - tracing > > - long lasting callbacks > > - being scheduled away when running in a VM > > > > in the case that the first tick_program_event() is failed, the second > > tick_program_event() set the expired time to more than one tick later. > > then next tick can happen after more than one tick, even though tick is > > not stopped by e.g. NOHZ. > > > > when the next tick occurs, update_process_times() -> scheduler_tick() > > -> update_cpu_load_active() is performed, assuming the distance between > > last tick and current tick is 1 tick! it's wrong in this case. thus, > > this abnormal case should be considered in update_cpu_load_active(). > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > > --- > > kernel/sched/fair.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 4d5f97b..829282f 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -4356,12 +4356,15 @@ void update_cpu_load_nohz(void) > > */ > > void update_cpu_load_active(struct rq *this_rq) > > { > > + unsigned long curr_jiffies = READ_ONCE(jiffies); > > + unsigned long pending_updates; > > unsigned long load = weighted_cpuload(cpu_of(this_rq)); > > /* > > * See the mess around update_idle_cpu_load() / update_cpu_load_nohz(). > > */ > > - this_rq->last_load_update_tick = jiffies; > > - __update_cpu_load(this_rq, load, 1); > > + pending_updates = curr_jiffies - this_rq->last_load_update_tick; > > + this_rq->last_load_update_tick = curr_jiffies; > > + __update_cpu_load(this_rq, load, pending_updates); > > } > > That's right but __update_cpu_load() doesn't handle correctly pending updates > with non-zero loads. Currently, pending updates are wheeled through decay_load_missed() > that assume it's all about idle load. i see, i will check it. > > But in the cases you've enumerated, as well as in the nohz full case, missed pending > updates can be about buzy loads. right. it can be about busy loads. > > I think we need to fix update_cpu_load() to handle that first, or your fix is > going to make things worse. i will try to fix it at first if there's already that kind of bugs. thanks, byungchul > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] sched: consider missed ticks when updating global cpu load 2015-10-02 0:43 ` Byungchul Park @ 2015-10-02 1:25 ` Byungchul Park 0 siblings, 0 replies; 6+ messages in thread From: Byungchul Park @ 2015-10-02 1:25 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: mingo, peterz, linux-kernel, tglx On Fri, Oct 02, 2015 at 09:43:17AM +0900, Byungchul Park wrote: > On Sat, Sep 26, 2015 at 03:14:45PM +0200, Frederic Weisbecker wrote: > > On Fri, Sep 25, 2015 at 05:52:37PM +0900, byungchul.park@lge.com wrote: > > > From: Byungchul Park <byungchul.park@lge.com> > > > > > > hello, > > > > > > i have already sent this patch about 1 month ago. > > > (see https://lkml.org/lkml/2015/8/13/160) > > > > > > now, i am resending the same patch with adding some additional commit > > > message. > > > > > > thank you, > > > byungchul > > > > > > ----->8----- > > > From 8ece9a0482e74a39cd2e9165bf8eec1d04665fa9 Mon Sep 17 00:00:00 2001 > > > From: Byungchul Park <byungchul.park@lge.com> > > > Date: Fri, 25 Sep 2015 17:10:10 +0900 > > > Subject: [RESEND PATCH] sched: consider missed ticks when updating global cpu > > > load > > > > > > in hrtimer_interrupt(), the first tick_program_event() can be failed > > > because the next timer could be already expired due to, > > > (see the comment in hrtimer_interrupt()) > > > > > > - tracing > > > - long lasting callbacks > > > - being scheduled away when running in a VM > > > > > > in the case that the first tick_program_event() is failed, the second > > > tick_program_event() set the expired time to more than one tick later. > > > then next tick can happen after more than one tick, even though tick is > > > not stopped by e.g. NOHZ. > > > > > > when the next tick occurs, update_process_times() -> scheduler_tick() > > > -> update_cpu_load_active() is performed, assuming the distance between > > > last tick and current tick is 1 tick! it's wrong in this case. thus, > > > this abnormal case should be considered in update_cpu_load_active(). > > > > > > Signed-off-by: Byungchul Park <byungchul.park@lge.com> > > > --- > > > kernel/sched/fair.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 4d5f97b..829282f 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -4356,12 +4356,15 @@ void update_cpu_load_nohz(void) > > > */ > > > void update_cpu_load_active(struct rq *this_rq) > > > { > > > + unsigned long curr_jiffies = READ_ONCE(jiffies); > > > + unsigned long pending_updates; > > > unsigned long load = weighted_cpuload(cpu_of(this_rq)); > > > /* > > > * See the mess around update_idle_cpu_load() / update_cpu_load_nohz(). > > > */ > > > - this_rq->last_load_update_tick = jiffies; > > > - __update_cpu_load(this_rq, load, 1); > > > + pending_updates = curr_jiffies - this_rq->last_load_update_tick; > > > + this_rq->last_load_update_tick = curr_jiffies; > > > + __update_cpu_load(this_rq, load, pending_updates); > > > } > > > > That's right but __update_cpu_load() doesn't handle correctly pending updates > > with non-zero loads. Currently, pending updates are wheeled through decay_load_missed() > > that assume it's all about idle load. > > i see, i will check it. > > > > > But in the cases you've enumerated, as well as in the nohz full case, missed pending > > updates can be about buzy loads. > > right. it can be about busy loads. > > > > > I think we need to fix update_cpu_load() to handle that first, or your fix is > > going to make things worse. > > i will try to fix it at first if there's already that kind of bugs. i checked it.. current code does not handle more than one active tick at all, rather than bug.. > > thanks, > byungchul > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-02 1:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-25 8:52 [RESEND PATCH] sched: consider missed ticks when updating global cpu load byungchul.park 2015-09-26 13:14 ` Frederic Weisbecker 2015-09-30 10:43 ` Peter Zijlstra 2015-10-02 1:04 ` Byungchul Park 2015-10-02 0:43 ` Byungchul Park 2015-10-02 1:25 ` Byungchul Park
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox