Re: [PATCH] cpu hotplug, sched:Introduce cpu_active_map and redoscheddomainmanagment (take 2)

[Max Krasnyansky <maxk@qualcomm.com>]

Gregory Haskins wrote:
> Hi Ingo,
>  Here is another patch submitted that has not been acked/nacked yet.  If
> you get a free moment, please let me know your thoughts.  Here is the
> full thread for your convenience:
> 
> http://lkml.org/lkml/2008/7/22/281
> 
> (and FYI it was ACKed by Peter here: http://lkml.org/lkml/2008/7/22/286)

I thought this went in already. It looks good to me too.

Max


> 
> Gregory Haskins wrote:
>> Max Krasnyansky wrote:
>>> Greg, correct me if I'm wrong but we seem to have exact same issue
>>> with the
>>> rq->rq->online map. Lets take "cpu going down" for example. We're
>>> clearing
>>> rq->rd->online bit on DYING event, but nothing AFAICS prevents
>>> another cpu
>>> calling rebuild_sched_domains()->partition_sched_domains() in the
>>> middle of
>>> the hotplug sequence.
>>> partition_sched_domains() will happily reset rd->rq->online mask and
>>> things
>>> will fail. I'm talking about this path
>>>
>>> __build_sched_domains() -> cpu_attach_domain() -> rq_attach_root()
>>>     ...
>>>     cpu_set(rq->cpu, rd->span);
>>>     if (cpu_isset(rq->cpu, cpu_online_map))
>>>         set_rq_online(rq);
>>>     ...
>>>
>>>   
>>
>> I think you are right, but wouldn't s/online/active above fix that as
>> well?  The active_map didnt exist at the time that code went in
>> initially ;)
>>
>>> -- 
>>>
>>> btw Why didn't we convert sched*.c to use rq->rd->online when it was
>>> introduced ? ie Instead of using cpu_online_map directly.
>>>   
>> I think things were converted where they made sense to convert.  But
>> we also had a different goal at that time, so perhaps something was
>> missed.  If you think something else should be converted, please point
>> it out.
>>
>> In the meantime, I would suggest we consider this patch on top of
>> yours (applies to tip/sched/devel):
>>
>> ----------------------
>>
>> sched: Fully integrate cpus_active_map and root-domain code
>>   Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>>
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index 62b1b8e..99ba70d 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -6611,7 +6611,7 @@ static void rq_attach_root(struct rq *rq, struct
>> root_domain *rd)
>>     rq->rd = rd;
>>
>>     cpu_set(rq->cpu, rd->span);
>> -    if (cpu_isset(rq->cpu, cpu_online_map))
>> +    if (cpu_isset(rq->cpu, cpu_active_map))
>>         set_rq_online(rq);
>>
>>     spin_unlock_irqrestore(&rq->lock, flags);
>> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
>> index 7f70026..2bae8de 100644
>> --- a/kernel/sched_fair.c
>> +++ b/kernel/sched_fair.c
>> @@ -1004,7 +1004,7 @@ static void yield_task_fair(struct rq *rq)
>>  * search starts with cpus closest then further out as needed,
>>  * so we always favor a closer, idle cpu.
>>  * Domains may include CPUs that are not usable for migration,
>> - * hence we need to mask them out (cpu_active_map)
>> + * hence we need to mask them out (rq->rd->online)
>>  *
>>  * Returns the CPU we should wake onto.
>>  */
>> @@ -1032,7 +1032,7 @@ static int wake_idle(int cpu, struct task_struct
>> *p)
>>             || ((sd->flags & SD_WAKE_IDLE_FAR)
>>             && !task_hot(p, task_rq(p)->clock, sd))) {
>>             cpus_and(tmp, sd->span, p->cpus_allowed);
>> -            cpus_and(tmp, tmp, cpu_active_map);
>> +            cpus_and(tmp, tmp, task_rq(p)->rd->online);
>>             for_each_cpu_mask(i, tmp) {
>>                 if (idle_cpu(i)) {
>>                     if (i != task_cpu(p)) {
>> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
>> index 24621ce..d93169d 100644
>> --- a/kernel/sched_rt.c
>> +++ b/kernel/sched_rt.c
>> @@ -936,13 +936,6 @@ static int find_lowest_rq(struct task_struct *task)
>>         return -1; /* No targets found */
>>
>>     /*
>> -     * Only consider CPUs that are usable for migration.
>> -     * I guess we might want to change cpupri_find() to ignore those
>> -     * in the first place.
>> -     */
>> -    cpus_and(*lowest_mask, *lowest_mask, cpu_active_map);
>> -
>> -    /*
>>      * At this point we have built a mask of cpus representing the
>>      * lowest priority tasks in the system.  Now we want to elect
>>      * the best one based on our affinity and topology.
>>
>> --------------
>>
>> Regards,
>> -Greg
>>
> 
>