Messages in this thread | | | From | Vincent Guittot <> | Date | Thu, 8 Feb 2018 17:52:00 +0100 | Subject | Re: [PATCH v2 1/3] sched: Stop nohz stats when decayed |
| |
On 8 February 2018 at 16:44, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Feb 08, 2018 at 04:05:58PM +0100, Vincent Guittot wrote: >> On 8 February 2018 at 15:00, Peter Zijlstra <peterz@infradead.org> wrote: >> > On Tue, Feb 06, 2018 at 08:23:05PM +0100, Vincent Guittot wrote: >> > >> >> @@ -9207,13 +9231,15 @@ void nohz_balance_enter_idle(int cpu) >> >> if (!housekeeping_cpu(cpu, HK_FLAG_SCHED)) >> >> return; >> >> >> >> + rq->has_blocked_load = 1; > > Should we not set that with rq->lock held? We already clear it while > holding rq->lock.
I think it's safe because it is used to re-enable the periodic decay unconditionally. It is cleared with rq->lock held to prevents any update of the cfs_rq *_avg while deciding if we can clear has_blocked_load
> >> >> + >> >> if (rq->nohz_tick_stopped) >> >> - return; >> > >> > this case is difficult... needs thinking >> >> The use case happens when a CPU wakes up and goes back to idle before >> the tick fires and clears nohz_tick_stopped. > > Yes, and so we could have accrued blocked load. Now in this case the CPU > must already be set in the cpumask, but we could've already cleared > has_blocked. > > My question is mostly about needing that "goto out" to set the flag, > because I think we can loose it on a store collision vs clearing it. But > in that case I suppose the iteration must already be in progress, which > in turn will observe rq->has_blocked_load and re-set nohz.has_blocked. > > So yes, this is good, but could use a comment. > >> > Without this ordering I think it would be possible to loose has_blocked >> > and not observe the CPU either. >> >> I think that you are right. >> I also wondered if some barriers were necessary but wrongly concluded >> that set operation on nohz.idle_cpus_mask and WRITE_ONCE with volatile >> would be enough to ensure the right ordering > > Yeah, so I forgot to write the comment in my patch, but it had the > barriers implied by cmpxchg.
| |