odd lockdep messages

odd lockdep messages

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]

(Not sure when this started, just noticed it... Wasn't present in
2.6.33-rc7-mmotm0210, is in 2.6.33-mmotm0302 and -mmotm0304).

Seen in dmesg:

 dmesg | grep -C 5 'BUG: key'
[    0.978944] Monitor-Mwait will be used to enter C-1 state
[    0.979944] Monitor-Mwait will be used to enter C-2 state
[    0.980943] Monitor-Mwait will be used to enter C-3 state
[    0.981056] Marking TSC unstable due to TSC halts in idle
[    0.981298] Switching to clocksource hpet
[    1.012163] BUG: key ffff88011efbf500 not in .data!
[    1.012284] BUG: key ffff88011efbf548 not in .data!
[    1.015935] thermal LNXTHERM:01: registered as thermal_zone0
[    1.016070] ACPI: Thermal Zone [THM] (39 C)
[    1.022610] Real Time Clock Driver v1.12b
[    1.022955] Linux agpgart interface v0.103
[    1.023342] Hangcheck: starting hangcheck timer 0.9.0 (tick is 180 seconds, margin is 60 seconds).
--
[    1.876870] iwlagn 0000:0c:00.0: setting latency timer to 64
[    1.876914] iwlagn 0000:0c:00.0: Detected Intel Wireless WiFi Link 5100AGN REV=0x54
[    1.899041] iwlagn 0000:0c:00.0: Tunable channels: 13 802.11bg, 24 802.11a channels
[    1.899170] iwlagn 0000:0c:00.0: irq 31 for MSI/MSI-X
[    1.899467] iwlagn 0000:0c:00.0: firmware: requesting iwlwifi-5000-2.ucode
[    1.899537] BUG: key ffff88011c57e670 not in .data!
[    1.899629] console [netcon0] enabled
[    1.899644] netconsole: network logging started
[    1.899699] ohci1394 0000:03:01.1: PCI INT B -> GSI 17 (level, low) -> IRQ 17
[    2.053034] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[    2.052015] ohci1394: fw-host0: Set PHY Reg timeout [0xffffffff/0x00004000/100]
--
[   10.518458] usb 5-1: Manufacturer: Broadcom Corp
[   10.518460] usb 5-1: SerialNumber: 0123456789ABCD
[   10.520315] usb 5-1: config 0 descriptor??
[   10.586699] iwlagn 0000:0c:00.0: request for firmware file 'iwlwifi-5000-2.ucode' failed.
[   10.586873] iwlagn 0000:0c:00.0: firmware: requesting iwlwifi-5000-1.ucode
[   10.586962] BUG: key ffff88011c57e670 not in .data!
[   10.594431] iwlagn 0000:0c:00.0: request for firmware file 'iwlwifi-5000-1.ucode' failed.
[   10.594436] iwlagn 0000:0c:00.0: no suitable firmware found!
[   10.594658] iwlagn 0000:0c:00.0: PCI INT A disabled
[   10.611235] usb 1-4.1: new low speed USB device using ehci_hcd and address 5
[   10.713441] usb 1-4.1: New USB device found, idVendor=045e, idProduct=0023

(-0302 threw BUG: 3 times, the first 2 and the last. In -0304, the third
one appears as well).

Worth instrumenting and chasing down? If so, what should a crash test dummy
be doing here? ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: odd lockdep messages

[Peter Zijlstra]

On Mon, 2010-03-08 at 13:30 -0500, Valdis.Kletnieks@vt.edu wrote:
> (Not sure when this started, just noticed it... Wasn't present in
> 2.6.33-rc7-mmotm0210, is in 2.6.33-mmotm0302 and -mmotm0304).
> 
> Seen in dmesg:
> 
>  dmesg | grep -C 5 'BUG: key'
> [    0.978944] Monitor-Mwait will be used to enter C-1 state
> [    0.979944] Monitor-Mwait will be used to enter C-2 state
> [    0.980943] Monitor-Mwait will be used to enter C-3 state
> [    0.981056] Marking TSC unstable due to TSC halts in idle
> [    0.981298] Switching to clocksource hpet
> [    1.012163] BUG: key ffff88011efbf500 not in .data!
> [    1.012284] BUG: key ffff88011efbf548 not in .data!
> [    1.015935] thermal LNXTHERM:01: registered as thermal_zone0
> [    1.016070] ACPI: Thermal Zone [THM] (39 C)
> [    1.022610] Real Time Clock Driver v1.12b
> [    1.022955] Linux agpgart interface v0.103
> [    1.023342] Hangcheck: starting hangcheck timer 0.9.0 (tick is 180 seconds, margin is 60 seconds).
> --
> [    1.876870] iwlagn 0000:0c:00.0: setting latency timer to 64
> [    1.876914] iwlagn 0000:0c:00.0: Detected Intel Wireless WiFi Link 5100AGN REV=0x54
> [    1.899041] iwlagn 0000:0c:00.0: Tunable channels: 13 802.11bg, 24 802.11a channels
> [    1.899170] iwlagn 0000:0c:00.0: irq 31 for MSI/MSI-X
> [    1.899467] iwlagn 0000:0c:00.0: firmware: requesting iwlwifi-5000-2.ucode
> [    1.899537] BUG: key ffff88011c57e670 not in .data!
> [    1.899629] console [netcon0] enabled
> [    1.899644] netconsole: network logging started
> [    1.899699] ohci1394 0000:03:01.1: PCI INT B -> GSI 17 (level, low) -> IRQ 17
> [    2.053034] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> [    2.052015] ohci1394: fw-host0: Set PHY Reg timeout [0xffffffff/0x00004000/100]
> --
> [   10.518458] usb 5-1: Manufacturer: Broadcom Corp
> [   10.518460] usb 5-1: SerialNumber: 0123456789ABCD
> [   10.520315] usb 5-1: config 0 descriptor??
> [   10.586699] iwlagn 0000:0c:00.0: request for firmware file 'iwlwifi-5000-2.ucode' failed.
> [   10.586873] iwlagn 0000:0c:00.0: firmware: requesting iwlwifi-5000-1.ucode
> [   10.586962] BUG: key ffff88011c57e670 not in .data!
> [   10.594431] iwlagn 0000:0c:00.0: request for firmware file 'iwlwifi-5000-1.ucode' failed.
> [   10.594436] iwlagn 0000:0c:00.0: no suitable firmware found!
> [   10.594658] iwlagn 0000:0c:00.0: PCI INT A disabled
> [   10.611235] usb 1-4.1: new low speed USB device using ehci_hcd and address 5
> [   10.713441] usb 1-4.1: New USB device found, idVendor=045e, idProduct=0023
> 
> (-0302 threw BUG: 3 times, the first 2 and the last. In -0304, the third
> one appears as well).
> 
> Worth instrumenting and chasing down? If so, what should a crash test dummy
> be doing here? ;)

Can that be wreckage due to the new per-cpu stuff?

Its a message printed when the below function fails, and that per-cpu
stuff seems the one most likely to break, given that there was quite a
lot of churn in that department recently.

---

/*
 * Is this the address of a static object:
 */
static int static_obj(void *obj)
{
        unsigned long start = (unsigned long) &_stext,
                      end   = (unsigned long) &_end,
                      addr  = (unsigned long) obj;
#ifdef CONFIG_SMP
        int i;
#endif

        /*
         * static variable?
         */
        if ((addr >= start) && (addr < end))
                return 1;

        if (arch_is_kernel_data(addr))
                return 1;

#ifdef CONFIG_SMP
        /*
         * percpu var?
         */
        for_each_possible_cpu(i) {
                start = (unsigned long) &__per_cpu_start + per_cpu_offset(i);
                end   = (unsigned long) &__per_cpu_start + PERCPU_ENOUGH_ROOM
                                        + per_cpu_offset(i);

                if ((addr >= start) && (addr < end))
                        return 1;
        }
#endif

        /*
         * module var?
         */
        return is_module_address(addr);
}

Re: odd lockdep messages

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On Mon, 08 Mar 2010 19:43:51 +0100, Peter Zijlstra said:
> On Mon, 2010-03-08 at 13:30 -0500, Valdis.Kletnieks@vt.edu wrote:
> > (Not sure when this started, just noticed it... Wasn't present in
> > 2.6.33-rc7-mmotm0210, is in 2.6.33-mmotm0302 and -mmotm0304).
> > 
> > Seen in dmesg:

> > [    1.012163] BUG: key ffff88011efbf500 not in .data!
> > [    1.012284] BUG: key ffff88011efbf548 not in .data!

> Can that be wreckage due to the new per-cpu stuff?
> 
> Its a message printed when the below function fails, and that per-cpu
> stuff seems the one most likely to break, given that there was quite a
> lot of churn in that department recently.

Would it make sense to stick some printk's on the 'return 1' cases

>         /*
>          * static variable?
>          */
>         if ((addr >= start) && (addr < end))
>                 return 1;
> 
>         if (arch_is_kernel_data(addr))
>                 return 1;
> 
> #ifdef CONFIG_SMP
>         /*
>          * percpu var?
>          */
>         for_each_possible_cpu(i) {
>                 start = (unsigned long) &__per_cpu_start + per_cpu_offset(i);
>                 end   = (unsigned long) &__per_cpu_start + PERCPU_ENOUGH_ROOM
>                                         + per_cpu_offset(i);
> 
>                 if ((addr >= start) && (addr < end))
>                         return 1;

or am I setting myself up for printk spam from hell if I do that?

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: odd lockdep messages

[Tejun Heo]

On 03/09/2010 03:43 AM, Peter Zijlstra wrote:
>> [    1.899537] BUG: key ffff88011c57e670 not in .data!
>>
>> (-0302 threw BUG: 3 times, the first 2 and the last. In -0304, the third
>> one appears as well).
>>
>> Worth instrumenting and chasing down? If so, what should a crash test dummy
>> be doing here? ;)
> 
> Can that be wreckage due to the new per-cpu stuff?
> 
> Its a message printed when the below function fails, and that per-cpu
> stuff seems the one most likely to break, given that there was quite a
> lot of churn in that department recently.

Yeap, PERCPU_ENOUGH_ROOM test doesn't hold anymore.  Does the
following patch fix the problem?

Thanks.

diff --git a/include/linux/percpu.h b/include/linux/percpu.h
index a93e5bf..6d300f5 100644
--- a/include/linux/percpu.h
+++ b/include/linux/percpu.h
@@ -137,6 +137,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
 extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
 extern void __percpu *__alloc_percpu(size_t size, size_t align);
 extern void free_percpu(void __percpu *__pdata);
+extern bool is_static_percpu_address(unsigned long addr);
 extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
 
 #ifndef CONFIG_HAVE_SETUP_PER_CPU_AREA
@@ -163,6 +164,12 @@ static inline void free_percpu(void __percpu *p)
 	kfree(p);
 }
 
+/* can't distinguish from other static vars, always %false */
+static inline bool is_static_percpu_address(unsigned long addr)
+{
+	return false;
+}
+
 static inline phys_addr_t per_cpu_ptr_to_phys(void *addr)
 {
 	return __pa(addr);
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 0c30d04..4206f6f 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -582,9 +582,6 @@ static int static_obj(void *obj)
 	unsigned long start = (unsigned long) &_stext,
 		      end   = (unsigned long) &_end,
 		      addr  = (unsigned long) obj;
-#ifdef CONFIG_SMP
-	int i;
-#endif
 
 	/*
 	 * static variable?
@@ -595,19 +592,11 @@ static int static_obj(void *obj)
 	if (arch_is_kernel_data(addr))
 		return 1;
 
-#ifdef CONFIG_SMP
 	/*
 	 * percpu var?
 	 */
-	for_each_possible_cpu(i) {
-		start = (unsigned long) &__per_cpu_start + per_cpu_offset(i);
-		end   = (unsigned long) &__per_cpu_start + PERCPU_ENOUGH_ROOM
-					+ per_cpu_offset(i);
-
-		if ((addr >= start) && (addr < end))
-			return 1;
-	}
-#endif
+	if (is_static_percpu_address(addr))
+		return 1;
 
 	/*
 	 * module var?
diff --git a/mm/percpu.c b/mm/percpu.c
index 768419d..ae4d058 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1304,6 +1304,32 @@ void free_percpu(void __percpu *ptr)
 EXPORT_SYMBOL_GPL(free_percpu);
 
 /**
+ * is_static_percpu_address - test whether address is from static percpu area
+ * @addr: address to test
+ *
+ * Test whether @addr belongs to static percpu area.  Module static
+ * percpu areas allocated via __alloc_reserved_percpu() is not
+ * considered.  Use is_module_address() for those.
+ *
+ * RETURNS:
+ * %true if @addr is from static percpu area, %false otherwise.
+ */
+bool is_static_percpu_address(unsigned long addr)
+{
+	const size_t static_size = __per_cpu_end - __per_cpu_start;
+	void __percpu *base = __addr_to_pcpu_ptr(pcpu_base_addr);
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		void *start = per_cpu_ptr(base, cpu);
+
+		if ((void *)addr >= start && (void *)addr < start + static_size)
+			return true;
+        }
+	return false;
+}
+
+/**
  * per_cpu_ptr_to_phys - convert translated percpu address to physical address
  * @addr: the address to be converted to physical address
  *

-- 
tejun

Re: odd lockdep messages

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Tue, 09 Mar 2010 10:54:20 +0900, Tejun Heo said:
> On 03/09/2010 03:43 AM, Peter Zijlstra wrote:
> >> [    1.899537] BUG: key ffff88011c57e670 not in .data!
> >>
> >> (-0302 threw BUG: 3 times, the first 2 and the last. In -0304, the third
> >> one appears as well).
> >>
> >> Worth instrumenting and chasing down? If so, what should a crash test dummy
> >> be doing here? ;)
> > 
> > Can that be wreckage due to the new per-cpu stuff?
> > 
> > Its a message printed when the below function fails, and that per-cpu
> > stuff seems the one most likely to break, given that there was quite a
> > lot of churn in that department recently.
> 
> Yeap, PERCPU_ENOUGH_ROOM test doesn't hold anymore.  Does the
> following patch fix the problem?

Doesn't seem to make a difference, sorry.

For what it's worth, adding the iwlwifi-5000-2.ucode to the builtin firmware
list fixed the two BUG: messages related to the firmware load.  Not sure if
that tells you anything or not.

However, I still have left:

[    0.997904] Monitor-Mwait will be used to enter C-1 state
[    0.998045] Monitor-Mwait will be used to enter C-2 state
[    0.998184] Monitor-Mwait will be used to enter C-3 state
[    0.998300] Marking TSC unstable due to TSC halts in idle
[    0.998476] Switching to clocksource hpet
[    1.030681] BUG: key ffff88011c8c1d00 not in .data!
[    1.030787] BUG: key ffff88011c8c1d48 not in .data!
[    1.035288] thermal LNXTHERM:01: registered as thermal_zone0
[    1.035405] ACPI: Thermal Zone [THM] (54 C)
[    1.041622] Real Time Clock Driver v1.12b
[    1.041955] Linux agpgart interface v0.103
[    1.042266] Hangcheck: starting hangcheck timer 0.9.0 (tick is 180 seconds, margin is 60 seconds).

I'll stick a 'WAR_ON(1)' next to that BUG printk so we get an idea where it's
happening, probably be morning before that happens though...

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: odd lockdep messages

[Tejun Heo]

Hello,

On 03/09/2010 03:27 PM, Valdis.Kletnieks@vt.edu wrote:
> Doesn't seem to make a difference, sorry.
> 
> For what it's worth, adding the iwlwifi-5000-2.ucode to the builtin firmware
> list fixed the two BUG: messages related to the firmware load.  Not sure if
> that tells you anything or not.
> 
> However, I still have left:
> 
> [    0.997904] Monitor-Mwait will be used to enter C-1 state
> [    0.998045] Monitor-Mwait will be used to enter C-2 state
> [    0.998184] Monitor-Mwait will be used to enter C-3 state
> [    0.998300] Marking TSC unstable due to TSC halts in idle
> [    0.998476] Switching to clocksource hpet
> [    1.030681] BUG: key ffff88011c8c1d00 not in .data!
> [    1.030787] BUG: key ffff88011c8c1d48 not in .data!
> [    1.035288] thermal LNXTHERM:01: registered as thermal_zone0
> [    1.035405] ACPI: Thermal Zone [THM] (54 C)
> [    1.041622] Real Time Clock Driver v1.12b
> [    1.041955] Linux agpgart interface v0.103
> [    1.042266] Hangcheck: starting hangcheck timer 0.9.0 (tick is 180 seconds, margin is 60 seconds).
> 
> I'll stick a 'WAR_ON(1)' next to that BUG printk so we get an idea where it's
> happening, probably be morning before that happens though...

Hmm... the original percpu address check wasn't correct but wasn't too
far off either so it wouldn't be surprising nothing triggered it.
Yeap, stack trace should tell us where the address is coming from.

Thanks.

-- 
tejun

Re: odd lockdep messages

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 5949 bytes --]

On Tue, 09 Mar 2010 15:44:28 +0900, Tejun Heo said:

(adding some cc: from the other 'BUG: key %p not found' thread)

> Hmm... the original percpu address check wasn't correct but wasn't too
> far off either so it wouldn't be surprising nothing triggered it.
> Yeap, stack trace should tell us where the address is coming from.

OK, I stuck in a dump_stack(), and took out the built-in firmware so we'd
get 4 hits rather than 2.  What we got:

[    1.007551] BUG: key ffff88011c8b7d00 not in .data!
[    1.007657] Pid: 1, comm: swapper Not tainted 2.6.33-mmotm0304 #5
[    1.007763] Call Trace:
[    1.007869]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
[    1.007977]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
[    1.008095]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
[    1.008201]  [<ffffffff81135e95>] sysfs_create_file+0x5a/0x63
[    1.008320]  [<ffffffff812fb51c>] device_create_file+0x14/0x16
[    1.008428]  [<ffffffff813d9942>] thermal_zone_device_register+0x3d2/0x63c
[    1.008538]  [<ffffffff81264be9>] acpi_thermal_add+0x2dc/0x59f
[    1.008646]  [<ffffffff8123a50b>] acpi_device_probe+0x4b/0x11d
[    1.008753]  [<ffffffff812fdd25>] driver_probe_device+0xd5/0x166
[    1.008861]  [<ffffffff812fde12>] __driver_attach+0x5c/0x80
[    1.008968]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
[    1.009079]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
[    1.009185]  [<ffffffff812fd31f>] bus_for_each_dev+0x54/0x89
[    1.009301]  [<ffffffff812fdb66>] driver_attach+0x19/0x1b
[    1.009407]  [<ffffffff812fd7b1>] bus_add_driver+0xb4/0x203
[    1.009513]  [<ffffffff812fe10f>] driver_register+0xb8/0x129
[    1.009621]  [<ffffffff81b4b7a0>] ? acpi_thermal_init+0x0/0x7b
[    1.009727]  [<ffffffff8123af12>] acpi_bus_register_driver+0x3e/0x40
[    1.009835]  [<ffffffff81b4b7f9>] acpi_thermal_init+0x59/0x7b
[    1.009942]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
[    1.010064]  [<ffffffff81b2c687>] kernel_init+0x14d/0x1d7
[    1.010171]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
[    1.010289]  [<ffffffff81584040>] ? restore_args+0x0/0x30
[    1.010394]  [<ffffffff81b2c53a>] ? kernel_init+0x0/0x1d7
[    1.010500]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10

[    1.010607] BUG: key ffff88011c8b7d48 not in .data!
[    1.010711] Pid: 1, comm: swapper Not tainted 2.6.33-mmotm0304 #5
[    1.010817] Call Trace:
[    1.010919]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
[    1.011030]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
[    1.011138]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
[    1.011243]  [<ffffffff81135e95>] sysfs_create_file+0x5a/0x63
[    1.011360]  [<ffffffff812fb51c>] device_create_file+0x14/0x16
[    1.011467]  [<ffffffff813d99c1>] thermal_zone_device_register+0x451/0x63c
[    1.011576]  [<ffffffff81264be9>] acpi_thermal_add+0x2dc/0x59f
[    1.011683]  [<ffffffff8123a50b>] acpi_device_probe+0x4b/0x11d
[    1.011790]  [<ffffffff812fdd25>] driver_probe_device+0xd5/0x166
[    1.011897]  [<ffffffff812fde12>] __driver_attach+0x5c/0x80
[    1.012003]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
[    1.012114]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
[    1.012221]  [<ffffffff812fd31f>] bus_for_each_dev+0x54/0x89
[    1.012340]  [<ffffffff812fdb66>] driver_attach+0x19/0x1b
[    1.012446]  [<ffffffff812fd7b1>] bus_add_driver+0xb4/0x203
[    1.012553]  [<ffffffff812fe10f>] driver_register+0xb8/0x129
[    1.012659]  [<ffffffff81b4b7a0>] ? acpi_thermal_init+0x0/0x7b
[    1.012766]  [<ffffffff8123af12>] acpi_bus_register_driver+0x3e/0x40
[    1.012874]  [<ffffffff81b4b7f9>] acpi_thermal_init+0x59/0x7b
[    1.012980]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
[    1.013092]  [<ffffffff81b2c687>] kernel_init+0x14d/0x1d7
[    1.013197]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
[    1.013314]  [<ffffffff81584040>] ? restore_args+0x0/0x30
[    1.013421]  [<ffffffff81b2c53a>] ? kernel_init+0x0/0x1d7
[    1.013526]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10

[    2.859634] BUG: key ffff88011fe16270 not in .data!
[    2.859654] Pid: 828, comm: firmware/iwlwif Not tainted 2.6.33-mmotm0304 #5
[    2.859656] Call Trace:
[    2.859665]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
[    2.859669]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
[    2.859672]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
[    2.859675]  [<ffffffff81137f08>] sysfs_create_bin_file+0x5a/0x63
[    2.859680]  [<ffffffff81303a0f>] _request_firmware+0x421/0x431
[    2.859683]  [<ffffffff81303a1f>] ? request_firmware_work_func+0x0/0x63
[    2.859686]  [<ffffffff81303a59>] request_firmware_work_func+0x3a/0x63
[    2.859690]  [<ffffffff81051d87>] kthread+0x7a/0x82
[    2.859694]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
[    2.859699]  [<ffffffff81584040>] ? restore_args+0x0/0x30
[    2.859702]  [<ffffffff81051d0d>] ? kthread+0x0/0x82
[    2.859704]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10

[   11.534112] BUG: key ffff88011fe16270 not in .data!
[   11.534116] Pid: 1556, comm: firmware/iwlwif Not tainted 2.6.33-mmotm0304 #5
[   11.534118] Call Trace:
[   11.534128]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
[   11.534133]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
[   11.534137]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
[   11.534141]  [<ffffffff81137f08>] sysfs_create_bin_file+0x5a/0x63
[   11.534151]  [<ffffffff81303a1f>] ? request_firmware_work_func+0x0/0x63
[   11.534147]  [<ffffffff81303a0f>] _request_firmware+0x421/0x431
[   11.534154]  [<ffffffff81303a59>] request_firmware_work_func+0x3a/0x63
[   11.534159]  [<ffffffff81051d87>] kthread+0x7a/0x82
[   11.534164]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
[   11.534169]  [<ffffffff81584040>] ? restore_args+0x0/0x30
[   11.534173]  [<ffffffff81051d0d>] ? kthread+0x0/0x82
[   11.534176]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10

So yeah, this looks like more sysfs breakage.

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

[PATCH] percpu,lockdep: implement and use is_static_percpu_address()

[Tejun Heo]

lockdep has custom code to check whether a pointer belongs to static
percpu area which is somewhat broken.  Implement proper
is_static_percpu_address() in percpu and replace the custom code.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
This wasn't the cause this time but I'll queue the fix in percpu tree
anyway.

Thanks.

 include/linux/percpu.h |    7 +++++++
 kernel/lockdep.c       |   15 ++-------------
 mm/percpu.c            |   26 ++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/include/linux/percpu.h b/include/linux/percpu.h
index a93e5bf..1eca064 100644
--- a/include/linux/percpu.h
+++ b/include/linux/percpu.h
@@ -137,6 +137,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
 extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
 extern void __percpu *__alloc_percpu(size_t size, size_t align);
 extern void free_percpu(void __percpu *__pdata);
+extern bool is_static_percpu_address(unsigned long addr);
 extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
 
 #ifndef CONFIG_HAVE_SETUP_PER_CPU_AREA
@@ -163,6 +164,12 @@ static inline void free_percpu(void __percpu *p)
 	kfree(p);
 }
 
+/* can't distinguish from other static vars, always false */
+static inline bool is_static_percpu_address(unsigned long addr)
+{
+	return false;
+}
+
 static inline phys_addr_t per_cpu_ptr_to_phys(void *addr)
 {
 	return __pa(addr);
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 0c30d04..4206f6f 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -582,9 +582,6 @@ static int static_obj(void *obj)
 	unsigned long start = (unsigned long) &_stext,
 		      end   = (unsigned long) &_end,
 		      addr  = (unsigned long) obj;
-#ifdef CONFIG_SMP
-	int i;
-#endif
 
 	/*
 	 * static variable?
@@ -595,19 +592,11 @@ static int static_obj(void *obj)
 	if (arch_is_kernel_data(addr))
 		return 1;
 
-#ifdef CONFIG_SMP
 	/*
 	 * percpu var?
 	 */
-	for_each_possible_cpu(i) {
-		start = (unsigned long) &__per_cpu_start + per_cpu_offset(i);
-		end   = (unsigned long) &__per_cpu_start + PERCPU_ENOUGH_ROOM
-					+ per_cpu_offset(i);
-
-		if ((addr >= start) && (addr < end))
-			return 1;
-	}
-#endif
+	if (is_static_percpu_address(addr))
+		return 1;
 
 	/*
 	 * module var?
diff --git a/mm/percpu.c b/mm/percpu.c
index 768419d..ae4d058 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1304,6 +1304,32 @@ void free_percpu(void __percpu *ptr)
 EXPORT_SYMBOL_GPL(free_percpu);
 
 /**
+ * is_static_percpu_address - test whether address is from static percpu area
+ * @addr: address to test
+ *
+ * Test whether @addr belongs to static percpu area.  Module static
+ * percpu areas allocated via __alloc_reserved_percpu() is not
+ * considered.  Use is_module_address() for those.
+ *
+ * RETURNS:
+ * %true if @addr is from static percpu area, %false otherwise.
+ */
+bool is_static_percpu_address(unsigned long addr)
+{
+	const size_t static_size = __per_cpu_end - __per_cpu_start;
+	void __percpu *base = __addr_to_pcpu_ptr(pcpu_base_addr);
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		void *start = per_cpu_ptr(base, cpu);
+
+		if ((void *)addr >= start && (void *)addr < start + static_size)
+			return true;
+        }
+	return false;
+}
+
+/**
  * per_cpu_ptr_to_phys - convert translated percpu address to physical address
  * @addr: the address to be converted to physical address
  *
-- 
1.6.4.2

Re: [PATCH] percpu,lockdep: implement and use is_static_percpu_address()

[Peter Zijlstra]

On Tue, 2010-03-09 at 18:12 +0900, Tejun Heo wrote:
> lockdep has custom code to check whether a pointer belongs to static
> percpu area which is somewhat broken.  Implement proper
> is_static_percpu_address() in percpu and replace the custom code.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
> This wasn't the cause this time but I'll queue the fix in percpu tree
> anyway.
> 
> Thanks.
> 
>  include/linux/percpu.h |    7 +++++++
>  kernel/lockdep.c       |   15 ++-------------
>  mm/percpu.c            |   26 ++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/percpu.h b/include/linux/percpu.h
> index a93e5bf..1eca064 100644
> --- a/include/linux/percpu.h
> +++ b/include/linux/percpu.h
> @@ -137,6 +137,7 @@ extern int __init pcpu_page_first_chunk(size_t reserved_size,
>  extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
>  extern void __percpu *__alloc_percpu(size_t size, size_t align);
>  extern void free_percpu(void __percpu *__pdata);
> +extern bool is_static_percpu_address(unsigned long addr);
>  extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
>  
>  #ifndef CONFIG_HAVE_SETUP_PER_CPU_AREA
> @@ -163,6 +164,12 @@ static inline void free_percpu(void __percpu *p)
>  	kfree(p);
>  }
>  
> +/* can't distinguish from other static vars, always false */
> +static inline bool is_static_percpu_address(unsigned long addr)
> +{
> +	return false;
> +}

At this point it might make sense to simply fail to compile if lockdep
is enabled as well.

I'm not sure if there's many SMP archs that don't have this and do have
lockdep, but simply failing this test isn't really an option.

>  static inline phys_addr_t per_cpu_ptr_to_phys(void *addr)
>  {
>  	return __pa(addr);
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c

> diff --git a/mm/percpu.c b/mm/percpu.c
> index 768419d..ae4d058 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -1304,6 +1304,32 @@ void free_percpu(void __percpu *ptr)
>  EXPORT_SYMBOL_GPL(free_percpu);
>  
>  /**
> + * is_static_percpu_address - test whether address is from static percpu area
> + * @addr: address to test
> + *
> + * Test whether @addr belongs to static percpu area.  Module static
> + * percpu areas allocated via __alloc_reserved_percpu() is not
> + * considered.  Use is_module_address() for those.
> + *
> + * RETURNS:
> + * %true if @addr is from static percpu area, %false otherwise.
> + */

So is_module_address() will only return true for static per-cpu module
storage, right?

Re: [PATCH] percpu,lockdep: implement and use is_static_percpu_address()

[Tejun Heo]

Hello, Peter.

On 03/09/2010 08:25 PM, Peter Zijlstra wrote:
>> +/* can't distinguish from other static vars, always false */
>> +static inline bool is_static_percpu_address(unsigned long addr)
>> +{
>> +	return false;
>> +}
> 
> At this point it might make sense to simply fail to compile if lockdep
> is enabled as well.
> 
> I'm not sure if there's many SMP archs that don't have this and do have
> lockdep, but simply failing this test isn't really an option.

That might be better.  Returning %false isn't that bad tho.  There
really is no distinction between percpu and !percpu variable on UP and
static variable address match will catch both.

>>  /**
>> + * is_static_percpu_address - test whether address is from static percpu area
>> + * @addr: address to test
>> + *
>> + * Test whether @addr belongs to static percpu area.  Module static
>> + * percpu areas allocated via __alloc_reserved_percpu() is not
>> + * considered.  Use is_module_address() for those.
>> + *
>> + * RETURNS:
>> + * %true if @addr is from static percpu area, %false otherwise.
>> + */
> 
> So is_module_address() will only return true for static per-cpu module
> storage, right?

Right, got confused there.  I'll update is_static_percpu_address() to
test for reserved regions too.

Thanks for the review.

-- 
tejun

Re: [PATCH] percpu,lockdep: implement and use is_static_percpu_address()

[Tejun Heo]

Hello,

On 03/09/2010 08:42 PM, Tejun Heo wrote:
>> So is_module_address() will only return true for static per-cpu module
>> storage, right?
> 
> Right, got confused there.  I'll update is_static_percpu_address() to
> test for reserved regions too.

Which I can't in straight forward manner because only the module code
knows the allocated address ranges on archs which don't require low
offset addresses for module static percpu symbols.  Seems like we'll
need to create is_module_percpu_address() or something.  I'll think
about it.

Thanks.

-- 
tejun

Re: [PATCH] percpu,lockdep: implement and use is_static_percpu_address()

[Peter Zijlstra]

On Tue, 2010-03-09 at 20:42 +0900, Tejun Heo wrote:
> Hello, Peter.
> 
> On 03/09/2010 08:25 PM, Peter Zijlstra wrote:
> >> +/* can't distinguish from other static vars, always false */
> >> +static inline bool is_static_percpu_address(unsigned long addr)
> >> +{
> >> +	return false;
> >> +}
> > 
> > At this point it might make sense to simply fail to compile if lockdep
> > is enabled as well.
> > 
> > I'm not sure if there's many SMP archs that don't have this and do have
> > lockdep, but simply failing this test isn't really an option.
> 
> That might be better.  Returning %false isn't that bad tho.  There
> really is no distinction between percpu and !percpu variable on UP and
> static variable address match will catch both.

Ah, if this is UP only then yes, no complaints ;-)

> >>  /**
> >> + * is_static_percpu_address - test whether address is from static percpu area
> >> + * @addr: address to test
> >> + *
> >> + * Test whether @addr belongs to static percpu area.  Module static
> >> + * percpu areas allocated via __alloc_reserved_percpu() is not
> >> + * considered.  Use is_module_address() for those.
> >> + *
> >> + * RETURNS:
> >> + * %true if @addr is from static percpu area, %false otherwise.
> >> + */
> > 
> > So is_module_address() will only return true for static per-cpu module
> > storage, right?
> 
> Right, got confused there.  I'll update is_static_percpu_address() to
> test for reserved regions too.

Or as per your other email, make the above be true, and have
is_module_address() be true for its static per-cpu regions.

Re: odd lockdep messages

[Greg KH]

On Tue, Mar 09, 2010 at 03:51:32AM -0500, Valdis.Kletnieks@vt.edu wrote:
> On Tue, 09 Mar 2010 15:44:28 +0900, Tejun Heo said:
> 
> (adding some cc: from the other 'BUG: key %p not found' thread)
> 
> > Hmm... the original percpu address check wasn't correct but wasn't too
> > far off either so it wouldn't be surprising nothing triggered it.
> > Yeap, stack trace should tell us where the address is coming from.
> 
> OK, I stuck in a dump_stack(), and took out the built-in firmware so we'd
> get 4 hits rather than 2.  What we got:
> 
> [    1.007551] BUG: key ffff88011c8b7d00 not in .data!
> [    1.007657] Pid: 1, comm: swapper Not tainted 2.6.33-mmotm0304 #5
> [    1.007763] Call Trace:
> [    1.007869]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
> [    1.007977]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
> [    1.008095]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
> [    1.008201]  [<ffffffff81135e95>] sysfs_create_file+0x5a/0x63
> [    1.008320]  [<ffffffff812fb51c>] device_create_file+0x14/0x16
> [    1.008428]  [<ffffffff813d9942>] thermal_zone_device_register+0x3d2/0x63c
> [    1.008538]  [<ffffffff81264be9>] acpi_thermal_add+0x2dc/0x59f
> [    1.008646]  [<ffffffff8123a50b>] acpi_device_probe+0x4b/0x11d
> [    1.008753]  [<ffffffff812fdd25>] driver_probe_device+0xd5/0x166
> [    1.008861]  [<ffffffff812fde12>] __driver_attach+0x5c/0x80
> [    1.008968]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
> [    1.009079]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
> [    1.009185]  [<ffffffff812fd31f>] bus_for_each_dev+0x54/0x89
> [    1.009301]  [<ffffffff812fdb66>] driver_attach+0x19/0x1b
> [    1.009407]  [<ffffffff812fd7b1>] bus_add_driver+0xb4/0x203
> [    1.009513]  [<ffffffff812fe10f>] driver_register+0xb8/0x129
> [    1.009621]  [<ffffffff81b4b7a0>] ? acpi_thermal_init+0x0/0x7b
> [    1.009727]  [<ffffffff8123af12>] acpi_bus_register_driver+0x3e/0x40
> [    1.009835]  [<ffffffff81b4b7f9>] acpi_thermal_init+0x59/0x7b
> [    1.009942]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
> [    1.010064]  [<ffffffff81b2c687>] kernel_init+0x14d/0x1d7
> [    1.010171]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
> [    1.010289]  [<ffffffff81584040>] ? restore_args+0x0/0x30
> [    1.010394]  [<ffffffff81b2c53a>] ? kernel_init+0x0/0x1d7
> [    1.010500]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10
> 
> [    1.010607] BUG: key ffff88011c8b7d48 not in .data!
> [    1.010711] Pid: 1, comm: swapper Not tainted 2.6.33-mmotm0304 #5
> [    1.010817] Call Trace:
> [    1.010919]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
> [    1.011030]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
> [    1.011138]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
> [    1.011243]  [<ffffffff81135e95>] sysfs_create_file+0x5a/0x63
> [    1.011360]  [<ffffffff812fb51c>] device_create_file+0x14/0x16
> [    1.011467]  [<ffffffff813d99c1>] thermal_zone_device_register+0x451/0x63c
> [    1.011576]  [<ffffffff81264be9>] acpi_thermal_add+0x2dc/0x59f
> [    1.011683]  [<ffffffff8123a50b>] acpi_device_probe+0x4b/0x11d
> [    1.011790]  [<ffffffff812fdd25>] driver_probe_device+0xd5/0x166
> [    1.011897]  [<ffffffff812fde12>] __driver_attach+0x5c/0x80
> [    1.012003]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
> [    1.012114]  [<ffffffff812fddb6>] ? __driver_attach+0x0/0x80
> [    1.012221]  [<ffffffff812fd31f>] bus_for_each_dev+0x54/0x89
> [    1.012340]  [<ffffffff812fdb66>] driver_attach+0x19/0x1b
> [    1.012446]  [<ffffffff812fd7b1>] bus_add_driver+0xb4/0x203
> [    1.012553]  [<ffffffff812fe10f>] driver_register+0xb8/0x129
> [    1.012659]  [<ffffffff81b4b7a0>] ? acpi_thermal_init+0x0/0x7b
> [    1.012766]  [<ffffffff8123af12>] acpi_bus_register_driver+0x3e/0x40
> [    1.012874]  [<ffffffff81b4b7f9>] acpi_thermal_init+0x59/0x7b
> [    1.012980]  [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
> [    1.013092]  [<ffffffff81b2c687>] kernel_init+0x14d/0x1d7
> [    1.013197]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
> [    1.013314]  [<ffffffff81584040>] ? restore_args+0x0/0x30
> [    1.013421]  [<ffffffff81b2c53a>] ? kernel_init+0x0/0x1d7
> [    1.013526]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10
> 
> [    2.859634] BUG: key ffff88011fe16270 not in .data!
> [    2.859654] Pid: 828, comm: firmware/iwlwif Not tainted 2.6.33-mmotm0304 #5
> [    2.859656] Call Trace:
> [    2.859665]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
> [    2.859669]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
> [    2.859672]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
> [    2.859675]  [<ffffffff81137f08>] sysfs_create_bin_file+0x5a/0x63
> [    2.859680]  [<ffffffff81303a0f>] _request_firmware+0x421/0x431
> [    2.859683]  [<ffffffff81303a1f>] ? request_firmware_work_func+0x0/0x63
> [    2.859686]  [<ffffffff81303a59>] request_firmware_work_func+0x3a/0x63
> [    2.859690]  [<ffffffff81051d87>] kthread+0x7a/0x82
> [    2.859694]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
> [    2.859699]  [<ffffffff81584040>] ? restore_args+0x0/0x30
> [    2.859702]  [<ffffffff81051d0d>] ? kthread+0x0/0x82
> [    2.859704]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10
> 
> [   11.534112] BUG: key ffff88011fe16270 not in .data!
> [   11.534116] Pid: 1556, comm: firmware/iwlwif Not tainted 2.6.33-mmotm0304 #5
> [   11.534118] Call Trace:
> [   11.534128]  [<ffffffff81061f63>] lockdep_init_map+0xab/0x10d
> [   11.534133]  [<ffffffff81135d83>] sysfs_add_file_mode+0x61/0xa7
> [   11.534137]  [<ffffffff81135dd5>] sysfs_add_file+0xc/0xe
> [   11.534141]  [<ffffffff81137f08>] sysfs_create_bin_file+0x5a/0x63
> [   11.534151]  [<ffffffff81303a1f>] ? request_firmware_work_func+0x0/0x63
> [   11.534147]  [<ffffffff81303a0f>] _request_firmware+0x421/0x431
> [   11.534154]  [<ffffffff81303a59>] request_firmware_work_func+0x3a/0x63
> [   11.534159]  [<ffffffff81051d87>] kthread+0x7a/0x82
> [   11.534164]  [<ffffffff810033d4>] kernel_thread_helper+0x4/0x10
> [   11.534169]  [<ffffffff81584040>] ? restore_args+0x0/0x30
> [   11.534173]  [<ffffffff81051d0d>] ? kthread+0x0/0x82
> [   11.534176]  [<ffffffff810033d0>] ? kernel_thread_helper+0x0/0x10
> 
> So yeah, this looks like more sysfs breakage.

It is due to sysfs, but it is because we forgot to proper initialize a
sysfs attribute somewhere.  Nothing is "broken" here, the system still
works properly.

Eric, care to look into this one as well?

thanks,

greg k-h

[PATCH 1/2] module: encapsulate percpu handling better and record percpu_size

[Tejun Heo]

Better encapsulate module static percpu area handling so that code
outsidef of CONFIG_SMP ifdef doesn't deal with mod->percpu directly
and add mod->percpu_size and record percpu_size in it.  Both percpu
fields are compiled out on UP.  While at it, mark mod->percpu w/
__percpu.

This is to prepare for is_module_percpu_address().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
Rusty, can you please ack this?  Tested on both SMP and UP.

Thanks.

 include/linux/module.h |    5 ++-
 kernel/module.c        |   66 +++++++++++++++++++++++++------------------------
 2 files changed, 39 insertions(+), 32 deletions(-)

Index: work/kernel/module.c
===================================================================
--- work.orig/kernel/module.c
+++ work/kernel/module.c
@@ -370,27 +370,33 @@ EXPORT_SYMBOL_GPL(find_module);
 
 #ifdef CONFIG_SMP
 
-static void *percpu_modalloc(unsigned long size, unsigned long align,
-			     const char *name)
+static inline void __percpu *mod_percpu(struct module *mod)
 {
-	void *ptr;
+	return mod->percpu;
+}
 
+static int percpu_modalloc(struct module *mod,
+			   unsigned long size, unsigned long align)
+{
 	if (align > PAGE_SIZE) {
 		printk(KERN_WARNING "%s: per-cpu alignment %li > %li\n",
-		       name, align, PAGE_SIZE);
+		       mod->name, align, PAGE_SIZE);
 		align = PAGE_SIZE;
 	}
 
-	ptr = __alloc_reserved_percpu(size, align);
-	if (!ptr)
+	mod->percpu = __alloc_reserved_percpu(size, align);
+	if (!mod->percpu) {
 		printk(KERN_WARNING
 		       "Could not allocate %lu bytes percpu data\n", size);
-	return ptr;
+		return -ENOMEM;
+	}
+	mod->percpu_size = size;
+	return 0;
 }
 
-static void percpu_modfree(void *freeme)
+static void percpu_modfree(struct module *mod)
 {
-	free_percpu(freeme);
+	free_percpu(mod->percpu);
 }
 
 static unsigned int find_pcpusec(Elf_Ehdr *hdr,
@@ -400,24 +406,28 @@ static unsigned int find_pcpusec(Elf_Ehd
 	return find_sec(hdr, sechdrs, secstrings, ".data.percpu");
 }
 
-static void percpu_modcopy(void *pcpudest, const void *from, unsigned long size)
+static void percpu_modcopy(struct module *mod,
+			   const void *from, unsigned long size)
 {
 	int cpu;
 
 	for_each_possible_cpu(cpu)
-		memcpy(pcpudest + per_cpu_offset(cpu), from, size);
+		memcpy(per_cpu_ptr(mod->percpu, cpu), from, size);
 }
 
 #else /* ... !CONFIG_SMP */
 
-static inline void *percpu_modalloc(unsigned long size, unsigned long align,
-				    const char *name)
+static inline void __percpu *mod_percpu(struct module *mod)
 {
 	return NULL;
 }
-static inline void percpu_modfree(void *pcpuptr)
+static inline int percpu_modalloc(struct module *mod,
+				  unsigned long size, unsigned long align)
+{
+	return -ENOMEM;
+}
+static inline void percpu_modfree(struct module *mod)
 {
-	BUG();
 }
 static inline unsigned int find_pcpusec(Elf_Ehdr *hdr,
 					Elf_Shdr *sechdrs,
@@ -425,8 +435,8 @@ static inline unsigned int find_pcpusec(
 {
 	return 0;
 }
-static inline void percpu_modcopy(void *pcpudst, const void *src,
-				  unsigned long size)
+static inline void percpu_modcopy(struct module *mod,
+				  const void *from, unsigned long size)
 {
 	/* pcpusec should be 0, and size of that section should be 0. */
 	BUG_ON(size != 0);
@@ -1400,8 +1410,7 @@ static void free_module(struct module *m
 	/* This may be NULL, but that's OK */
 	module_free(mod, mod->module_init);
 	kfree(mod->args);
-	if (mod->percpu)
-		percpu_modfree(mod->percpu);
+	percpu_modfree(mod);
 #if defined(CONFIG_MODULE_UNLOAD)
 	if (mod->refptr)
 		free_percpu(mod->refptr);
@@ -1520,7 +1529,7 @@ static int simplify_symbols(Elf_Shdr *se
 		default:
 			/* Divert to percpu allocation if a percpu var. */
 			if (sym[i].st_shndx == pcpuindex)
-				secbase = (unsigned long)mod->percpu;
+				secbase = (unsigned long)mod_percpu(mod);
 			else
 				secbase = sechdrs[sym[i].st_shndx].sh_addr;
 			sym[i].st_value += secbase;
@@ -1954,7 +1963,7 @@ static noinline struct module *load_modu
 	unsigned int modindex, versindex, infoindex, pcpuindex;
 	struct module *mod;
 	long err = 0;
-	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
+	void *ptr = NULL; /* Stops spurious gcc warning */
 	unsigned long symoffs, stroffs, *strmap;
 
 	mm_segment_t old_fs;
@@ -2094,15 +2103,11 @@ static noinline struct module *load_modu
 
 	if (pcpuindex) {
 		/* We have a special allocation for this section. */
-		percpu = percpu_modalloc(sechdrs[pcpuindex].sh_size,
-					 sechdrs[pcpuindex].sh_addralign,
-					 mod->name);
-		if (!percpu) {
-			err = -ENOMEM;
+		err = percpu_modalloc(mod, sechdrs[pcpuindex].sh_size,
+				      sechdrs[pcpuindex].sh_addralign);
+		if (err)
 			goto free_mod;
-		}
 		sechdrs[pcpuindex].sh_flags &= ~(unsigned long)SHF_ALLOC;
-		mod->percpu = percpu;
 	}
 
 	/* Determine total sizes, and put offsets in sh_entsize.  For now
@@ -2317,7 +2322,7 @@ static noinline struct module *load_modu
 	sort_extable(mod->extable, mod->extable + mod->num_exentries);
 
 	/* Finally, copy percpu area over. */
-	percpu_modcopy(mod->percpu, (void *)sechdrs[pcpuindex].sh_addr,
+	percpu_modcopy(mod, (void *)sechdrs[pcpuindex].sh_addr,
 		       sechdrs[pcpuindex].sh_size);
 
 	add_kallsyms(mod, sechdrs, hdr->e_shnum, symindex, strindex,
@@ -2409,8 +2414,7 @@ static noinline struct module *load_modu
 	module_free(mod, mod->module_core);
 	/* mod will be freed with core. Don't access it beyond this line! */
  free_percpu:
-	if (percpu)
-		percpu_modfree(percpu);
+	percpu_modfree(mod);
  free_mod:
 	kfree(args);
 	kfree(strmap);
Index: work/include/linux/module.h
===================================================================
--- work.orig/include/linux/module.h
+++ work/include/linux/module.h
@@ -329,8 +329,11 @@ struct module
 	struct module_notes_attrs *notes_attrs;
 #endif
 
+#ifdef CONFIG_SMP
 	/* Per-cpu data. */
-	void *percpu;
+	void __percpu *percpu;
+	unsigned int percpu_size;
+#endif
 
 	/* The command line arguments (may be mangled).  People like
 	   keeping pointers to this stuff */

[PATCH 2/2] percpu,module: implement and use is_kernel/module_percpu_address()

[Tejun Heo]

lockdep has custom code to check whether a pointer belongs to static
percpu area which is somewhat broken.  Implement proper
is_kernel/module_percpu_address() and replace the custom code.

On UP, percpu variables are regular static variables and can't be
distinguished from them.  Always return %false on UP.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
I went for is_kernel/module_percpu_address() so that the tests are
orthogonal.  Tested on SMP and UP.  If no one objects, I'll put this
into percpu#for-linus for a while and push it to Linus.

Thanks.

 include/linux/module.h |    1 +
 include/linux/percpu.h |    7 +++++++
 kernel/lockdep.c       |   21 +++++----------------
 kernel/module.c        |   38 ++++++++++++++++++++++++++++++++++++++
 mm/percpu.c            |   26 ++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 16 deletions(-)

Index: work/include/linux/module.h
===================================================================
--- work.orig/include/linux/module.h
+++ work/include/linux/module.h
@@ -394,6 +394,7 @@ static inline int module_is_live(struct
 struct module *__module_text_address(unsigned long addr);
 struct module *__module_address(unsigned long addr);
 bool is_module_address(unsigned long addr);
+bool is_module_percpu_address(unsigned long addr);
 bool is_module_text_address(unsigned long addr);
 
 static inline int within_module_core(unsigned long addr, struct module *mod)
Index: work/include/linux/percpu.h
===================================================================
--- work.orig/include/linux/percpu.h
+++ work/include/linux/percpu.h
@@ -137,6 +137,7 @@ extern int __init pcpu_page_first_chunk(
 extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
 extern void __percpu *__alloc_percpu(size_t size, size_t align);
 extern void free_percpu(void __percpu *__pdata);
+extern bool is_kernel_percpu_address(unsigned long addr);
 extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
 
 #ifndef CONFIG_HAVE_SETUP_PER_CPU_AREA
@@ -163,6 +164,12 @@ static inline void free_percpu(void __pe
 	kfree(p);
 }
 
+/* can't distinguish from other static vars, always false */
+static inline bool is_kernel_percpu_address(unsigned long addr)
+{
+	return false;
+}
+
 static inline phys_addr_t per_cpu_ptr_to_phys(void *addr)
 {
 	return __pa(addr);
Index: work/kernel/lockdep.c
===================================================================
--- work.orig/kernel/lockdep.c
+++ work/kernel/lockdep.c
@@ -582,9 +582,6 @@ static int static_obj(void *obj)
 	unsigned long start = (unsigned long) &_stext,
 		      end   = (unsigned long) &_end,
 		      addr  = (unsigned long) obj;
-#ifdef CONFIG_SMP
-	int i;
-#endif
 
 	/*
 	 * static variable?
@@ -595,24 +592,16 @@ static int static_obj(void *obj)
 	if (arch_is_kernel_data(addr))
 		return 1;
 
-#ifdef CONFIG_SMP
 	/*
-	 * percpu var?
+	 * in-kernel percpu var?
 	 */
-	for_each_possible_cpu(i) {
-		start = (unsigned long) &__per_cpu_start + per_cpu_offset(i);
-		end   = (unsigned long) &__per_cpu_start + PERCPU_ENOUGH_ROOM
-					+ per_cpu_offset(i);
-
-		if ((addr >= start) && (addr < end))
-			return 1;
-	}
-#endif
+	if (is_kernel_percpu_address(addr))
+		return 1;
 
 	/*
-	 * module var?
+	 * module static or percpu var?
 	 */
-	return is_module_address(addr);
+	return is_module_address(addr) || is_module_percpu_address(addr);
 }
 
 /*
Index: work/kernel/module.c
===================================================================
--- work.orig/kernel/module.c
+++ work/kernel/module.c
@@ -415,6 +415,40 @@ static void percpu_modcopy(struct module
 		memcpy(per_cpu_ptr(mod->percpu, cpu), from, size);
 }
 
+/**
+ * is_module_percpu_address - test whether address is from module static percpu
+ * @addr: address to test
+ *
+ * Test whether @addr belongs to module static percpu area.
+ *
+ * RETURNS:
+ * %true if @addr is from module static percpu area
+ */
+bool is_module_percpu_address(unsigned long addr)
+{
+	struct module *mod;
+	unsigned int cpu;
+
+	preempt_disable();
+
+	list_for_each_entry_rcu(mod, &modules, list) {
+		if (!mod->percpu_size)
+			continue;
+		for_each_possible_cpu(cpu) {
+			void *start = per_cpu_ptr(mod->percpu, cpu);
+
+			if ((void *)addr >= start &&
+			    (void *)addr < start + mod->percpu_size) {
+				preempt_enable();
+				return true;
+			}
+		}
+	}
+
+	preempt_enable();
+	return false;
+}
+
 #else /* ... !CONFIG_SMP */
 
 static inline void __percpu *mod_percpu(struct module *mod)
@@ -441,6 +475,10 @@ static inline void percpu_modcopy(struct
 	/* pcpusec should be 0, and size of that section should be 0. */
 	BUG_ON(size != 0);
 }
+bool is_module_percpu_address(unsigned long addr)
+{
+	return false;
+}
 
 #endif /* CONFIG_SMP */
 
Index: work/mm/percpu.c
===================================================================
--- work.orig/mm/percpu.c
+++ work/mm/percpu.c
@@ -1304,6 +1304,32 @@ void free_percpu(void __percpu *ptr)
 EXPORT_SYMBOL_GPL(free_percpu);
 
 /**
+ * is_kernel_percpu_address - test whether address is from static percpu area
+ * @addr: address to test
+ *
+ * Test whether @addr belongs to in-kernel static percpu area.  Module
+ * static percpu areas are not considered.  For those, use
+ * is_module_percpu_address().
+ *
+ * RETURNS:
+ * %true if @addr is from in-kernel static percpu area, %false otherwise.
+ */
+bool is_kernel_percpu_address(unsigned long addr)
+{
+	const size_t static_size = __per_cpu_end - __per_cpu_start;
+	void __percpu *base = __addr_to_pcpu_ptr(pcpu_base_addr);
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		void *start = per_cpu_ptr(base, cpu);
+
+		if ((void *)addr >= start && (void *)addr < start + static_size)
+			return true;
+        }
+	return false;
+}
+
+/**
  * per_cpu_ptr_to_phys - convert translated percpu address to physical address
  * @addr: the address to be converted to physical address
  *

Re: [PATCH 2/2] percpu,module: implement and use is_kernel/module_percpu_address()

[Peter Zijlstra]

On Wed, 2010-03-10 at 18:57 +0900, Tejun Heo wrote:
> lockdep has custom code to check whether a pointer belongs to static
> percpu area which is somewhat broken.  Implement proper
> is_kernel/module_percpu_address() and replace the custom code.
> 
> On UP, percpu variables are regular static variables and can't be
> distinguished from them.  Always return %false on UP.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>

Looks good to me, thanks Tejun!

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Re: [PATCH 1/2] module: encapsulate percpu handling better and record percpu_size

[Rusty Russell]

On Wed, 10 Mar 2010 08:26:10 pm Tejun Heo wrote:
> Better encapsulate module static percpu area handling so that code
> outsidef of CONFIG_SMP ifdef doesn't deal with mod->percpu directly
> and add mod->percpu_size and record percpu_size in it.  Both percpu
> fields are compiled out on UP.  While at it, mark mod->percpu w/
> __percpu.

Nice cleanup.

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks!
Rusty.

Re: [PATCH 1/2] module: encapsulate percpu handling better and record percpu_size

[Tejun Heo]

On 03/29/2010 05:26 PM, Rusty Russell wrote:
> On Wed, 10 Mar 2010 08:26:10 pm Tejun Heo wrote:
>> Better encapsulate module static percpu area handling so that code
>> outsidef of CONFIG_SMP ifdef doesn't deal with mod->percpu directly
>> and add mod->percpu_size and record percpu_size in it.  Both percpu
>> fields are compiled out on UP.  While at it, mark mod->percpu w/
>> __percpu.
> 
> Nice cleanup.
> 
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Great.  Patches are pushed out to linux-next with your and Peter's
ACKs added.  I'll ask Linus to pull it after several days.

Thanks!

-- 
tejun