* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 6:44 ` Ingo Molnar
@ 2008-04-25 9:11 ` Gabriel C
2008-04-25 9:19 ` Priit Laes
2008-04-25 9:45 ` Andi Kleen
2 siblings, 0 replies; 9+ messages in thread
From: Gabriel C @ 2008-04-25 9:11 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Priit Laes, linux-kernel, Jack Steiner
Ingo Molnar wrote:
> * Priit Laes <plaes@plaes.org> wrote:
>
>> Hey,
>>
>> I am getting two following warnings (probable one per each CPU core)
>> using the latest Linus's git:
>>
>> WARNING: at arch/x86/kernel/genapic_64.c:86 read_apic_id+0x30/0x62()
>> Modules linked in:
>> Pid: 1, comm: swapper Not tainted 2.6.25-03563-gf7288b1 #20
>
> thanks - could you check the patch below, does it fix the warnings?
Hi Ingo ,
I got the same warning on my Core2Quad Q9300 box , your patch fixes the warning for me.
>
> Ingo
>
> --------------->
> Subject: x86: disable preemption in native_smp_prepare_cpus
> From: Ingo Molnar <mingo@elte.hu>
> Date: Fri Apr 18 11:07:10 CEST 2008
>
> Priit Laes reported the following warning:
>
> Call Trace:
> [<ffffffff8022f1e1>] warn_on_slowpath+0x51/0x63
> [<ffffffff80282e48>] sys_ioctl+0x2d/0x5d
> [<ffffffff805185ff>] _spin_lock+0xe/0x24
> [<ffffffff80227459>] task_rq_lock+0x3d/0x73
> [<ffffffff805133c3>] set_cpu_sibling_map+0x336/0x350
> [<ffffffff8021c1b8>] read_apic_id+0x30/0x62
> [<ffffffff806d921d>] verify_local_APIC+0x90/0x138
> [<ffffffff806d84b5>] native_smp_prepare_cpus+0x1f9/0x305
> [<ffffffff806ce7b1>] kernel_init+0x59/0x2d9
> [<ffffffff80518a26>] _spin_unlock_irq+0x11/0x2b
> [<ffffffff8020bf48>] child_rip+0xa/0x12
> [<ffffffff806ce758>] kernel_init+0x0/0x2d9
> [<ffffffff8020bf3e>] child_rip+0x0/0x12
>
> fix this by generally disabling preemption in native_smp_prepare_cpus().
>
> Reported-and-bisected-by: Priit Laes <plaes@plaes.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> arch/x86/kernel/smpboot.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: linux-x86.q/arch/x86/kernel/smpboot.c
> ===================================================================
> --- linux-x86.q.orig/arch/x86/kernel/smpboot.c
> +++ linux-x86.q/arch/x86/kernel/smpboot.c
> @@ -1181,6 +1181,7 @@ static void __init smp_cpu_index_default
> */
> void __init native_smp_prepare_cpus(unsigned int max_cpus)
> {
> + preempt_disable();
> nmi_watchdog_default();
> smp_cpu_index_default();
> current_cpu_data = boot_cpu_data;
> @@ -1197,7 +1198,7 @@ void __init native_smp_prepare_cpus(unsi
> if (smp_sanity_check(max_cpus) < 0) {
> printk(KERN_INFO "SMP disabled\n");
> disable_smp();
> - return;
> + goto out;
> }
>
> preempt_disable();
> @@ -1237,6 +1238,8 @@ void __init native_smp_prepare_cpus(unsi
> printk(KERN_INFO "CPU%d: ", 0);
> print_cpu_info(&cpu_data(0));
> setup_boot_clock();
> +out:
> + preempt_enable();
> }
> /*
> * Early setup to make printk work.
Regards,
Gabriel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 6:44 ` Ingo Molnar
2008-04-25 9:11 ` Gabriel C
@ 2008-04-25 9:19 ` Priit Laes
2008-04-25 9:45 ` Andi Kleen
2 siblings, 0 replies; 9+ messages in thread
From: Priit Laes @ 2008-04-25 9:19 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linux-kernel, Jack Steiner, Gabriel C
Ühel kenal päeval, R, 2008-04-25 kell 08:44, kirjutas Ingo Molnar:
> * Priit Laes <plaes@plaes.org> wrote:
>
> > Hey,
> >
> > I am getting two following warnings (probable one per each CPU core)
> > using the latest Linus's git:
> >
> > WARNING: at arch/x86/kernel/genapic_64.c:86 read_apic_id+0x30/0x62()
> > Modules linked in:
> > Pid: 1, comm: swapper Not tainted 2.6.25-03563-gf7288b1 #20
>
> thanks - could you check the patch below, does it fix the warnings?
Works for me!
Thanks :)
--
Priit Laes
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 6:44 ` Ingo Molnar
2008-04-25 9:11 ` Gabriel C
2008-04-25 9:19 ` Priit Laes
@ 2008-04-25 9:45 ` Andi Kleen
2008-04-25 11:21 ` Gabriel C
` (2 more replies)
2 siblings, 3 replies; 9+ messages in thread
From: Andi Kleen @ 2008-04-25 9:45 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Priit Laes, linux-kernel, Jack Steiner
Ingo Molnar <mingo@elte.hu> writes:
> * Priit Laes <plaes@plaes.org> wrote:
>
>> Hey,
>>
>> I am getting two following warnings (probable one per each CPU core)
>> using the latest Linus's git:
>>
>> WARNING: at arch/x86/kernel/genapic_64.c:86 read_apic_id+0x30/0x62()
>> Modules linked in:
>> Pid: 1, comm: swapper Not tainted 2.6.25-03563-gf7288b1 #20
>
> thanks - could you check the patch below, does it fix the warnings?
I think it would have been better to change the WARN_ON to
WARN_ON(preemptible() && num_online_cpus() > 1)
Preemption is actually ok here because this runs before the other
CPUs are started
Alternative patch appended.
-Andi
---
Don't warn in read_apic_id() when preemptible but only one CPU online.
Signed-off-by: Andi Kleen <andi@firstfloor.org>
Index: linux/arch/x86/kernel/genapic_64.c
===================================================================
--- linux.orig/arch/x86/kernel/genapic_64.c
+++ linux/arch/x86/kernel/genapic_64.c
@@ -83,7 +83,7 @@ unsigned int read_apic_id(void)
{
unsigned int id;
- WARN_ON(preemptible());
+ WARN_ON(preemptible() && num_online_cpus() > 1);
id = apic_read(APIC_ID);
if (uv_system_type >= UV_X2APIC)
id |= __get_cpu_var(x2apic_extra_bits);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 9:45 ` Andi Kleen
@ 2008-04-25 11:21 ` Gabriel C
2008-04-25 11:41 ` Gabriel C
2008-04-25 15:53 ` Priit Laes
2008-04-28 17:41 ` Ingo Molnar
2 siblings, 1 reply; 9+ messages in thread
From: Gabriel C @ 2008-04-25 11:21 UTC (permalink / raw)
To: Andi Kleen; +Cc: Ingo Molnar, Priit Laes, linux-kernel, Jack Steiner
Andi Kleen wrote:
> Ingo Molnar <mingo@elte.hu> writes:
>
>> * Priit Laes <plaes@plaes.org> wrote:
>>
>>> Hey,
>>>
>>> I am getting two following warnings (probable one per each CPU core)
>>> using the latest Linus's git:
>>>
>>> WARNING: at arch/x86/kernel/genapic_64.c:86 read_apic_id+0x30/0x62()
>>> Modules linked in:
>>> Pid: 1, comm: swapper Not tainted 2.6.25-03563-gf7288b1 #20
>> thanks - could you check the patch below, does it fix the warnings?
>
> I think it would have been better to change the WARN_ON to
>
> WARN_ON(preemptible() && num_online_cpus() > 1)
Hmm , I think you are right , Ingo's patch fixes the warning on boot but now I get one when I reboot the box.
>
> Preemption is actually ok here because this runs before the other
> CPUs are started
>
> Alternative patch appended.
I will test your patch in some minutes.
>
> -Andi
Gabriel
>
> ---
>
> Don't warn in read_apic_id() when preemptible but only one CPU online.
>
> Signed-off-by: Andi Kleen <andi@firstfloor.org>
>
> Index: linux/arch/x86/kernel/genapic_64.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/genapic_64.c
> +++ linux/arch/x86/kernel/genapic_64.c
> @@ -83,7 +83,7 @@ unsigned int read_apic_id(void)
> {
> unsigned int id;
>
> - WARN_ON(preemptible());
> + WARN_ON(preemptible() && num_online_cpus() > 1);
> id = apic_read(APIC_ID);
> if (uv_system_type >= UV_X2APIC)
> id |= __get_cpu_var(x2apic_extra_bits);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 11:21 ` Gabriel C
@ 2008-04-25 11:41 ` Gabriel C
0 siblings, 0 replies; 9+ messages in thread
From: Gabriel C @ 2008-04-25 11:41 UTC (permalink / raw)
To: Andi Kleen; +Cc: Ingo Molnar, Priit Laes, linux-kernel, Jack Steiner
Gabriel C wrote:
> Andi Kleen wrote:
>> Ingo Molnar <mingo@elte.hu> writes:
>>
>>> * Priit Laes <plaes@plaes.org> wrote:
>>>
>>>> Hey,
>>>>
>>>> I am getting two following warnings (probable one per each CPU core)
>>>> using the latest Linus's git:
>>>>
>>>> WARNING: at arch/x86/kernel/genapic_64.c:86 read_apic_id+0x30/0x62()
>>>> Modules linked in:
>>>> Pid: 1, comm: swapper Not tainted 2.6.25-03563-gf7288b1 #20
>>> thanks - could you check the patch below, does it fix the warnings?
>> I think it would have been better to change the WARN_ON to
>>
>> WARN_ON(preemptible() && num_online_cpus() > 1)
>
> Hmm , I think you are right , Ingo's patch fixes the warning on boot but now I get one when I reboot the box.
>
>> Preemption is actually ok here because this runs before the other
>> CPUs are started
>>
>> Alternative patch appended.
>
> I will test your patch in some minutes.
Your patch works better , at least for my box , 4 reboots with your patch and boot/reboot warnings are all gone.
>>
>> Don't warn in read_apic_id() when preemptible but only one CPU online.
>>
>> Signed-off-by: Andi Kleen <andi@firstfloor.org>
>>
>> Index: linux/arch/x86/kernel/genapic_64.c
>> ===================================================================
>> --- linux.orig/arch/x86/kernel/genapic_64.c
>> +++ linux/arch/x86/kernel/genapic_64.c
>> @@ -83,7 +83,7 @@ unsigned int read_apic_id(void)
>> {
>> unsigned int id;
>>
>> - WARN_ON(preemptible());
>> + WARN_ON(preemptible() && num_online_cpus() > 1);
>> id = apic_read(APIC_ID);
>> if (uv_system_type >= UV_X2APIC)
>> id |= __get_cpu_var(x2apic_extra_bits);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 9:45 ` Andi Kleen
2008-04-25 11:21 ` Gabriel C
@ 2008-04-25 15:53 ` Priit Laes
2008-04-28 17:41 ` Ingo Molnar
2 siblings, 0 replies; 9+ messages in thread
From: Priit Laes @ 2008-04-25 15:53 UTC (permalink / raw)
To: Andi Kleen; +Cc: Ingo Molnar, linux-kernel, Jack Steiner
Ühel kenal päeval, R, 2008-04-25 kell 11:45, kirjutas Andi Kleen:
> Ingo Molnar <mingo@elte.hu> writes:
>
> > * Priit Laes <plaes@plaes.org> wrote:
> >
> >> Hey,
> >>
> >> I am getting two following warnings (probable one per each CPU core)
> >> using the latest Linus's git:
> >>
> >> WARNING: at arch/x86/kernel/genapic_64.c:86 read_apic_id+0x30/0x62()
> >> Modules linked in:
> >> Pid: 1, comm: swapper Not tainted 2.6.25-03563-gf7288b1 #20
> >
> > thanks - could you check the patch below, does it fix the warnings?
>
> I think it would have been better to change the WARN_ON to
>
> WARN_ON(preemptible() && num_online_cpus() > 1)
>
> Preemption is actually ok here because this runs before the other
> CPUs are started
>
> Alternative patch appended.
Worksforme :)
--
Priit Laes
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [REGRESSION] x86: support for new UV apic
2008-04-25 9:45 ` Andi Kleen
2008-04-25 11:21 ` Gabriel C
2008-04-25 15:53 ` Priit Laes
@ 2008-04-28 17:41 ` Ingo Molnar
2 siblings, 0 replies; 9+ messages in thread
From: Ingo Molnar @ 2008-04-28 17:41 UTC (permalink / raw)
To: Andi Kleen; +Cc: Priit Laes, linux-kernel, Jack Steiner
* Andi Kleen <andi@firstfloor.org> wrote:
> Alternative patch appended.
> Don't warn in read_apic_id() when preemptible but only one CPU online.
applied, thanks Andi.
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread