[PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Rakib Mullick]

 disabled_cpus has been incremented from the call path of
generic_processor_info (i.e from  acpi_register_lapic and
MP_processor_info) when a perticular cpu is not enabled. So, we can
remove the redundant increment of disabled_cpus from
generic_processor_info.


Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
---

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index ce65d44..2499664 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1943,7 +1943,6 @@ void __cpuinit generic_processor_info(int
apicid, int version)
 			"ACPI: NR_CPUS/possible_cpus limit of %i reached."
 			"  Processor %d/0x%x ignored.\n", max, thiscpu, apicid);

-		disabled_cpus++;
 		return;
 	}

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Ingo Molnar]

* Rakib Mullick <rakib.mullick@gmail.com> wrote:

>  disabled_cpus has been incremented from the call path of
> generic_processor_info (i.e from  acpi_register_lapic and
> MP_processor_info) when a perticular cpu is not enabled. So, we can
> remove the redundant increment of disabled_cpus from
> generic_processor_info.
> 
> 
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> ---
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index ce65d44..2499664 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1943,7 +1943,6 @@ void __cpuinit generic_processor_info(int
> apicid, int version)
>  			"ACPI: NR_CPUS/possible_cpus limit of %i reached."
>  			"  Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
> 
> -		disabled_cpus++;
>  		return;

Hm, what effects does this have in practice? smpboot.c uses disabled_cpus as a value 
to calculate limits - why has this bug not caused some misbehavior somewhere? (or if 
it has caused misbehavior, what is that?)
 
Thanks,

	Ingo

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Cyrill Gorcunov]

On 01/09/2011 12:39 PM, Ingo Molnar wrote:
> 
> * Rakib Mullick <rakib.mullick@gmail.com> wrote:
> 
>>  disabled_cpus has been incremented from the call path of
>> generic_processor_info (i.e from  acpi_register_lapic and
>> MP_processor_info) when a perticular cpu is not enabled. So, we can
>> remove the redundant increment of disabled_cpus from
>> generic_processor_info.
>>
>>
>> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
...
> Hm, what effects does this have in practice? smpboot.c uses disabled_cpus as a value 
> to calculate limits - why has this bug not caused some misbehavior somewhere? (or if 
> it has caused misbehavior, what is that?)
>  
> Thanks,
> 
> 	Ingo

This disabled cpu may happen if nr_cpus early param specified and it's less than
nr_cpu_ids, but cpu might still be enabled in mp/acpi so we have to disable it.
And if we will not increment disabled_cpus this will lead the further report
about disabled cpu would refer to wrong cpu number. So I don't see why we
need to remove this disabled_cpus increment here.

Rakib is there something I miss?

-- 
    Cyrill

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Rakib Mullick]

On Sun, Jan 9, 2011 at 5:44 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On 01/09/2011 12:39 PM, Ingo Molnar wrote:
>>
>> * Rakib Mullick <rakib.mullick@gmail.com> wrote:
>>
>>>  disabled_cpus has been incremented from the call path of
>>> generic_processor_info (i.e from  acpi_register_lapic and
>>> MP_processor_info) when a perticular cpu is not enabled. So, we can
>>> remove the redundant increment of disabled_cpus from
>>> generic_processor_info.
>>>
>>>
>>> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> ...
>> Hm, what effects does this have in practice? smpboot.c uses disabled_cpus as a value
>> to calculate limits - why has this bug not caused some misbehavior somewhere? (or if
>> it has caused misbehavior, what is that?)
>>
>> Thanks,
>>
>>       Ingo
>
> This disabled cpu may happen if nr_cpus early param specified and it's less than
> nr_cpu_ids, but cpu might still be enabled in mp/acpi so we have to disable it.
> And if we will not increment disabled_cpus this will lead the further report
> about disabled cpu would refer to wrong cpu number. So I don't see why we
> need to remove this disabled_cpus increment here.
>
> Rakib is there something I miss?
>
Lets see what Ingo says. :)

> --
>    Cyrill
>

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Rakib Mullick]

On Sun, Jan 9, 2011 at 3:39 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> Hm, what effects does this have in practice? smpboot.c uses disabled_cpus as a value
> to calculate limits - why has this bug not caused some misbehavior somewhere? (or if
> it has caused misbehavior, what is that?)

If I'm not wrong, smpboot.c tries to get the possible cpu map by
calculating disabled_cpus and num_processors. When we pass nr_cpus=n,
which is less than no. of CPUs available, we can't put more CPUs
online. So, no of cpu we detect at startup is okay. Or am I missing
anything?

thanks,
rakib

>
> Thanks,
>
>        Ingo
>

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Cyrill Gorcunov]

On 01/09/2011 07:57 PM, Rakib Mullick wrote:
> On Sun, Jan 9, 2011 at 3:39 PM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>> Hm, what effects does this have in practice? smpboot.c uses disabled_cpus as a value
>> to calculate limits - why has this bug not caused some misbehavior somewhere? (or if
>> it has caused misbehavior, what is that?)
> 
> If I'm not wrong, smpboot.c tries to get the possible cpu map by
> calculating disabled_cpus and num_processors. When we pass nr_cpus=n,
> which is less than no. of CPUs available, we can't put more CPUs
> online. So, no of cpu we detect at startup is okay. Or am I missing
> anything?
> 
> thanks,
> rakib
> 

 When nr_cpus=n passed from command line and there is N > n physical cpu
present we *still* have to increment disabled_cpus in generic_processor_info
because:

1) We're priting out the number of cpu which is disabled
2) total_cpus become inconsistent

and while (1) is not that important, total_cpus _is_ important (it
is used to print out offlined cpus).

 So I still fail to see why we need to drop the former increment in
first place.
-- 
    Cyrill

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Rakib Mullick]

On Mon, Jan 10, 2011 at 12:38 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On 01/09/2011 07:57 PM, Rakib Mullick wrote:
>> On Sun, Jan 9, 2011 at 3:39 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
>  When nr_cpus=n passed from command line and there is N > n physical cpu
> present we *still* have to increment disabled_cpus in generic_processor_info
> because:
>
> 1) We're priting out the number of cpu which is disabled
> 2) total_cpus become inconsistent
>
> and while (1) is not that important, total_cpus _is_ important (it
> is used to print out offlined cpus).
>
When we use nr_cpus=n, it works as an upper limit. If there are any
other CPUs beyond that limit those are not counted and we couldn't put
them back on work. So, when we couldn't use hotpluging feature to back
them into work, should we care about them?


>  So I still fail to see why we need to drop the former increment in
> first place.
> --
>    Cyrill
>

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Cyrill Gorcunov]

On 01/10/2011 07:06 AM, Rakib Mullick wrote:
> On Mon, Jan 10, 2011 at 12:38 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> On 01/09/2011 07:57 PM, Rakib Mullick wrote:
>>> On Sun, Jan 9, 2011 at 3:39 PM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>>  When nr_cpus=n passed from command line and there is N > n physical cpu
>> present we *still* have to increment disabled_cpus in generic_processor_info
>> because:
>>
>> 1) We're priting out the number of cpu which is disabled
>> 2) total_cpus become inconsistent
>>
>> and while (1) is not that important, total_cpus _is_ important (it
>> is used to print out offlined cpus).
>>
> When we use nr_cpus=n, it works as an upper limit. If there are any
> other CPUs beyond that limit those are not counted and we couldn't put
> them back on work. So, when we couldn't use hotpluging feature to back
> them into work, should we care about them?
> 

  Yes we should, the cpus which are present but offlined (due to maxcpus
or whatever reason) are listed in sysfs so i fear such a change would
break compatibility with userspace as well.

  Rakib, don't get me wrong, I don't like to complain but the side effect
of the patch might be pretty inconvenient.

> 
>>  So I still fail to see why we need to drop the former increment in
>> first place.

-- 
    Cyrill

Re: [PATCH] x86, apic: Do not increment disabled_cpus from generic_processor_info.

[Rakib Mullick]

On Mon, Jan 10, 2011 at 3:57 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On 01/10/2011 07:06 AM, Rakib Mullick wrote:
>> On Mon, Jan 10, 2011 at 12:38 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>>
>> When we use nr_cpus=n, it works as an upper limit. If there are any
>> other CPUs beyond that limit those are not counted and we couldn't put
>> them back on work. So, when we couldn't use hotpluging feature to back
>> them into work, should we care about them?
>>
>
>  Yes we should, the cpus which are present but offlined (due to maxcpus
> or whatever reason) are listed in sysfs so i fear such a change would
> break compatibility with userspace as well.
>
For an example, there are 4 CPUs, we did nr_cpus=2 at boot time. Then
using hotpluging feature we can put CPUs online or offline only these
two CPUs, not the 3rd or 4th CPUs that we left out at startup. But, by
incrementing disabled_cpus at generic_processor_info we are accouting
those two left out CPUs. And latter on at cpumask build up (in
smpboot.c) we're using disabled_cpus to build cpumask bit.

So, as you are fearing about sysfs listing, those maybe right but only
for CPUs we have by passing nr_cpus.

>  Rakib, don't get me wrong, I don't like to complain but the side effect
> of the patch might be pretty inconvenient.
>
It's okay. My primary intention was to raise the issue and have some
disscussion about it. Not necessarily, it must have to included. And
it also helps to clear up the concept and that is why was expecting
Ingo's opinion.


thanks,
rakib

> --
>    Cyrill
>