linux-kernel - Re: [PATCH v7] sched: fix init NOHZ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 23 Apr 2013 11:01:40 +0200
From:	Vincent Guittot <vincent.guittot@...aro.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	Ingo Molnar <mingo@...nel.org>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Paul Turner <pjt@...gle.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mike Galbraith <efault@....de>
Subject: Re: [PATCH v7] sched: fix init NOHZ_IDLE flag

On 23 April 2013 10:50, Peter Zijlstra <peterz@...radead.org> 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/