[PATCH] apic: use GFP_ATOMIC in lapic_resume

[PATCH] apic: use GFP_ATOMIC in lapic_resume

[Zhang Rui]

sysdev .suspend/.resume is invoked with irq disabled,
GFP_ATOMIC should be used in lapic_resume.

Without this patch, I got the following warning messages after resume.
[  109.780371] BUG: sleeping function called from invalid context at mm/slub.c:793
[  109.780782] in_atomic(): 0, irqs_disabled(): 1, pid: 1391, name: bash
[  109.781024] Pid: 1391, comm: bash Not tainted 2.6.37-rc5+ #182
[  109.781264] Call Trace:
[  109.781501]  [<ffffffff8104156a>] __might_sleep+0xeb/0xf0
[  109.781743]  [<ffffffff8110d5e2>] slab_pre_alloc_hook.clone.33+0x28/0x31
[  109.781987]  [<ffffffff8110dcef>] __kmalloc+0x88/0x115
[  109.782224]  [<ffffffff81025e4a>] ? kzalloc.clone.19+0x13/0x15
[  109.782465]  [<ffffffff81025e4a>] kzalloc.clone.19+0x13/0x15
[  109.782704]  [<ffffffff81025ff0>] alloc_ioapic_entries+0x20/0x82
[  109.782948]  [<ffffffff81024201>] lapic_resume+0x3a/0x245
[  109.783189]  [<ffffffff813a8329>] ? cpufreq_resume+0x30/0xb0
[  109.783431]  [<ffffffff812ec4ee>] __sysdev_resume+0x25/0xc5
[  109.783673]  [<ffffffff812ec644>] sysdev_resume+0xb6/0xfb
[  109.783914]  [<ffffffff81083256>] suspend_devices_and_enter+0x13c/0x1c1
[  109.784159]  [<ffffffff810833b8>] enter_state+0xdd/0x12e
[  109.784398]  [<ffffffff81082a61>] state_store+0xae/0xcb
[  109.784640]  [<ffffffff81225737>] kobj_attr_store+0x17/0x19
[  109.784882]  [<ffffffff81173311>] sysfs_write_file+0x114/0x150
[  109.785124]  [<ffffffff8111aebb>] vfs_write+0xac/0xff
[  109.785365]  [<ffffffff8111b0c2>] sys_write+0x4a/0x6e
[  109.785604]  [<ffffffff8100ac82>] system_call_fastpath+0x16/0x1b

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/include/asm/io_apic.h |    2 +-
 arch/x86/kernel/apic/apic.c    |    4 ++--
 arch/x86/kernel/apic/io_apic.c |    7 +++----
 3 files changed, 6 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -589,20 +589,19 @@ static int __init ioapic_pirq_setup(char
 __setup("pirq=", ioapic_pirq_setup);
 #endif /* CONFIG_X86_32 */
 
-struct IO_APIC_route_entry **alloc_ioapic_entries(void)
+struct IO_APIC_route_entry **alloc_ioapic_entries(gfp_t flags)
 {
 	int apic;
 	struct IO_APIC_route_entry **ioapic_entries;
 
-	ioapic_entries = kzalloc(sizeof(*ioapic_entries) * nr_ioapics,
-				GFP_KERNEL);
+	ioapic_entries = kzalloc(sizeof(*ioapic_entries) * nr_ioapics, flags);
 	if (!ioapic_entries)
 		return 0;
 
 	for (apic = 0; apic < nr_ioapics; apic++) {
 		ioapic_entries[apic] =
 			kzalloc(sizeof(struct IO_APIC_route_entry) *
-				nr_ioapic_registers[apic], GFP_KERNEL);
+				nr_ioapic_registers[apic], flags);
 		if (!ioapic_entries[apic])
 			goto nomem;
 	}
Index: linux-2.6/arch/x86/include/asm/io_apic.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/io_apic.h
+++ linux-2.6/arch/x86/include/asm/io_apic.h
@@ -162,7 +162,7 @@ void setup_IO_APIC_irq_extra(u32 gsi);
 extern void ioapic_init_mappings(void);
 extern void ioapic_insert_resources(void);
 
-extern struct IO_APIC_route_entry **alloc_ioapic_entries(void);
+extern struct IO_APIC_route_entry **alloc_ioapic_entries(gfp_t flags);
 extern void free_ioapic_entries(struct IO_APIC_route_entry **ioapic_entries);
 extern int save_IO_APIC_setup(struct IO_APIC_route_entry **ioapic_entries);
 extern void mask_IO_APIC_setup(struct IO_APIC_route_entry **ioapic_entries);
Index: linux-2.6/arch/x86/kernel/apic/apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6/arch/x86/kernel/apic/apic.c
@@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
 	if (dmar_table_init_ret && !x2apic_supported())
 		return;
 
-	ioapic_entries = alloc_ioapic_entries();
+	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
 	if (!ioapic_entries) {
 		pr_err("Allocate ioapic_entries failed\n");
 		goto out;
@@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
 
 	local_irq_save(flags);
 	if (intr_remapping_enabled) {
-		ioapic_entries = alloc_ioapic_entries();
+		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
 		if (!ioapic_entries) {
 			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
 			ret = -ENOMEM;

Re: [PATCH] apic: use GFP_ATOMIC in lapic_resume

[David Rientjes]

On Tue, 28 Dec 2010, Zhang Rui wrote:

> Index: linux-2.6/arch/x86/kernel/apic/apic.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> +++ linux-2.6/arch/x86/kernel/apic/apic.c
> @@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
>  	if (dmar_table_init_ret && !x2apic_supported())
>  		return;
>  
> -	ioapic_entries = alloc_ioapic_entries();
> +	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
>  	if (!ioapic_entries) {
>  		pr_err("Allocate ioapic_entries failed\n");
>  		goto out;
> @@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
>  
>  	local_irq_save(flags);
>  	if (intr_remapping_enabled) {
> -		ioapic_entries = alloc_ioapic_entries();
> +		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
>  		if (!ioapic_entries) {
>  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
>  			ret = -ENOMEM;

You can't do the allocation before disabling irqs when 
intr_remapping_enabled is set?

Re: [PATCH] apic: use GFP_ATOMIC in lapic_resume

[Zhang Rui]

On Tue, 2010-12-28 at 15:00 +0800, David Rientjes wrote:
> On Tue, 28 Dec 2010, Zhang Rui wrote:
> 
> > Index: linux-2.6/arch/x86/kernel/apic/apic.c
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> > +++ linux-2.6/arch/x86/kernel/apic/apic.c
> > @@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
> >  	if (dmar_table_init_ret && !x2apic_supported())
> >  		return;
> >  
> > -	ioapic_entries = alloc_ioapic_entries();
> > +	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
> >  	if (!ioapic_entries) {
> >  		pr_err("Allocate ioapic_entries failed\n");
> >  		goto out;
> > @@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
> >  
> >  	local_irq_save(flags);
> >  	if (intr_remapping_enabled) {
> > -		ioapic_entries = alloc_ioapic_entries();
> > +		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
> >  		if (!ioapic_entries) {
> >  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
> >  			ret = -ENOMEM;
> 
> You can't do the allocation before disabling irqs when 
> intr_remapping_enabled is set?

yes, we can. The first idea came into my mind is to register a pm
notifier callback to allocate/free the memory. But that one duplicates
the code of alloc_ioapic_entries, which doesn't look nice, neither.
Plus, is there any problem with this one?

thanks,
rui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] apic: use GFP_ATOMIC in lapic_resume

[David Rientjes]

On Tue, 28 Dec 2010, Zhang Rui wrote:

> > > Index: linux-2.6/arch/x86/kernel/apic/apic.c
> > > ===================================================================
> > > --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> > > +++ linux-2.6/arch/x86/kernel/apic/apic.c
> > > @@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
> > >  	if (dmar_table_init_ret && !x2apic_supported())
> > >  		return;
> > >  
> > > -	ioapic_entries = alloc_ioapic_entries();
> > > +	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
> > >  	if (!ioapic_entries) {
> > >  		pr_err("Allocate ioapic_entries failed\n");
> > >  		goto out;
> > > @@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
> > >  
> > >  	local_irq_save(flags);
> > >  	if (intr_remapping_enabled) {
> > > -		ioapic_entries = alloc_ioapic_entries();
> > > +		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
> > >  		if (!ioapic_entries) {
> > >  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
> > >  			ret = -ENOMEM;
> > 
> > You can't do the allocation before disabling irqs when 
> > intr_remapping_enabled is set?
> 
> yes, we can. The first idea came into my mind is to register a pm
> notifier callback to allocate/free the memory. But that one duplicates
> the code of alloc_ioapic_entries, which doesn't look nice, neither.
> Plus, is there any problem with this one?
> 

We try to avoid GFP_ATOMIC whenever possible and this seems like a 
particularly trivial case.  You can simply move the alloc_ioapic_entries() 
and NULL check before local_irq_save() and GFP_KERNEL will work fine.

Re: [PATCH] apic: use GFP_ATOMIC in lapic_resume

[Zhang Rui]

On Tue, 2010-12-28 at 15:39 +0800, David Rientjes wrote:
> On Tue, 28 Dec 2010, Zhang Rui wrote:
> 
> > > > Index: linux-2.6/arch/x86/kernel/apic/apic.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> > > > +++ linux-2.6/arch/x86/kernel/apic/apic.c
> > > > @@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
> > > >  	if (dmar_table_init_ret && !x2apic_supported())
> > > >  		return;
> > > >  
> > > > -	ioapic_entries = alloc_ioapic_entries();
> > > > +	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
> > > >  	if (!ioapic_entries) {
> > > >  		pr_err("Allocate ioapic_entries failed\n");
> > > >  		goto out;
> > > > @@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
> > > >  
> > > >  	local_irq_save(flags);
> > > >  	if (intr_remapping_enabled) {
> > > > -		ioapic_entries = alloc_ioapic_entries();
> > > > +		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
> > > >  		if (!ioapic_entries) {
> > > >  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
> > > >  			ret = -ENOMEM;
> > > 
> > > You can't do the allocation before disabling irqs when 
> > > intr_remapping_enabled is set?
> > 
> > yes, we can. The first idea came into my mind is to register a pm
> > notifier callback to allocate/free the memory. But that one duplicates
> > the code of alloc_ioapic_entries, which doesn't look nice, neither.
> > Plus, is there any problem with this one?
> > 
> 
> We try to avoid GFP_ATOMIC whenever possible and this seems like a 
> particularly trivial case.  You can simply move the alloc_ioapic_entries() 
> and NULL check before local_irq_save() and GFP_KERNEL will work fine.

I'm afraid not.
lapic_resume is invoked in sysdev_resume, which is done with irq
disabled, please refer to the code in kernel/power/suspend.c.

        arch_suspend_disable_irqs();
        BUG_ON(!irqs_disabled()); 

        error = sysdev_suspend(PMSG_SUSPEND);
        if (!error) {    
                if (!suspend_test(TEST_CORE) &&
pm_check_wakeup_events()) {
                        error = suspend_ops->enter(state);
                        events_check_enabled = false;
                }
                sysdev_resume();
        }       
        
        arch_suspend_enable_irqs();
        BUG_ON(irqs_disabled());

To pre-allocate the memory, we need to build a notifier bloack and call
register_pm_notifier.

thanks,
rui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] apic: use GFP_ATOMIC in lapic_resume

[Zhang Rui]

On Tue, 2010-12-28 at 15:56 +0800, Zhang Rui wrote:
> On Tue, 2010-12-28 at 15:39 +0800, David Rientjes wrote:
> > On Tue, 28 Dec 2010, Zhang Rui wrote:
> > 
> > > > > Index: linux-2.6/arch/x86/kernel/apic/apic.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> > > > > +++ linux-2.6/arch/x86/kernel/apic/apic.c
> > > > > @@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
> > > > >  	if (dmar_table_init_ret && !x2apic_supported())
> > > > >  		return;
> > > > >  
> > > > > -	ioapic_entries = alloc_ioapic_entries();
> > > > > +	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
> > > > >  	if (!ioapic_entries) {
> > > > >  		pr_err("Allocate ioapic_entries failed\n");
> > > > >  		goto out;
> > > > > @@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
> > > > >  
> > > > >  	local_irq_save(flags);
> > > > >  	if (intr_remapping_enabled) {
> > > > > -		ioapic_entries = alloc_ioapic_entries();
> > > > > +		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
> > > > >  		if (!ioapic_entries) {
> > > > >  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
> > > > >  			ret = -ENOMEM;
> > > > 
> > > > You can't do the allocation before disabling irqs when 
> > > > intr_remapping_enabled is set?
> > > 
> > > yes, we can. The first idea came into my mind is to register a pm
> > > notifier callback to allocate/free the memory. But that one duplicates
> > > the code of alloc_ioapic_entries, which doesn't look nice, neither.
> > > Plus, is there any problem with this one?
> > > 
> > 
> > We try to avoid GFP_ATOMIC whenever possible and this seems like a 
> > particularly trivial case.  You can simply move the alloc_ioapic_entries() 
> > and NULL check before local_irq_save() and GFP_KERNEL will work fine.
> 
> I'm afraid not.
> lapic_resume is invoked in sysdev_resume, which is done with irq
> disabled, please refer to the code in kernel/power/suspend.c.
> 
>         arch_suspend_disable_irqs();
>         BUG_ON(!irqs_disabled()); 
> 
>         error = sysdev_suspend(PMSG_SUSPEND);
>         if (!error) {    
>                 if (!suspend_test(TEST_CORE) &&
> pm_check_wakeup_events()) {
>                         error = suspend_ops->enter(state);
>                         events_check_enabled = false;
>                 }
>                 sysdev_resume();
>         }       
>         
>         arch_suspend_enable_irqs();
>         BUG_ON(irqs_disabled());
> 
> To pre-allocate the memory, we need to build a notifier bloack and call
> register_pm_notifier.

what about this one?
Note that it just builds okay, I have not test the patch yet.

pre-allocate the memory used in lapic_resume because lapic_resume, as a
sysdev .resume callback, is always invoked with irq disabled.

Without this patch, I got the following warning messages after resume.
[  109.780371] BUG: sleeping function called from invalid context at
mm/slub.c:793
[  109.780782] in_atomic(): 0, irqs_disabled(): 1, pid: 1391, name: bash
[  109.781024] Pid: 1391, comm: bash Not tainted 2.6.37-rc5+ #182
[  109.781264] Call Trace:
[  109.781501]  [<ffffffff8104156a>] __might_sleep+0xeb/0xf0
[  109.781743]  [<ffffffff8110d5e2>] slab_pre_alloc_hook.clone.33
+0x28/0x31
[  109.781987]  [<ffffffff8110dcef>] __kmalloc+0x88/0x115
[  109.782224]  [<ffffffff81025e4a>] ? kzalloc.clone.19+0x13/0x15
[  109.782465]  [<ffffffff81025e4a>] kzalloc.clone.19+0x13/0x15
[  109.782704]  [<ffffffff81025ff0>] alloc_ioapic_entries+0x20/0x82
[  109.782948]  [<ffffffff81024201>] lapic_resume+0x3a/0x245
[  109.783189]  [<ffffffff813a8329>] ? cpufreq_resume+0x30/0xb0
[  109.783431]  [<ffffffff812ec4ee>] __sysdev_resume+0x25/0xc5
[  109.783673]  [<ffffffff812ec644>] sysdev_resume+0xb6/0xfb
[  109.783914]  [<ffffffff81083256>] suspend_devices_and_enter
+0x13c/0x1c1
[  109.784159]  [<ffffffff810833b8>] enter_state+0xdd/0x12e
[  109.784398]  [<ffffffff81082a61>] state_store+0xae/0xcb
[  109.784640]  [<ffffffff81225737>] kobj_attr_store+0x17/0x19
[  109.784882]  [<ffffffff81173311>] sysfs_write_file+0x114/0x150
[  109.785124]  [<ffffffff8111aebb>] vfs_write+0xac/0xff
[  109.785365]  [<ffffffff8111b0c2>] sys_write+0x4a/0x6e
[  109.785604]  [<ffffffff8100ac82>] system_call_fastpath+0x16/0x1b

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/kernel/apic/apic.c |   38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

Index: linux-2.6/arch/x86/kernel/apic/apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6/arch/x86/kernel/apic/apic.c
@@ -34,6 +34,7 @@
 #include <linux/nmi.h>
 #include <linux/smp.h>
 #include <linux/mm.h>
+#include <linux/suspend.h>
 
 #include <asm/perf_event.h>
 #include <asm/x86_init.h>
@@ -2033,6 +2034,34 @@ static struct {
 	unsigned int apic_thmr;
 } apic_pm_state;
 
+static struct IO_APIC_route_entry **ioapic_entries;
+
+static int lapic_notifier_cb(struct notifier_block *nb,
+			unsigned long val, void *unused)
+{
+	switch(val) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
+	case PM_RESTORE_PREPARE:
+		ioapic_entries = alloc_ioapic_entries();
+		if (!ioapic_entries)
+			return NOTIFY_BAD;
+		return NOTIFY_OK;
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+	case PM_POST_RESTORE:
+		free_ioapic_entries(ioapic_entries);
+		ioapic_entries = NULL;
+		return NOTIFY_OK;
+	}
+	return NOTIFY_BAD;
+}
+
+static struct notifier_block lapic_pm_nb = {
+	.notifier_call = lapic_notifier_cb,
+	.priority = 0,
+};
+
 static int lapic_suspend(struct sys_device *dev, pm_message_t state)
 {
 	unsigned long flags;
@@ -2077,14 +2106,12 @@ static int lapic_resume(struct sys_devic
 	unsigned long flags;
 	int maxlvt;
 	int ret = 0;
-	struct IO_APIC_route_entry **ioapic_entries = NULL;
 
 	if (!apic_pm_state.active)
 		return 0;
 
 	local_irq_save(flags);
 	if (intr_remapping_enabled) {
-		ioapic_entries = alloc_ioapic_entries();
 		if (!ioapic_entries) {
 			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
 			ret = -ENOMEM;
@@ -2094,7 +2121,6 @@ static int lapic_resume(struct sys_devic
 		ret = save_IO_APIC_setup(ioapic_entries);
 		if (ret) {
 			WARN(1, "Saving IO-APIC state failed: %d\n", ret);
-			free_ioapic_entries(ioapic_entries);
 			goto restore;
 		}
 
@@ -2145,7 +2171,6 @@ static int lapic_resume(struct sys_devic
 		reenable_intr_remapping(x2apic_mode);
 		legacy_pic->restore_mask();
 		restore_IO_APIC_setup(ioapic_entries);
-		free_ioapic_entries(ioapic_entries);
 	}
 restore:
 	local_irq_restore(flags);
@@ -2183,8 +2208,11 @@ static int __init init_lapic_sysfs(void)
 	/* XXX: remove suspend/resume procs if !apic_pm_state.active? */
 
 	error = sysdev_class_register(&lapic_sysclass);
-	if (!error)
+	if (!error) {
 		error = sysdev_register(&device_lapic);
+        	if (!error)
+			error = register_pm_notifier(&lapic_pm_nb);
+	}
 	return error;
 }
 

Re: [PATCH] apic: use GFP_ATOMIC in lapic_resume

[Rafael J. Wysocki]

On Tuesday, December 28, 2010, Zhang Rui wrote:
> On Tue, 2010-12-28 at 15:56 +0800, Zhang Rui wrote:
> > On Tue, 2010-12-28 at 15:39 +0800, David Rientjes wrote:
> > > On Tue, 28 Dec 2010, Zhang Rui wrote:
> > > 
> > > > > > Index: linux-2.6/arch/x86/kernel/apic/apic.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> > > > > > +++ linux-2.6/arch/x86/kernel/apic/apic.c
> > > > > > @@ -1459,7 +1459,7 @@ void __init enable_IR_x2apic(void)
> > > > > >  	if (dmar_table_init_ret && !x2apic_supported())
> > > > > >  		return;
> > > > > >  
> > > > > > -	ioapic_entries = alloc_ioapic_entries();
> > > > > > +	ioapic_entries = alloc_ioapic_entries(GFP_KERNEL);
> > > > > >  	if (!ioapic_entries) {
> > > > > >  		pr_err("Allocate ioapic_entries failed\n");
> > > > > >  		goto out;
> > > > > > @@ -2084,7 +2084,7 @@ static int lapic_resume(struct sys_devic
> > > > > >  
> > > > > >  	local_irq_save(flags);
> > > > > >  	if (intr_remapping_enabled) {
> > > > > > -		ioapic_entries = alloc_ioapic_entries();
> > > > > > +		ioapic_entries = alloc_ioapic_entries(GFP_ATOMIC);
> > > > > >  		if (!ioapic_entries) {
> > > > > >  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
> > > > > >  			ret = -ENOMEM;
> > > > > 
> > > > > You can't do the allocation before disabling irqs when 
> > > > > intr_remapping_enabled is set?
> > > > 
> > > > yes, we can. The first idea came into my mind is to register a pm
> > > > notifier callback to allocate/free the memory. But that one duplicates
> > > > the code of alloc_ioapic_entries, which doesn't look nice, neither.
> > > > Plus, is there any problem with this one?
> > > > 
> > > 
> > > We try to avoid GFP_ATOMIC whenever possible and this seems like a 
> > > particularly trivial case.  You can simply move the alloc_ioapic_entries() 
> > > and NULL check before local_irq_save() and GFP_KERNEL will work fine.
> > 
> > I'm afraid not.
> > lapic_resume is invoked in sysdev_resume, which is done with irq
> > disabled, please refer to the code in kernel/power/suspend.c.
> > 
> >         arch_suspend_disable_irqs();
> >         BUG_ON(!irqs_disabled()); 
> > 
> >         error = sysdev_suspend(PMSG_SUSPEND);
> >         if (!error) {    
> >                 if (!suspend_test(TEST_CORE) &&
> > pm_check_wakeup_events()) {
> >                         error = suspend_ops->enter(state);
> >                         events_check_enabled = false;
> >                 }
> >                 sysdev_resume();
> >         }       
> >         
> >         arch_suspend_enable_irqs();
> >         BUG_ON(irqs_disabled());
> > 
> > To pre-allocate the memory, we need to build a notifier bloack and call
> > register_pm_notifier.
> 
> what about this one?

Quite frankly, I very much prefer the simple patch doing an atomic allocation.

Using GFP_KERNEL in a sysdev callback is a plain bug and should be fixed,
preferably in the simplest possible way (ie. replace GFP_KERNEL with
GFP_ATOMIC).

The question whether or not we can avoid allocating memory in there is a
different one and I'm not sure if the solution below is the most
straightforward one.

Thanks,
Rafael


> Note that it just builds okay, I have not test the patch yet.
> 
> pre-allocate the memory used in lapic_resume because lapic_resume, as a
> sysdev .resume callback, is always invoked with irq disabled.
> 
> Without this patch, I got the following warning messages after resume.
> [  109.780371] BUG: sleeping function called from invalid context at
> mm/slub.c:793
> [  109.780782] in_atomic(): 0, irqs_disabled(): 1, pid: 1391, name: bash
> [  109.781024] Pid: 1391, comm: bash Not tainted 2.6.37-rc5+ #182
> [  109.781264] Call Trace:
> [  109.781501]  [<ffffffff8104156a>] __might_sleep+0xeb/0xf0
> [  109.781743]  [<ffffffff8110d5e2>] slab_pre_alloc_hook.clone.33
> +0x28/0x31
> [  109.781987]  [<ffffffff8110dcef>] __kmalloc+0x88/0x115
> [  109.782224]  [<ffffffff81025e4a>] ? kzalloc.clone.19+0x13/0x15
> [  109.782465]  [<ffffffff81025e4a>] kzalloc.clone.19+0x13/0x15
> [  109.782704]  [<ffffffff81025ff0>] alloc_ioapic_entries+0x20/0x82
> [  109.782948]  [<ffffffff81024201>] lapic_resume+0x3a/0x245
> [  109.783189]  [<ffffffff813a8329>] ? cpufreq_resume+0x30/0xb0
> [  109.783431]  [<ffffffff812ec4ee>] __sysdev_resume+0x25/0xc5
> [  109.783673]  [<ffffffff812ec644>] sysdev_resume+0xb6/0xfb
> [  109.783914]  [<ffffffff81083256>] suspend_devices_and_enter
> +0x13c/0x1c1
> [  109.784159]  [<ffffffff810833b8>] enter_state+0xdd/0x12e
> [  109.784398]  [<ffffffff81082a61>] state_store+0xae/0xcb
> [  109.784640]  [<ffffffff81225737>] kobj_attr_store+0x17/0x19
> [  109.784882]  [<ffffffff81173311>] sysfs_write_file+0x114/0x150
> [  109.785124]  [<ffffffff8111aebb>] vfs_write+0xac/0xff
> [  109.785365]  [<ffffffff8111b0c2>] sys_write+0x4a/0x6e
> [  109.785604]  [<ffffffff8100ac82>] system_call_fastpath+0x16/0x1b
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  arch/x86/kernel/apic/apic.c |   38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/arch/x86/kernel/apic/apic.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/apic.c
> +++ linux-2.6/arch/x86/kernel/apic/apic.c
> @@ -34,6 +34,7 @@
>  #include <linux/nmi.h>
>  #include <linux/smp.h>
>  #include <linux/mm.h>
> +#include <linux/suspend.h>
>  
>  #include <asm/perf_event.h>
>  #include <asm/x86_init.h>
> @@ -2033,6 +2034,34 @@ static struct {
>  	unsigned int apic_thmr;
>  } apic_pm_state;
>  
> +static struct IO_APIC_route_entry **ioapic_entries;
> +
> +static int lapic_notifier_cb(struct notifier_block *nb,
> +			unsigned long val, void *unused)
> +{
> +	switch(val) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +	case PM_RESTORE_PREPARE:
> +		ioapic_entries = alloc_ioapic_entries();
> +		if (!ioapic_entries)
> +			return NOTIFY_BAD;
> +		return NOTIFY_OK;
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_SUSPEND:
> +	case PM_POST_RESTORE:
> +		free_ioapic_entries(ioapic_entries);
> +		ioapic_entries = NULL;
> +		return NOTIFY_OK;
> +	}
> +	return NOTIFY_BAD;
> +}
> +
> +static struct notifier_block lapic_pm_nb = {
> +	.notifier_call = lapic_notifier_cb,
> +	.priority = 0,
> +};
> +
>  static int lapic_suspend(struct sys_device *dev, pm_message_t state)
>  {
>  	unsigned long flags;
> @@ -2077,14 +2106,12 @@ static int lapic_resume(struct sys_devic
>  	unsigned long flags;
>  	int maxlvt;
>  	int ret = 0;
> -	struct IO_APIC_route_entry **ioapic_entries = NULL;
>  
>  	if (!apic_pm_state.active)
>  		return 0;
>  
>  	local_irq_save(flags);
>  	if (intr_remapping_enabled) {
> -		ioapic_entries = alloc_ioapic_entries();
>  		if (!ioapic_entries) {
>  			WARN(1, "Alloc ioapic_entries in lapic resume failed.");
>  			ret = -ENOMEM;
> @@ -2094,7 +2121,6 @@ static int lapic_resume(struct sys_devic
>  		ret = save_IO_APIC_setup(ioapic_entries);
>  		if (ret) {
>  			WARN(1, "Saving IO-APIC state failed: %d\n", ret);
> -			free_ioapic_entries(ioapic_entries);
>  			goto restore;
>  		}
>  
> @@ -2145,7 +2171,6 @@ static int lapic_resume(struct sys_devic
>  		reenable_intr_remapping(x2apic_mode);
>  		legacy_pic->restore_mask();
>  		restore_IO_APIC_setup(ioapic_entries);
> -		free_ioapic_entries(ioapic_entries);
>  	}
>  restore:
>  	local_irq_restore(flags);
> @@ -2183,8 +2208,11 @@ static int __init init_lapic_sysfs(void)
>  	/* XXX: remove suspend/resume procs if !apic_pm_state.active? */
>  
>  	error = sysdev_class_register(&lapic_sysclass);
> -	if (!error)
> +	if (!error) {
>  		error = sysdev_register(&device_lapic);
> +        	if (!error)
> +			error = register_pm_notifier(&lapic_pm_nb);
> +	}
>  	return error;
>  }
>  
> 
> 
> 
>