Skip to contentSkip to conversation

vunmap() on large regions may trigger soft lockup warnings

PATCH David Vrabel 2 participants 11 messages Dec 11, 2013 – Dec 16, 2013

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

Andrew,

Dietmar Hahn reported an issue where calling vunmap() on a large (50 GB) region would trigger soft lockup warnings.

The following patch would resolve this (by adding a cond_resched() call to vunmap_pmd_range()). Almost calls of vunmap(), unmap_kernel_range() are from process context (as far as I could tell) except for an ACPI driver (drivers/acpi/apei/ghes.c) calls unmap_kernel_range_noflush() from an interrupt and NMI contexts.

Can you advise on a preferred solution?

For example, an unmap_kernel_page() function (callable from atomic context) could be provided since the GHES driver only maps/unmaps a single page.

8<-------------------------
mm/vmalloc: avoid soft lockup warnings when vunmap()'ing large ranges

From: David Vrabel <[email protected]>

If vunmap() is used to unmap a large (e.g., 50 GB) region, it may take sufficiently long that it triggers soft lockup warnings.

Add a cond_resched() into vunmap_pmd_range() so the calling task may be resheduled after unmapping each PMD entry. This is how zap_pmd_range() fixes the same problem for userspace mappings.

---
 mm/vmalloc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
mm/vmalloc.c +0 -0
index 0fdf968..b1b5b39 100644
@@ -75,6 +75,7 @@ static void vunmap_pmd_range(pud_t *pud, unsigned long

addr, unsigned long end)
if (pmd_none_or_clear_bad(pmd))
continue;
vunmap_pte_range(pmd, addr, next);
+ cond_resched();
} while (pmd++, addr = next, addr != end);
}

Signature · 1 line
1.7.2.5
maintainer
#1 lore raw
Quoted 34 lines · On Wed, 11 Dec 2013 16:58:19 +0000 David Vrabel <[email protected]> wrote:

Andrew,

Dietmar Hahn reported an issue where calling vunmap() on a large (50 GB) region would trigger soft lockup warnings.

The following patch would resolve this (by adding a cond_resched() call to vunmap_pmd_range()). Almost calls of vunmap(), unmap_kernel_range() are from process context (as far as I could tell) except for an ACPI driver (drivers/acpi/apei/ghes.c) calls unmap_kernel_range_noflush() from an interrupt and NMI contexts.

Can you advise on a preferred solution?

For example, an unmap_kernel_page() function (callable from atomic context) could be provided since the GHES driver only maps/unmaps a single page.

8<-------------------------
mm/vmalloc: avoid soft lockup warnings when vunmap()'ing large ranges

From: David Vrabel <[email protected]>

If vunmap() is used to unmap a large (e.g., 50 GB) region, it may take sufficiently long that it triggers soft lockup warnings.

Add a cond_resched() into vunmap_pmd_range() so the calling task may be resheduled after unmapping each PMD entry. This is how zap_pmd_range() fixes the same problem for userspace mappings.

...

mm/vmalloc.c static void vunmap_pmd_range(pud_t *pud, unsigned long · line 81

Quoted 7 lines
mm/vmalloc.c +1 -0
@@ -75,6 +75,7 @@ static void vunmap_pmd_range(pud_t *pud, unsigned long
 addr, unsigned long end)
 		if (pmd_none_or_clear_bad(pmd))
 			continue;
 		vunmap_pte_range(pmd, addr, next);
+		cond_resched();
 	} while (pmd++, addr = next, addr != end);
 }

Well that's ugly.

We could redo unmap_kernel_range() so it takes an `atomic' flag then loops around unmapping N MB at a time, doing

if (!atomic)
cond_resched()

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

#2 lore raw
Quoted 59 lines · Andrew Morton
Quoted 34 lines · On Wed, 11 Dec 2013 16:58:19 +0000 David Vrabel <[email protected]> wrote:

Andrew,

Dietmar Hahn reported an issue where calling vunmap() on a large (50 GB) region would trigger soft lockup warnings.

The following patch would resolve this (by adding a cond_resched() call to vunmap_pmd_range()). Almost calls of vunmap(), unmap_kernel_range() are from process context (as far as I could tell) except for an ACPI driver (drivers/acpi/apei/ghes.c) calls unmap_kernel_range_noflush() from an interrupt and NMI contexts.

Can you advise on a preferred solution?

For example, an unmap_kernel_page() function (callable from atomic context) could be provided since the GHES driver only maps/unmaps a single page.

8<-------------------------
mm/vmalloc: avoid soft lockup warnings when vunmap()'ing large ranges

From: David Vrabel <[email protected]>

If vunmap() is used to unmap a large (e.g., 50 GB) region, it may take sufficiently long that it triggers soft lockup warnings.

Add a cond_resched() into vunmap_pmd_range() so the calling task may be resheduled after unmapping each PMD entry. This is how zap_pmd_range() fixes the same problem for userspace mappings.

...

mm/vmalloc.c static void vunmap_pmd_range(pud_t *pud, unsigned long · line 81

Quoted 7 lines
mm/vmalloc.c +1 -0
@@ -75,6 +75,7 @@ static void vunmap_pmd_range(pud_t *pud, unsigned long
 addr, unsigned long end)
 		if (pmd_none_or_clear_bad(pmd))
 			continue;
 		vunmap_pte_range(pmd, addr, next);
+		cond_resched();
 	} while (pmd++, addr = next, addr != end);
 }

Well that's ugly.

We could redo unmap_kernel_range() so it takes an `atomic' flag then loops around unmapping N MB at a time, doing

if (!atomic)
cond_resched()

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

David

maintainer
#3 lore raw
Quoted 14 lines · On Thu, 12 Dec 2013 12:50:47 +0000 David Vrabel <[email protected]> wrote:
Quoted 7 lines

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

Quoted 8 lines

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

Is ghes.c the only problem source here? If so then a suitable solution would be to declare that driver hopelessly busted and proceed as if it didn't exist :(

Just from a quick look, the thing is doing ioremap() from NMI context! ioremap has to do a bunch of memory allocations, takes spinlocks etc.

#4 lore raw
Quoted 21 lines · Andrew Morton
Quoted 19 lines · On Thu, 12 Dec 2013 12:50:47 +0000 David Vrabel <[email protected]> wrote:
Quoted 7 lines

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Quoted 7 lines
Quoted 3 lines

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

Is ghes.c the only problem source here? If so then a suitable solution would be to declare that driver hopelessly busted and proceed as if it didn't exist :(

All the other callers do so from non-atomic context. ghes.c is the only broken caller.

Shall I resend or are you happy to take the patch off the first email in this thread?

David

maintainer
#5 lore raw
Quoted 30 lines · On Mon, 16 Dec 2013 12:56:13 +0000 David Vrabel <[email protected]> wrote:
Quoted 21 lines · Andrew Morton
Quoted 19 lines · On Thu, 12 Dec 2013 12:50:47 +0000 David Vrabel <[email protected]> wrote:
Quoted 7 lines

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Quoted 7 lines
Quoted 3 lines

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

Is ghes.c the only problem source here? If so then a suitable solution would be to declare that driver hopelessly busted and proceed as if it didn't exist :(

Quoted 5 lines

All the other callers do so from non-atomic context. ghes.c is the only broken caller.

Shall I resend or are you happy to take the patch off the first email in this thread?

Well first we should attempt to wake up the ghes maintainers and tell them we're about to break their stuff. (Which I believe is already broken).

The fix won't be easy - presumably ghes will need to punt all its IRQ- and NMI_context operations up into kernel thread context.

maintainer
#6 lore raw
In reply to David Vrabel
Quoted 30 lines · On Mon, 16 Dec 2013 12:56:13 +0000 David Vrabel <[email protected]> wrote:
Quoted 21 lines · Andrew Morton
Quoted 19 lines · On Thu, 12 Dec 2013 12:50:47 +0000 David Vrabel <[email protected]> wrote:
Quoted 7 lines

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Quoted 7 lines
Quoted 3 lines

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

Is ghes.c the only problem source here? If so then a suitable solution would be to declare that driver hopelessly busted and proceed as if it didn't exist :(

Quoted 5 lines

All the other callers do so from non-atomic context. ghes.c is the only broken caller.

Shall I resend or are you happy to take the patch off the first email in this thread?

Well first we should attempt to wake up the ghes maintainers and tell them we're about to break their stuff. (Which I believe is already broken).

The fix won't be easy - presumably ghes will need to punt all its IRQ- and NMI_context operations up into kernel thread context.

#7 lore raw
In reply to Andrew Morton
Quoted 21 lines · Andrew Morton
Quoted 19 lines · On Thu, 12 Dec 2013 12:50:47 +0000 David Vrabel <[email protected]> wrote:
Quoted 7 lines

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Quoted 7 lines
Quoted 3 lines

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

Is ghes.c the only problem source here? If so then a suitable solution would be to declare that driver hopelessly busted and proceed as if it didn't exist :(

All the other callers do so from non-atomic context. ghes.c is the only broken caller.

Shall I resend or are you happy to take the patch off the first email in this thread?

David

maintainer
#8 lore raw
In reply to David Vrabel
Quoted 14 lines · On Thu, 12 Dec 2013 12:50:47 +0000 David Vrabel <[email protected]> wrote:
Quoted 7 lines

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

Quoted 8 lines

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

Is ghes.c the only problem source here? If so then a suitable solution would be to declare that driver hopelessly busted and proceed as if it didn't exist :(

Just from a quick look, the thing is doing ioremap() from NMI context! ioremap has to do a bunch of memory allocations, takes spinlocks etc.

#9 lore raw
In reply to Andrew Morton
Quoted 59 lines · Andrew Morton
Quoted 34 lines · On Wed, 11 Dec 2013 16:58:19 +0000 David Vrabel <[email protected]> wrote:

Andrew,

Dietmar Hahn reported an issue where calling vunmap() on a large (50 GB) region would trigger soft lockup warnings.

The following patch would resolve this (by adding a cond_resched() call to vunmap_pmd_range()). Almost calls of vunmap(), unmap_kernel_range() are from process context (as far as I could tell) except for an ACPI driver (drivers/acpi/apei/ghes.c) calls unmap_kernel_range_noflush() from an interrupt and NMI contexts.

Can you advise on a preferred solution?

For example, an unmap_kernel_page() function (callable from atomic context) could be provided since the GHES driver only maps/unmaps a single page.

8<-------------------------
mm/vmalloc: avoid soft lockup warnings when vunmap()'ing large ranges

From: David Vrabel <[email protected]>

If vunmap() is used to unmap a large (e.g., 50 GB) region, it may take sufficiently long that it triggers soft lockup warnings.

Add a cond_resched() into vunmap_pmd_range() so the calling task may be resheduled after unmapping each PMD entry. This is how zap_pmd_range() fixes the same problem for userspace mappings.

...

mm/vmalloc.c static void vunmap_pmd_range(pud_t *pud, unsigned long · line 81

Quoted 7 lines
mm/vmalloc.c +1 -0
@@ -75,6 +75,7 @@ static void vunmap_pmd_range(pud_t *pud, unsigned long
 addr, unsigned long end)
 		if (pmd_none_or_clear_bad(pmd))
 			continue;
 		vunmap_pte_range(pmd, addr, next);
+		cond_resched();
 	} while (pmd++, addr = next, addr != end);
 }

Well that's ugly.

We could redo unmap_kernel_range() so it takes an `atomic' flag then loops around unmapping N MB at a time, doing

if (!atomic)
cond_resched()

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();
Quoted 6 lines

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.

That would work for the unmap, but looking at the GHES driver some more and it looks like it's call to ioremap_page_range() is already unsafe -- it may need to allocate a new PTE page with a non-atomic alloc in pte_alloc_one_kernel().

Perhaps what's needed here is a pair of ioremap_page_atomic() and iounmap_page_atomic() calls? With some prep function to sure the PTE pages (etc.) are preallocated.

David

maintainer
#10 lore raw
In reply to David Vrabel
Quoted 34 lines · On Wed, 11 Dec 2013 16:58:19 +0000 David Vrabel <[email protected]> wrote:

Andrew,

Dietmar Hahn reported an issue where calling vunmap() on a large (50 GB) region would trigger soft lockup warnings.

The following patch would resolve this (by adding a cond_resched() call to vunmap_pmd_range()). Almost calls of vunmap(), unmap_kernel_range() are from process context (as far as I could tell) except for an ACPI driver (drivers/acpi/apei/ghes.c) calls unmap_kernel_range_noflush() from an interrupt and NMI contexts.

Can you advise on a preferred solution?

For example, an unmap_kernel_page() function (callable from atomic context) could be provided since the GHES driver only maps/unmaps a single page.

8<-------------------------
mm/vmalloc: avoid soft lockup warnings when vunmap()'ing large ranges

From: David Vrabel <[email protected]>

If vunmap() is used to unmap a large (e.g., 50 GB) region, it may take sufficiently long that it triggers soft lockup warnings.

Add a cond_resched() into vunmap_pmd_range() so the calling task may be resheduled after unmapping each PMD entry. This is how zap_pmd_range() fixes the same problem for userspace mappings.

...

mm/vmalloc.c static void vunmap_pmd_range(pud_t *pud, unsigned long · line 81

Quoted 7 lines
mm/vmalloc.c +1 -0
@@ -75,6 +75,7 @@ static void vunmap_pmd_range(pud_t *pud, unsigned long
 addr, unsigned long end)
 		if (pmd_none_or_clear_bad(pmd))
 			continue;
 		vunmap_pte_range(pmd, addr, next);
+		cond_resched();
 	} while (pmd++, addr = next, addr != end);
 }

Well that's ugly.

We could redo unmap_kernel_range() so it takes an `atomic' flag then loops around unmapping N MB at a time, doing

if (!atomic)
cond_resched()

each time. But that would require difficult tuning of N.

I suppose we could just do

	if (!in_interrupt())
		cond_resched();

in vunmap_pmd_range(), but that's pretty specific to ghes.c and doesn't permit unmap-inside-spinlock.

So I can't immediately think of a suitable fix apart from adding a new unmap_kernel_range_atomic(). Then add a `bool atomic' arg to vunmap_page_range() and pass that all the way down.