* [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-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-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-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;
as well as URLs for NNTP newsgroup(s).