lkml.org 
[lkml]   [2015]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/2] sched: consider missed ticks when updating global cpu load
On Mon, Oct 05, 2015 at 10:15:55AM +0200, Peter Zijlstra wrote:
> On Sun, Oct 04, 2015 at 03:58:19PM +0900, Byungchul Park wrote:
> > anyway, it's wrong for update_process_times() to assume 1 tick because
> > tick_irq_exit() -> tick_nohz_irq_exit() -> tick_nohz_full_update_tick()
> > -> tick_nohz_restart_sched_tick() can happen at full NOHZ as i already
> > said. in this full NOHZ case for tick to restart from non-idle,
>
> NO_HZ_FULL is very much a work in progress, there's plenty wrong with
> it. But yes, if it does this then its broken here too, I'm not sure if
> Frederic is aware of this or not (I'm sure he's got a fairly big list of
> broken for NO_HZ_FULL).

Indeed and cpu load active is part of what needs to be fixed. I hope this
patchset will help.

>
> > 1. update_process_times() -> account_process_tick() must be able to handle
> > more than one tick, or tick_nohz_restart_sched_tick() should handle the
> > case additionally. (i think the latter is better.) i will try to modify
> > the code to handle it if you agree with me.
>
> Yes, and we need to audit all the other stuff called from
> update_process_times().
>
> run_local_timers() seems be ok.
> rcu_check_clalbacks() also doesn't seem to care about ticks.
>
> I _think_ we fixed most of the scheduler_tick()
> stuff (under the assumption that TSC is stable), but I'm not sure.

Concerning the variable pending ticks, we are fine with update_process_times()
except a few stuff in scheduler_tick():

* cpu load active
* sched_avg_update() handles well missed ticks as it's based on rq clock
and specific period for updates. But I'm worried about remote reads of rt_avg,
if any.
* calc_global_load_tick(), not sure about this one
* trigger_load_balance()
* the infamous task_tick() :-)

But load avg appears to me as a pretty standalone issue. So are each of these small
issues.

>
> and run_posix_cpu_timers() might also be ok.
>
> > 2. to handle full NOHZ, tick_nohz_restart_sched_tick() should call
> > update_cpu_load_active() instead of update_cpu_load_nohz() with my 1/2
> > patch and 2/2 patch, or we should modify update_cpu_load_nohz() to know
> > full NOHZ, which currently don't know full NOHZ. (you may agree with the
> > latter.) in any case, 1/2 patch is necessary which current code is
> > absolutely missing.
> >
> > peter, what do you think about my opinion? and about my 1/2 patch?
>
> I did not look too closely, but it might have the right shape for
> dealing with !idle ticks. I'd have to look more closely at it.
>
> > i will modify 2/2 patch depending on your feedback.
>
> I think it will take more than a single patch to rework all of
> update_process_times(). And we should also ask Thomas for his opinion,
> but I think we want:
>
> - make update_process_times() take a nr_ticks argument
> - fixup everything below it
>
> - fix tick_nohz_handler to not ignore the hrtimer_forward()
> return value and pass it into
> tick_sched_handle()/update_process_times().
>
> (assuming this is the right oneshot tick part, tick-common
> seems to be about periodic timers which aren't used much ?!)

this_nohz_handler() is the low res nohz handler. tick_sched_handle()
is the high res one (I should rename these). I think we should rather
find out the pending updates from update_process_times() itself and pass
it to scheduler_tick() which is the one interested in it.

Thanks.


\
 
 \ /
  Last update: 2015-10-12 20:01    [W:0.081 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site