[PATCH v2 0/2] sched: fix nr_busy_cpus

[PATCH v2 0/2] sched: fix nr_busy_cpus

[Vincent Guittot]

The nr_busy_cpus field of the sched_group_power is sometime different from 0
whereas the platform is fully idle. This serie fixes 3 use cases:
 - when some CPUs enter idle state while booting all CPUs
 - when a CPU is unplug and/or replug

Change since V1:
 - remove the patch for SCHED softirq on an idle core use case as it was
   a side effect of the other use cases.

Vincent Guittot (2):
  sched: fix init NOHZ_IDLE flag
  sched: fix update NOHZ_IDLE flag

 kernel/sched/core.c |    1 +
 kernel/sched/fair.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Vincent Guittot]

On my smp platform which is made of 5 cores in 2 clusters,I have the
nr_busy_cpu field of sched_group_power struct that is not null when the
platform is fully idle. The root cause seems to be:
During the boot sequence, some CPUs reach the idle loop and set their
NOHZ_IDLE flag while waiting for others CPUs to boot. But the nr_busy_cpus
field is initialized later with the assumption that all CPUs are in the busy
state whereas some CPUs have already set their NOHZ_IDLE flag.
We clear the NOHZ_IDLE flag when nr_busy_cpus is initialized in order to
have a coherent configuration.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 257002c..fd41924 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5884,6 +5884,7 @@ static void init_sched_groups_power(int cpu, struct sched_domain *sd)
 
 	update_group_power(sd, cpu);
 	atomic_set(&sg->sgp->nr_busy_cpus, sg->group_weight);
+	clear_bit(NOHZ_IDLE, nohz_flags(cpu));
 }
 
 int __weak arch_sd_sibling_asym_packing(void)
-- 
1.7.9.5

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Frederic Weisbecker]

2013/1/29 Vincent Guittot <vincent.guittot@linaro.org>:
> On my smp platform which is made of 5 cores in 2 clusters,I have the
> nr_busy_cpu field of sched_group_power struct that is not null when the
> platform is fully idle. The root cause seems to be:
> During the boot sequence, some CPUs reach the idle loop and set their
> NOHZ_IDLE flag while waiting for others CPUs to boot. But the nr_busy_cpus
> field is initialized later with the assumption that all CPUs are in the busy
> state whereas some CPUs have already set their NOHZ_IDLE flag.
> We clear the NOHZ_IDLE flag when nr_busy_cpus is initialized in order to
> have a coherent configuration.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  kernel/sched/core.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 257002c..fd41924 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5884,6 +5884,7 @@ static void init_sched_groups_power(int cpu, struct sched_domain *sd)
>
>         update_group_power(sd, cpu);
>         atomic_set(&sg->sgp->nr_busy_cpus, sg->group_weight);
> +       clear_bit(NOHZ_IDLE, nohz_flags(cpu));

So that's a real issue indeed.  nr_busy_cpus was never correct.

Now I'm still a bit worried with this solution. What if an idle task
started in smp_init() has not yet stopped its tick, but is about to do
so? The domains are not yet available to the task but the nohz flags
are. When it later restarts the tick, it's going to erroneously
increase nr_busy_cpus.

It probably won't happen in practice. But then there is more: sched
domains can be concurrently rebuild anytime, right?  So what if we
call set_cpu_sd_state_idle() and decrease nr_busy_cpus while the
domain is switched concurrently. Are we having a new sched group along
the way? If so we have a bug here as well because we can have
NOHZ_IDLE set but nr_busy_cpus accounting the CPU.

May be we need to set the per cpu nohz flags on the child leaf sched
domain? This way it's initialized and stored on the same RCU pointer
and we nohz_flags and nr_busy_cpus become sync.

Also we probably still need the first patch of your previous round.
Because the current patch may introduce situations where we have idle
CPUs with NOHZ_IDLE flags cleared.

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Vincent Guittot]

On 1 February 2013 19:03, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 2013/1/29 Vincent Guittot <vincent.guittot@linaro.org>:
>> On my smp platform which is made of 5 cores in 2 clusters,I have the
>> nr_busy_cpu field of sched_group_power struct that is not null when the
>> platform is fully idle. The root cause seems to be:
>> During the boot sequence, some CPUs reach the idle loop and set their
>> NOHZ_IDLE flag while waiting for others CPUs to boot. But the nr_busy_cpus
>> field is initialized later with the assumption that all CPUs are in the busy
>> state whereas some CPUs have already set their NOHZ_IDLE flag.
>> We clear the NOHZ_IDLE flag when nr_busy_cpus is initialized in order to
>> have a coherent configuration.
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>>  kernel/sched/core.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 257002c..fd41924 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -5884,6 +5884,7 @@ static void init_sched_groups_power(int cpu, struct sched_domain *sd)
>>
>>         update_group_power(sd, cpu);
>>         atomic_set(&sg->sgp->nr_busy_cpus, sg->group_weight);
>> +       clear_bit(NOHZ_IDLE, nohz_flags(cpu));
>
> So that's a real issue indeed.  nr_busy_cpus was never correct.
>
> Now I'm still a bit worried with this solution. What if an idle task
> started in smp_init() has not yet stopped its tick, but is about to do
> so? The domains are not yet available to the task but the nohz flags
> are. When it later restarts the tick, it's going to erroneously
> increase nr_busy_cpus.

My 1st idea was to clear NOHZ_IDLE flag and nr_busy_cpus in
init_sched_groups_power instead of setting them as it is done now. If
a CPU enters idle during the init sequence, the flag is already
cleared, and nohz_flags and nr_busy_cpus will stay synced and cleared
while a NULL sched_domain is attached to the CPU thanks to patch 2.
This should solve all use cases ?

>
> It probably won't happen in practice. But then there is more: sched
> domains can be concurrently rebuild anytime, right?  So what if we
> call set_cpu_sd_state_idle() and decrease nr_busy_cpus while the
> domain is switched concurrently. Are we having a new sched group along
> the way? If so we have a bug here as well because we can have
> NOHZ_IDLE set but nr_busy_cpus accounting the CPU.

When the sched_domain are rebuilt, we set a null sched_domain during
the rebuild sequence and a new sched_group_power is created as well

>
> May be we need to set the per cpu nohz flags on the child leaf sched
> domain? This way it's initialized and stored on the same RCU pointer
> and we nohz_flags and nr_busy_cpus become sync.
>
> Also we probably still need the first patch of your previous round.
> Because the current patch may introduce situations where we have idle
> CPUs with NOHZ_IDLE flags cleared.

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Frederic Weisbecker]

2013/2/4 Vincent Guittot <vincent.guittot@linaro.org>:
> On 1 February 2013 19:03, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> index 257002c..fd41924 100644
>>> --- a/kernel/sched/core.c
>>> +++ b/kernel/sched/core.c
>>> @@ -5884,6 +5884,7 @@ static void init_sched_groups_power(int cpu, struct sched_domain *sd)
>>>
>>>         update_group_power(sd, cpu);
>>>         atomic_set(&sg->sgp->nr_busy_cpus, sg->group_weight);
>>> +       clear_bit(NOHZ_IDLE, nohz_flags(cpu));
>>
>> So that's a real issue indeed.  nr_busy_cpus was never correct.
>>
>> Now I'm still a bit worried with this solution. What if an idle task
>> started in smp_init() has not yet stopped its tick, but is about to do
>> so? The domains are not yet available to the task but the nohz flags
>> are. When it later restarts the tick, it's going to erroneously
>> increase nr_busy_cpus.
>
> My 1st idea was to clear NOHZ_IDLE flag and nr_busy_cpus in
> init_sched_groups_power instead of setting them as it is done now. If
> a CPU enters idle during the init sequence, the flag is already
> cleared, and nohz_flags and nr_busy_cpus will stay synced and cleared
> while a NULL sched_domain is attached to the CPU thanks to patch 2.
> This should solve all use cases ?

This may work on smp_init(). But the per cpu domain can be changed concurrently
anytime on cpu hotplug, with a new sched group power struct, right?

What if the following happen (inventing function names but you get the idea):

CPU 0                                           CPU 1

dom = new_domain(...) {
           nr_cpus_busy = 0;
           set_idle(CPU 1);                  old_dom =get_dom()
                                                     clear_idle(CPU 1)
}
rcu_assign_pointer(cpu1_dom, dom);


Can this scenario happen?


>>
>> It probably won't happen in practice. But then there is more: sched
>> domains can be concurrently rebuild anytime, right?  So what if we
>> call set_cpu_sd_state_idle() and decrease nr_busy_cpus while the
>> domain is switched concurrently. Are we having a new sched group along
>> the way? If so we have a bug here as well because we can have
>> NOHZ_IDLE set but nr_busy_cpus accounting the CPU.
>
> When the sched_domain are rebuilt, we set a null sched_domain during
> the rebuild sequence and a new sched_group_power is created as well

So at that time we may race with a CPU setting/clearing its NOHZ_IDLE flag
as in my above scenario?

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Vincent Guittot]

On 8 February 2013 16:35, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 2013/2/4 Vincent Guittot <vincent.guittot@linaro.org>:
>> On 1 February 2013 19:03, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>>> index 257002c..fd41924 100644
>>>> --- a/kernel/sched/core.c
>>>> +++ b/kernel/sched/core.c
>>>> @@ -5884,6 +5884,7 @@ static void init_sched_groups_power(int cpu, struct sched_domain *sd)
>>>>
>>>>         update_group_power(sd, cpu);
>>>>         atomic_set(&sg->sgp->nr_busy_cpus, sg->group_weight);
>>>> +       clear_bit(NOHZ_IDLE, nohz_flags(cpu));
>>>
>>> So that's a real issue indeed.  nr_busy_cpus was never correct.
>>>
>>> Now I'm still a bit worried with this solution. What if an idle task
>>> started in smp_init() has not yet stopped its tick, but is about to do
>>> so? The domains are not yet available to the task but the nohz flags
>>> are. When it later restarts the tick, it's going to erroneously
>>> increase nr_busy_cpus.
>>
>> My 1st idea was to clear NOHZ_IDLE flag and nr_busy_cpus in
>> init_sched_groups_power instead of setting them as it is done now. If
>> a CPU enters idle during the init sequence, the flag is already
>> cleared, and nohz_flags and nr_busy_cpus will stay synced and cleared
>> while a NULL sched_domain is attached to the CPU thanks to patch 2.
>> This should solve all use cases ?
>
> This may work on smp_init(). But the per cpu domain can be changed concurrently
> anytime on cpu hotplug, with a new sched group power struct, right?

During a cpu hotplug, a null domain is attached to each CPU of the
partition because we have to build new sched_domains so we have a
similar behavior than smp_init.
So if we clear  NOHZ_IDLE flag and nr_busy_cpus in
init_sched_groups_power, we should be safe for init and hotplug.

More generally speaking, if the sched_domains of a group of CPUs must
be rebuilt, a NULL sched_domain is attached to these CPUs during the
build

>
> What if the following happen (inventing function names but you get the idea):
>
> CPU 0                                           CPU 1
>
> dom = new_domain(...) {
>            nr_cpus_busy = 0;
>            set_idle(CPU 1);                  old_dom =get_dom()
>                                                      clear_idle(CPU 1)
> }
> rcu_assign_pointer(cpu1_dom, dom);
>
>
> Can this scenario happen?

This scenario will be:

 CPU 0                                           CPU 1

 detach_and_destroy_domain {
            rcu_assign_pointer(cpu1_dom, NULL);
 }

 dom = new_domain(...) {
            nr_cpus_busy = 0;
            set_idle(CPU 1);                  old_dom =get_dom()
                                                      old_dom is null
                                                      //clear_idle(CPU
1) can't happen because a null domain is attached so we will never
call nohz_kick_needed which is the only place where we can clear_idle
 }
 rcu_assign_pointer(cpu1_dom, dom);

>
>
>>>
>>> It probably won't happen in practice. But then there is more: sched
>>> domains can be concurrently rebuild anytime, right?  So what if we
>>> call set_cpu_sd_state_idle() and decrease nr_busy_cpus while the
>>> domain is switched concurrently. Are we having a new sched group along
>>> the way? If so we have a bug here as well because we can have
>>> NOHZ_IDLE set but nr_busy_cpus accounting the CPU.
>>
>> When the sched_domain are rebuilt, we set a null sched_domain during
>> the rebuild sequence and a new sched_group_power is created as well
>
> So at that time we may race with a CPU setting/clearing its NOHZ_IDLE flag
> as in my above scenario?

Unless i have missed a use case, we always have a null domain attached
to a CPU while we build the new one. So the patch 2/2 should protect
us against clearing the NOHZ_IDLE whereas the new nr_busy_cpus is not
yet attached.

I'm going to send a new version which set the NOHZ_IDLE bit and clear
nr_busy_cpus during the built of a sched_domain

Vincent

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Frederic Weisbecker]

2013/2/8 Vincent Guittot <vincent.guittot@linaro.org>:
> On 8 February 2013 16:35, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> What if the following happen (inventing function names but you get the idea):
>>
>> CPU 0                                           CPU 1
>>
>> dom = new_domain(...) {
>>            nr_cpus_busy = 0;
>>            set_idle(CPU 1);                  old_dom =get_dom()
>>                                                      clear_idle(CPU 1)
>> }
>> rcu_assign_pointer(cpu1_dom, dom);
>>
>>
>> Can this scenario happen?
>
> This scenario will be:
>
>  CPU 0                                           CPU 1
>
>  detach_and_destroy_domain {
>             rcu_assign_pointer(cpu1_dom, NULL);
>  }
>
>  dom = new_domain(...) {
>             nr_cpus_busy = 0;
>             set_idle(CPU 1);                  old_dom =get_dom()
>                                                       old_dom is null
>                                                       //clear_idle(CPU
> 1) can't happen because a null domain is attached so we will never
> call nohz_kick_needed which is the only place where we can clear_idle
>  }
>  rcu_assign_pointer(cpu1_dom, dom);

So is the following possible?

= CPU 0 =                                                       = CPU 1=

detach_and_destroy_domain {
            rcu_assign_pointer(cpu1_dom, NULL);
}

dom = new_domain(...) {
             nr_cpus_busy = 0;
             set_idle(CPU 1);
}

clear_idle(CPU 1)

dom = rcu_dereference(cpu1_dom)

//dom == NULL, return

rcu_assign_pointer(cpu1_dom, NULL);


set_idle(CPU 1)

dom = rcu_dereference(cpu1_dom)

//dec nr_cpus_busy, making it negative

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Frederic Weisbecker]

2013/2/18 Frederic Weisbecker <fweisbec@gmail.com>:
> 2013/2/8 Vincent Guittot <vincent.guittot@linaro.org>:
>> On 8 February 2013 16:35, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>> What if the following happen (inventing function names but you get the idea):
>>>
>>> CPU 0                                           CPU 1
>>>
>>> dom = new_domain(...) {
>>>            nr_cpus_busy = 0;
>>>            set_idle(CPU 1);                  old_dom =get_dom()
>>>                                                      clear_idle(CPU 1)
>>> }
>>> rcu_assign_pointer(cpu1_dom, dom);
>>>
>>>
>>> Can this scenario happen?
>>
>> This scenario will be:
>>
>>  CPU 0                                           CPU 1
>>
>>  detach_and_destroy_domain {
>>             rcu_assign_pointer(cpu1_dom, NULL);
>>  }
>>
>>  dom = new_domain(...) {
>>             nr_cpus_busy = 0;
>>             set_idle(CPU 1);                  old_dom =get_dom()
>>                                                       old_dom is null
>>                                                       //clear_idle(CPU
>> 1) can't happen because a null domain is attached so we will never
>> call nohz_kick_needed which is the only place where we can clear_idle
>>  }
>>  rcu_assign_pointer(cpu1_dom, dom);
>
> So is the following possible?
>
> = CPU 0 =                                                       = CPU 1=
>
> detach_and_destroy_domain {
>             rcu_assign_pointer(cpu1_dom, NULL);
> }
>
> dom = new_domain(...) {
>              nr_cpus_busy = 0;
>              set_idle(CPU 1);
> }
>
> clear_idle(CPU 1)
>
> dom = rcu_dereference(cpu1_dom)
>
> //dom == NULL, return
>
> rcu_assign_pointer(cpu1_dom, NULL);
>
>
> set_idle(CPU 1)
>
> dom = rcu_dereference(cpu1_dom)
>
> //dec nr_cpus_busy, making it negative

Sorry, gmail messed up as usual.

I pasted the original at: http://pastebin.com/DMm5U8J8

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Vincent Guittot]

On 18 February 2013 15:38, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 2013/2/18 Frederic Weisbecker <fweisbec@gmail.com>:
>> 2013/2/8 Vincent Guittot <vincent.guittot@linaro.org>:
>>> On 8 February 2013 16:35, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>>> What if the following happen (inventing function names but you get the idea):
>>>>
>>>> CPU 0                                           CPU 1
>>>>
>>>> dom = new_domain(...) {
>>>>            nr_cpus_busy = 0;
>>>>            set_idle(CPU 1);                  old_dom =get_dom()
>>>>                                                      clear_idle(CPU 1)
>>>> }
>>>> rcu_assign_pointer(cpu1_dom, dom);
>>>>
>>>>
>>>> Can this scenario happen?
>>>
>>> This scenario will be:
>>>
>>>  CPU 0                                           CPU 1
>>>
>>>  detach_and_destroy_domain {
>>>             rcu_assign_pointer(cpu1_dom, NULL);
>>>  }
>>>
>>>  dom = new_domain(...) {
>>>             nr_cpus_busy = 0;
>>>             set_idle(CPU 1);                  old_dom =get_dom()
>>>                                                       old_dom is null
>>>                                                       //clear_idle(CPU
>>> 1) can't happen because a null domain is attached so we will never
>>> call nohz_kick_needed which is the only place where we can clear_idle
>>>  }
>>>  rcu_assign_pointer(cpu1_dom, dom);
>>
>> So is the following possible?
>>
>> = CPU 0 =                                                       = CPU 1=
>>
>> detach_and_destroy_domain {
>>             rcu_assign_pointer(cpu1_dom, NULL);
>> }
>>
>> dom = new_domain(...) {
>>              nr_cpus_busy = 0;
>>              set_idle(CPU 1);
>> }
>>
>> clear_idle(CPU 1)
>>
>> dom = rcu_dereference(cpu1_dom)
>>
>> //dom == NULL, return
>>
>> rcu_assign_pointer(cpu1_dom, NULL);
>>
>>
>> set_idle(CPU 1)
>>
>> dom = rcu_dereference(cpu1_dom)
>>
>> //dec nr_cpus_busy, making it negative
>
> Sorry, gmail messed up as usual.
>
> I pasted the original at: http://pastebin.com/DMm5U8J8

We can clear the idle flag only in the nohz_kick_needed which will not
be called if the sched_domain is NULL so the sequence will be

= CPU 0 =                                            = CPU 1=

detach_and_destroy_domain {
            rcu_assign_pointer(cpu1_dom, NULL);
}

dom = new_domain(...) {
             nr_cpus_busy = 0;
             set_idle(CPU 1);
}
                                                    dom =
rcu_dereference(cpu1_dom)
                                                    //dom == NULL, return

rcu_assign_pointer(cpu1_dom, dom);

                                                    dom =
rcu_dereference(cpu1_dom)
                                                    //dom != NULL,
                                                    nohz_kick_needed {

set_idle(CPU 1)
                                                                   dom
= rcu_dereference(cpu1_dom)

//dec nr_cpus_busy,
                                                    }

Vincent

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Frederic Weisbecker]

2013/2/18 Vincent Guittot <vincent.guittot@linaro.org>:
> On 18 February 2013 15:38, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> I pasted the original at: http://pastebin.com/DMm5U8J8
>
> We can clear the idle flag only in the nohz_kick_needed which will not
> be called if the sched_domain is NULL so the sequence will be
>
> = CPU 0 =                                            = CPU 1=
>
> detach_and_destroy_domain {
>             rcu_assign_pointer(cpu1_dom, NULL);
> }
>
> dom = new_domain(...) {
>              nr_cpus_busy = 0;
>              set_idle(CPU 1);
> }
>                                                     dom =
> rcu_dereference(cpu1_dom)
>                                                     //dom == NULL, return
>
> rcu_assign_pointer(cpu1_dom, dom);
>
>                                                     dom =
> rcu_dereference(cpu1_dom)
>                                                     //dom != NULL,
>                                                     nohz_kick_needed {
>
> set_idle(CPU 1)
>                                                                    dom
> = rcu_dereference(cpu1_dom)
>
> //dec nr_cpus_busy,
>                                                     }
>
> Vincent

Ok but CPU 0 can assign NULL to the domain of cpu1 while CPU 1 is
already in the middle of nohz_kick_needed().

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Vincent Guittot]

On 18 February 2013 16:40, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 2013/2/18 Vincent Guittot <vincent.guittot@linaro.org>:
>> On 18 February 2013 15:38, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>> I pasted the original at: http://pastebin.com/DMm5U8J8
>>
>> We can clear the idle flag only in the nohz_kick_needed which will not
>> be called if the sched_domain is NULL so the sequence will be
>>
>> = CPU 0 =                                            = CPU 1=
>>
>> detach_and_destroy_domain {
>>             rcu_assign_pointer(cpu1_dom, NULL);
>> }
>>
>> dom = new_domain(...) {
>>              nr_cpus_busy = 0;
>>              set_idle(CPU 1);
>> }
>>                                                     dom =
>> rcu_dereference(cpu1_dom)
>>                                                     //dom == NULL, return
>>
>> rcu_assign_pointer(cpu1_dom, dom);
>>
>>                                                     dom =
>> rcu_dereference(cpu1_dom)
>>                                                     //dom != NULL,
>>                                                     nohz_kick_needed {
>>
>> set_idle(CPU 1)
>>                                                                    dom
>> = rcu_dereference(cpu1_dom)
>>
>> //dec nr_cpus_busy,
>>                                                     }
>>
>> Vincent
>
> Ok but CPU 0 can assign NULL to the domain of cpu1 while CPU 1 is
> already in the middle of nohz_kick_needed().

Yes nothing prevents the sequence below to occur

= CPU 0 =                                            = CPU 1=
                                                    dom =
rcu_dereference(cpu1_dom)
                                                    //dom != NULL
detach_and_destroy_domain {
            rcu_assign_pointer(cpu1_dom, NULL);
}

dom = new_domain(...) {
             nr_cpus_busy = 0;
             //nr_cpus_busy in the new_dom
             set_idle(CPU 1);
}
                                                    nohz_kick_needed {
                                                             clear_idle(CPU 1)
                                                             dom =
rcu_dereference(cpu1_dom)

//cpu1_dom == old_dom
                                                             inc nr_cpus_busy,

//nr_cpus_busy in the old_dom
                                                    }

rcu_assign_pointer(cpu1_dom, dom);
//cpu1_dom == new_dom

I'm not sure that this can happen in practice because CPU1 is in
interrupt handler but we don't have any mechanism to prevent the
sequence.

The NULL sched_domain can be used to detect this situation and the
set_cpu_sd_state_busy function can be modified like below

inline void set_cpu_sd_state_busy
 {
 	struct sched_domain *sd;
 	int cpu = smp_processor_id();
+	int clear = 0;

 	if (!test_bit(NOHZ_IDLE, nohz_flags(cpu)))
 		return;
-	clear_bit(NOHZ_IDLE, nohz_flags(cpu));

 	rcu_read_lock();
 	for_each_domain(cpu, sd) {
 		atomic_inc(&sd->groups->sgp->nr_busy_cpus);
+		clear = 1;
 	}
 	rcu_read_unlock();
+
+	if (likely(clear))
+		clear_bit(NOHZ_IDLE, nohz_flags(cpu));
 }

The NOHZ_IDLE flag will not be clear if we have a NULL sched_domain
attached to the CPU.
With this implementation, we still don't need to get the sched_domain
for testing the NOHZ_IDLE flag which occurs each time CPU becomes idle

The patch 2 become useless

Vincent

Re: [PATCH v2 1/2] sched: fix init NOHZ_IDLE flag

[Vincent Guittot]

On 19 February 2013 11:29, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> On 18 February 2013 16:40, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> 2013/2/18 Vincent Guittot <vincent.guittot@linaro.org>:
>>> On 18 February 2013 15:38, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>>>> I pasted the original at: http://pastebin.com/DMm5U8J8
>>>
>>> We can clear the idle flag only in the nohz_kick_needed which will not
>>> be called if the sched_domain is NULL so the sequence will be
>>>
>>> = CPU 0 =                                            = CPU 1=
>>>
>>> detach_and_destroy_domain {
>>>             rcu_assign_pointer(cpu1_dom, NULL);
>>> }
>>>
>>> dom = new_domain(...) {
>>>              nr_cpus_busy = 0;
>>>              set_idle(CPU 1);
>>> }
>>>                                                     dom =
>>> rcu_dereference(cpu1_dom)
>>>                                                     //dom == NULL, return
>>>
>>> rcu_assign_pointer(cpu1_dom, dom);
>>>
>>>                                                     dom =
>>> rcu_dereference(cpu1_dom)
>>>                                                     //dom != NULL,
>>>                                                     nohz_kick_needed {
>>>
>>> set_idle(CPU 1)
>>>                                                                    dom
>>> = rcu_dereference(cpu1_dom)
>>>
>>> //dec nr_cpus_busy,
>>>                                                     }
>>>
>>> Vincent
>>
>> Ok but CPU 0 can assign NULL to the domain of cpu1 while CPU 1 is
>> already in the middle of nohz_kick_needed().
>
> Yes nothing prevents the sequence below to occur
>
> = CPU 0 =                                            = CPU 1=
>                                                     dom =
> rcu_dereference(cpu1_dom)
>                                                     //dom != NULL
> detach_and_destroy_domain {
>             rcu_assign_pointer(cpu1_dom, NULL);
> }
>
> dom = new_domain(...) {
>              nr_cpus_busy = 0;
>              //nr_cpus_busy in the new_dom
>              set_idle(CPU 1);
> }
>                                                     nohz_kick_needed {
>                                                              clear_idle(CPU 1)
>                                                              dom =
> rcu_dereference(cpu1_dom)
>
> //cpu1_dom == old_dom
>                                                              inc nr_cpus_busy,
>
> //nr_cpus_busy in the old_dom
>                                                     }
>
> rcu_assign_pointer(cpu1_dom, dom);
> //cpu1_dom == new_dom

The sequence above is not correct in addition to become unreadable
after going through gmail

The correct and readable version
https://pastebin.linaro.org/1750/

Vincent

>
> I'm not sure that this can happen in practice because CPU1 is in
> interrupt handler but we don't have any mechanism to prevent the
> sequence.
>
> The NULL sched_domain can be used to detect this situation and the
> set_cpu_sd_state_busy function can be modified like below
>
> inline void set_cpu_sd_state_busy
>  {
>         struct sched_domain *sd;
>         int cpu = smp_processor_id();
> +       int clear = 0;
>
>         if (!test_bit(NOHZ_IDLE, nohz_flags(cpu)))
>                 return;
> -       clear_bit(NOHZ_IDLE, nohz_flags(cpu));
>
>         rcu_read_lock();
>         for_each_domain(cpu, sd) {
>                 atomic_inc(&sd->groups->sgp->nr_busy_cpus);
> +               clear = 1;
>         }
>         rcu_read_unlock();
> +
> +       if (likely(clear))
> +               clear_bit(NOHZ_IDLE, nohz_flags(cpu));
>  }
>
> The NOHZ_IDLE flag will not be clear if we have a NULL sched_domain
> attached to the CPU.
> With this implementation, we still don't need to get the sched_domain
> for testing the NOHZ_IDLE flag which occurs each time CPU becomes idle
>
> The patch 2 become useless
>
> Vincent

[PATCH v2 2/2] sched: fix update NOHZ_IDLE flag

[Vincent Guittot]

The function nohz_kick_needed modifies NOHZ_IDLE flag that is used to update
the nr_busy_cpus of the sched_group.
When the sched_domain are updated (during the boot or because of the unplug of
a CPUs as an example) a null_domain is attached to CPUs. We have to test
likely(!on_null_domain(cpu) first in order to detect such intialization step
and to not modify the NOHZ_IDLE flag

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5eea870..dac2edf 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5695,7 +5695,7 @@ void trigger_load_balance(struct rq *rq, int cpu)
 	    likely(!on_null_domain(cpu)))
 		raise_softirq(SCHED_SOFTIRQ);
 #ifdef CONFIG_NO_HZ
-	if (nohz_kick_needed(rq, cpu) && likely(!on_null_domain(cpu)))
+	if (likely(!on_null_domain(cpu)) && nohz_kick_needed(rq, cpu))
 		nohz_balancer_kick(cpu);
 #endif
 }
-- 
1.7.9.5