On 23 April 2013 10:50, Peter Zijlstra <[email protected]> wrote: > On Tue, 2013-04-23 at 09:52 +0200, Vincent Guittot wrote: > >> > static inline unsigned long *rq_nohz_flags(int cpu) >> > { >> > return rcu_dereference(cpu_rq(cpu)->sd)->nohz_flags; >> > } >> > >> > if (!test_bit(0, rq_nohz_flags(cpu))) >> > return; >> > clear_bit(0, rq_nohz_flags(cpu)); >> > >> >> AFAICT, if we use different rcu_dereferences for modifying nohz_flags >> and for updating the nr_busy_cpus, we open a time window for >> desynchronization, isn't it ? > > Oh right, we need to call rq_nohz_flags() once and use the pointer > twice. > >> >> +unlock: >> >> rcu_read_unlock(); >> >> } >> >> >> >> @@ -5409,14 +5416,21 @@ void set_cpu_sd_state_idle(void) >> >> { >> >> struct sched_domain *sd; >> >> int cpu = smp_processor_id(); >> >> - >> >> - if (test_bit(NOHZ_IDLE, nohz_flags(cpu))) >> >> - return; >> >> - set_bit(NOHZ_IDLE, nohz_flags(cpu)); >> >> + int first_nohz_idle = 1; >> >> >> >> rcu_read_lock(); >> >> - for_each_domain(cpu, sd) >> >> + for_each_domain(cpu, sd) { >> >> + if (first_nohz_idle) { >> >> + if (test_bit(NOHZ_IDLE, &sd->nohz_flags)) >> >> + goto unlock; >> >> + >> >> + set_bit(NOHZ_IDLE, &sd->nohz_flags); >> >> + first_nohz_idle = 0; >> >> + } >> >> + >> >> atomic_dec(&sd->groups->sgp->nr_busy_cpus); >> >> + } >> >> +unlock: >> > >> > Same here, .. why on earth do it for every sched_domain for that >> cpu? >> >> The update of rq_nohz_flags is done only once on the top level >> sched_domain and the nr_busy_cpus is updated on each sched_domain >> level in order to have a quick access to the number of busy cpu when >> we check for the need to kick an idle load balance > > Ah, I didn't see the whole first_nohz_idle thing.. but why did you > place it inside the loop in the first place? Wouldn't GCC be able to
My goal was to keep for_each_domain that is a normally used for accessing sched_domain tree > avoid the double cpu_rq(cpu)->sd dereference using CSE? Argh no,.. its > got an ACCESS_ONCE in it that defeats GCC. > > Bothersome.. > > I'd almost write it like: > > struct sched_domain *sd; > > rcu_read_lock(); > sd = rcu_dereference_check_sched_domain(cpu_rq(cpu)->sd); > > if (sd->nohz_idle) > goto unlock; > xchg(&sd->nohz_idle, 1); /* do we actually need atomics here? */ > > for (; sd; sd = sd->parent) > atomic_dec(&sd->groups->sgp->nr_busy_cpus); > unlock: > rcu_read_unlock(); > ok, i'm going to modify the patch and follow the sequence above Vincent > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
