ioapic: fix potential resume deadlock

Fix a potential deadlock when resuming; here the calling function has disabled interrupts, so we cannot sleep.

Signed-off-by Daniel J Blueman <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |    4 ++--

 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/apic/io_apic.c +1 -1

index 45fd33d..df63620 100644
@@ -621,14 +621,14 @@ struct IO_APIC_route_entry **alloc_ioapic_entries(void)
        struct IO_APIC_route_entry **ioapic_entries;
 
        ioapic_entries = kzalloc(sizeof(*ioapic_entries) * nr_ioapics,
-                               GFP_KERNEL);
+                               GFP_ATOMIC);
        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], GFP_ATOMIC); if (!ioapic_entries[apic])
                       goto nomem;
       }

Fix a potential deadlock when resuming; here the calling function has disabled interrupts, so we cannot sleep.

Signed-off-by Daniel J Blueman <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/apic/io_apic.c +1 -1

index 45fd33d..df63620 100644
@@ -621,14 +621,14 @@ struct IO_APIC_route_entry **alloc_ioapic_entries(void)
 	struct IO_APIC_route_entry **ioapic_entries;
 
 	ioapic_entries = kzalloc(sizeof(*ioapic_entries) * nr_ioapics,
-				GFP_KERNEL);
+				GFP_ATOMIC);
 	if (!ioapic_entries)

Quoted 23 lines · Daniel J Blueman

Fix a potential deadlock when resuming; here the calling function has disabled interrupts, so we cannot sleep.

Signed-off-by Daniel J Blueman <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |    4 ++--

 1 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/apic/io_apic.c +1 -1

index 45fd33d..df63620 100644
@@ -621,14 +621,14 @@ struct IO_APIC_route_entry **alloc_ioapic_entries(void)
       struct IO_APIC_route_entry **ioapic_entries;
 
       ioapic_entries = kzalloc(sizeof(*ioapic_entries) * nr_ioapics,
-                             GFP_KERNEL);
+                             GFP_ATOMIC);
       if (!ioapic_entries)
               return 0;
 
       for (apic = 0; apic < nr_ioapics; apic++) {

Quoted 7 lines

              ioapic_entries[apic] =
                      kzalloc(sizeof(struct IO_APIC_route_entry) *
-                             nr_ioapic_registers[apic], GFP_KERNEL);
+                             nr_ioapic_registers[apic], GFP_ATOMIC); if (!ioapic_entries[apic])
                      goto nomem;
      }

Hm, there must be some other bug here.

ioapic entries should be allocated on system bootup and should never really be deallocated.

The bug might be in the idea to call to enable_IR_x2apic() on resume - why are ioapic entries reallocated there? That call in default_setup_apic_routing() was introduced some time ago in:

 fa47f7e52874: x86, x2apic: Simplify apic init in SMP and UP builds

and that it results in reallocation in the suspend patch is distinctly wrong.

I suspect the reason why this has not triggered for others is the relative rarity of affected systems?

Suresh?

Quoted 9 lines · Ingo Molnar

Hm, there must be some other bug here.

ioapic entries should be allocated on system bootup and should never really be deallocated.

The bug might be in the idea to call to enable_IR_x2apic() on resume - why are ioapic entries reallocated there? That call in default_setup_apic_routing() was introduced some time ago in:

Quoted 8 lines

fa47f7e52874: x86, x2apic: Simplify apic init in SMP and UP builds

and that it results in reallocation in the suspend patch is distinctly wrong.

I suspect the reason why this has not triggered for others is the relative rarity of affected systems?

Suresh?

Quoted 10 lines · Daniel J Blueman

Quoted 9 lines · Ingo Molnar

Hm, there must be some other bug here.

ioapic entries should be allocated on system bootup and should never really be deallocated.

The bug might be in the idea to call to enable_IR_x2apic() on resume - why are ioapic entries reallocated there? That call in default_setup_apic_routing() was introduced some time ago in:

Quoted 8 lines

Quoted 8 lines

 fa47f7e52874: x86, x2apic: Simplify apic init in SMP and UP builds

and that it results in reallocation in the suspend patch is distinctly wrong.

I suspect the reason why this has not triggered for others is the relative rarity of affected systems?

Suresh?

This will happen only on interrupt-remapping enabled systems and hence hidden so far.

enable_IR_x2apic() / default_setup_apic_routing() routines don't get called during resume. Problem is with the lapic_resume() path which is trying to re-enable x2apic mode and this requires masking io-apic entries, re-enabling x2apic and interrupt-remapping, restoring io-apic entries etc. ioapic entry allocation that is happening in lapic_resume() is just for storing the original io-apic RTE's during the mask/restore operations that happen around re-enabling x2apic.

Anyways looking at that code, there is some duplicate code between x2apic code and the io-apic suspend/resume sequence. So a better fix will be to remove this duplicate code.

Daniel, I appended a quick patch which should fix the problem you reported. This is not a final patch, as I can do some more cleanup and re-use the io-apic suspend/resume code a bit more.

If this patch works for you, then I can spend some more time to do the complete cleanup patch in a day or two. Thanks.

Superb, this works, tested against 2.6.39-rc7 and addresses the "BUG:
sleeping function called from invalid context at mm/slub.c:824" warning I was previously seeing. It would be good to get this fix into 2.6.39-final if possible.

Tested-by Daniel J Blueman <[email protected]>

Quoted 6 lines · Daniel J Blueman

Superb, this works, tested against 2.6.39-rc7 and addresses the "BUG:
sleeping function called from invalid context at mm/slub.c:824" warning I was previously seeing. It would be good to get this fix into 2.6.39-final if possible.

Tested-by Daniel J Blueman <[email protected]>

Thanks Daniel for testing my quick patch. I have appended the complete patch which cleans up this code.

Ingo, This patch is relatively big (mostly removes the duplicate code and changes the location where we allocate ioapic_saved_data, so that this can be shared between interrupt-remapping and io-apic suspend/resume flows). May be this can go into 2.6.40-rc1 and probably go to 2.6.39-stable?

Or we can take the Daniel's GFP_ATOMIC patch for 2.6.39 and push this patch for 2.6.40-rc1. I am ok either way.

Hi Suresh,

Quoted 11 lines · Suresh Siddha

Quoted 6 lines · Daniel J Blueman

Superb, this works, tested against 2.6.39-rc7 and addresses the "BUG:
sleeping function called from invalid context at mm/slub.c:824" warning I was previously seeing. It would be good to get this fix into 2.6.39-final if possible.

Tested-by Daniel J Blueman <[email protected]>

Thanks Daniel for testing my quick patch. I have appended the complete patch which cleans up this code.

Quoted 8 lines

Ingo, This patch is relatively big (mostly removes the duplicate code and changes the location where we allocate ioapic_saved_data, so that this can be shared between interrupt-remapping and io-apic suspend/resume flows). May be this can go into 2.6.40-rc1 and probably go to 2.6.39-stable?

Or we can take the Daniel's GFP_ATOMIC patch for 2.6.39 and push this patch for 2.6.40-rc1. I am ok either way.

[]

Testing this, all looks well in that the patch resolves the potentially sleeping allocation, however I do see (on boot) this suspicious message (though suspend and resume does work):

IOAPIC 0: suspend/resume impossible!

I guess it's not expected...

Quoted 24 lines · Daniel J Blueman

Hi Suresh,

Quoted 11 lines · Suresh Siddha

Quoted 6 lines · Daniel J Blueman

Superb, this works, tested against 2.6.39-rc7 and addresses the "BUG:
sleeping function called from invalid context at mm/slub.c:824" warning I was previously seeing. It would be good to get this fix into 2.6.39-final if possible.

Tested-by Daniel J Blueman <[email protected]>

Thanks Daniel for testing my quick patch. I have appended the complete patch which cleans up this code.

Quoted 8 lines

Ingo, This patch is relatively big (mostly removes the duplicate code and changes the location where we allocate ioapic_saved_data, so that this can be shared between interrupt-remapping and io-apic suspend/resume flows). May be this can go into 2.6.40-rc1 and probably go to 2.6.39-stable?

Or we can take the Daniel's GFP_ATOMIC patch for 2.6.39 and push this patch for 2.6.40-rc1. I am ok either way.

[]

Quoted 7 lines

Testing this, all looks well in that the patch resolves the potentially sleeping allocation, however I do see (on boot) this suspicious message (though suspend and resume does work):

IOAPIC 0: suspend/resume impossible!

I guess it's not expected...

No. Has this been introduced by Suresh's patch?

Quoted 6 lines · Daniel J Blueman

Superb, this works, tested against 2.6.39-rc7 and addresses the "BUG:
sleeping function called from invalid context at mm/slub.c:824" warning I was previously seeing. It would be good to get this fix into 2.6.39-final if possible.

Tested-by Daniel J Blueman <[email protected]>

Thanks Daniel for testing my quick patch. I have appended the complete patch which cleans up this code.

Ingo, This patch is relatively big (mostly removes the duplicate code and changes the location where we allocate ioapic_saved_data, so that this can be shared between interrupt-remapping and io-apic suspend/resume flows). May be this can go into 2.6.40-rc1 and probably go to 2.6.39-stable?

 arch/x86/include/asm/io_apic.h |   21 ++----
 arch/x86/kernel/apic/apic.c    |   49 ++++----------
 arch/x86/kernel/apic/io_apic.c |  148 +++++++++++-----------------------------
 3 files changed, 61 insertions(+), 157 deletions(-)