x86/mm: DEBUG_PAGEALLOC musings

It turns out that commit 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()") was a little too quick to remove the debug_pagealloc exception for cpa_lock.

Notably __kernel_map_pages() is used by the page-allocator from any context the page-allocator itself is used, which violates the cpa_lock rules.

Re-instate the exception, except make it specific to the __kernel_map_pages() such that any other cpa() usage is still fully serialized by cpa_lock. Also note that since cpa() should not be used on memory that isn't allocated, the page-allocator locking and cpa are infact mutually exclusive and all cpa usage in fully serialized.

Add a comment explaining this and other 'funnies' surrounding DEBUG_PAGEALLOC, including how pgd_lock is not affected and the TLB trickery.

Fixes 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

arch/x86/mm/pat/set_memory.c +14 -0

+	 * that isn't allocated.
+	 *
+	 * Additionally, DEBUG_PAGEALLOC ensures (per probe_page_size_mask())
+	 * that the kernel mapping is 4k pages, therefore there are no large
+	 * pages to split/collapse.
+	 *
+	 * Furthermore, the page-allocator strictly manages pages that
+	 * *exist*, avoiding pgd_lock.
+	 *
+	 * Therefore, it is safe to not take cpa_lock.
+	 */
+	if (debug_pagealloc_enabled() && (cpa->flags & CPA_DEBUG_PAGEALLOC))
+		lock = false;
+
 	while (rempages) {
 		/*
 		 * Store the remaining nr of pages for the large page

@@ -2008,9 +2033,12 @@ static int __change_page_attr_set_clr(st
 		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
 			cpa->numpages = 1;

arch/x86/mm/pat/set_memory.c +2 -2

 	if (enable)
-		__set_pages_p(page, numpages);
+		__set_pages_p(page, numpages, CPA_DEBUG_PAGEALLOC);
 	else
-		__set_pages_np(page, numpages);
+		__set_pages_np(page, numpages, CPA_DEBUG_PAGEALLOC);

Quoted 21 lines · Peter Zijlstra

It turns out that commit 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()") was a little too quick to remove the debug_pagealloc exception for cpa_lock.

Notably __kernel_map_pages() is used by the page-allocator from any context the page-allocator itself is used, which violates the cpa_lock rules.

Re-instate the exception, except make it specific to the __kernel_map_pages() such that any other cpa() usage is still fully serialized by cpa_lock. Also note that since cpa() should not be used on memory that isn't allocated, the page-allocator locking and cpa are infact mutually exclusive and all cpa usage in fully serialized.

Add a comment explaining this and other 'funnies' surrounding DEBUG_PAGEALLOC, including how pgd_lock is not affected and the TLB trickery.

Fixes 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

arch/x86/mm/pat/set_memory.c static int __change_page_attr_set_clr(st · line 2011

Quoted 9 lines

arch/x86/mm/pat/set_memory.c +9 -0

+	/*
+	 * DEBUG_PAGEALLOC is special; it is called from any context the
+	 * page-allocator is, which violates the normal cpa_lock locking
+	 * rules.
+	 *
+	 * However, since it is part of the page-allocator, things are still
+	 * properly serialized by the page-allocator locking and the fact that
+	 * when a page is owned by the page-allocator, it isn't owned by
+	 * anybody else. That is, you *SHOULD* not be calling cpa() on memory

*SHOULD NOT* ?

Quoted 21 lines

arch/x86/mm/pat/set_memory.c +14 -0

+	 * that isn't allocated.
+	 *
+	 * Additionally, DEBUG_PAGEALLOC ensures (per probe_page_size_mask())
+	 * that the kernel mapping is 4k pages, therefore there are no large
+	 * pages to split/collapse.
+	 *
+	 * Furthermore, the page-allocator strictly manages pages that
+	 * *exist*, avoiding pgd_lock.
+	 *
+	 * Therefore, it is safe to not take cpa_lock.
+	 */
+	if (debug_pagealloc_enabled() && (cpa->flags & CPA_DEBUG_PAGEALLOC))
+		lock = false;
+
 	while (rempages) {
 		/*
 		 * Store the remaining nr of pages for the large page

@@ -2008,9 +2033,12 @@ static int __change_page_attr_set_clr(st
 		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
 			cpa->numpages = 1;
 

arch/x86/mm/pat/set_memory.c static int __change_page_attr_set_clr(st · line 2041

Quoted 9 lines

arch/x86/mm/pat/set_memory.c +6 -3

-		spin_lock(&cpa_lock);
-		ret = __change_page_attr(cpa, primary);
-		spin_unlock(&cpa_lock);
+		if (lock) {
+			guard(spinlock)(&cpa_lock);
+			ret = __change_page_attr(cpa, primary);
+		} else {
+			ret = __change_page_attr(cpa, primary);
+		}

This does make DEBUG_PAGEALLOC exception more explicit *here*, but OTOH the spin_(un)lock(&cpa_lock) in split_large_page() becomes confusing.

I like my version with your comments added there more as it localizes the DEBUG_PAGEALLOC exception in the lock wrappers.

arch/x86/mm/pat/set_memory.c void __kernel_map_pages(struct page *pag · line 2690

Quoted 6 lines

arch/x86/mm/pat/set_memory.c +0 -0

 		if (ret)
 			goto out;
 

@@ -2661,15 +2689,23 @@ void __kernel_map_pages(struct page *pag
 	 * and hence no memory allocations during large page split.
 	 */

I'd also return early and maybe even WARN if !debug_pagealloc_enabled().

Quoted 6 lines

arch/x86/mm/pat/set_memory.c +2 -2

 	if (enable)
-		__set_pages_p(page, numpages);
+		__set_pages_p(page, numpages, CPA_DEBUG_PAGEALLOC);
 	else
-		__set_pages_np(page, numpages);
+		__set_pages_np(page, numpages, CPA_DEBUG_PAGEALLOC);

Quoted 22 lines · Mike Rapoport

Quoted 21 lines · Peter Zijlstra

It turns out that commit 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()") was a little too quick to remove the debug_pagealloc exception for cpa_lock.

Notably __kernel_map_pages() is used by the page-allocator from any context the page-allocator itself is used, which violates the cpa_lock rules.

Re-instate the exception, except make it specific to the __kernel_map_pages() such that any other cpa() usage is still fully serialized by cpa_lock. Also note that since cpa() should not be used on memory that isn't allocated, the page-allocator locking and cpa are infact mutually exclusive and all cpa usage in fully serialized.

Add a comment explaining this and other 'funnies' surrounding DEBUG_PAGEALLOC, including how pgd_lock is not affected and the TLB trickery.

Fixes 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Quoted 11 lines

arch/x86/mm/pat/set_memory.c static int __change_page_attr_set_clr(st · line 2011

Quoted 9 lines

arch/x86/mm/pat/set_memory.c +9 -0

+	/*
+	 * DEBUG_PAGEALLOC is special; it is called from any context the
+	 * page-allocator is, which violates the normal cpa_lock locking
+	 * rules.
+	 *
+	 * However, since it is part of the page-allocator, things are still
+	 * properly serialized by the page-allocator locking and the fact that
+	 * when a page is owned by the page-allocator, it isn't owned by
+	 * anybody else. That is, you *SHOULD* not be calling cpa() on memory

*SHOULD NOT* ?

Well yeah, d'0h.

Quoted 21 lines

Quoted 21 lines

arch/x86/mm/pat/set_memory.c +14 -0

+	 * that isn't allocated.
+	 *
+	 * Additionally, DEBUG_PAGEALLOC ensures (per probe_page_size_mask())
+	 * that the kernel mapping is 4k pages, therefore there are no large
+	 * pages to split/collapse.
+	 *
+	 * Furthermore, the page-allocator strictly manages pages that
+	 * *exist*, avoiding pgd_lock.
+	 *
+	 * Therefore, it is safe to not take cpa_lock.
+	 */
+	if (debug_pagealloc_enabled() && (cpa->flags & CPA_DEBUG_PAGEALLOC))
+		lock = false;
+
 	while (rempages) {
 		/*
 		 * Store the remaining nr of pages for the large page

@@ -2008,9 +2033,12 @@ static int __change_page_attr_set_clr(st
 		if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY))
 			cpa->numpages = 1;
 

Quoted 12 lines

arch/x86/mm/pat/set_memory.c static int __change_page_attr_set_clr(st · line 2041

Quoted 9 lines

arch/x86/mm/pat/set_memory.c +6 -3

-		spin_lock(&cpa_lock);
-		ret = __change_page_attr(cpa, primary);
-		spin_unlock(&cpa_lock);
+		if (lock) {
+			guard(spinlock)(&cpa_lock);
+			ret = __change_page_attr(cpa, primary);
+		} else {
+			ret = __change_page_attr(cpa, primary);
+		}

This does make DEBUG_PAGEALLOC exception more explicit *here*, but OTOH the spin_(un)lock(&cpa_lock) in split_large_page() becomes confusing.

So as the comment states, with DEBUG_PAGEALLOC there are no large pages, so you should never hit split_large_page().

It is the same as pgd_lock; that isn't guarded anywhere either, and works by the same reasons; DEBUG_PAGEALLOC isn't ever supposed to hit those paths.

Quoted 2 lines

I like my version with your comments added there more as it localizes the DEBUG_PAGEALLOC exception in the lock wrappers.

So I don't like removing cpa_lock entirely; it is still serializing cpa

usage, even though it isn't as critical on 4k only. Having cpa behave significantly different for DEBUG_PAGEALLOC just seems like a very dodgy situation.

And again, pdg_lock is in the same spot. It all works because the code 'magically' never hits the pgd_lock taking paths.

Quoted 8 lines

arch/x86/mm/pat/set_memory.c void __kernel_map_pages(struct page *pag · line 2690

Quoted 6 lines

arch/x86/mm/pat/set_memory.c +0 -0

 		if (ret)
 			goto out;
 

@@ -2661,15 +2689,23 @@ void __kernel_map_pages(struct page *pag
 	 * and hence no memory allocations during large page split.
 	 */

I'd also return early and maybe even WARN if !debug_pagealloc_enabled().

The callsites be like:

	if (debug_pagealloc_enabled_static())
		__kernel_map_pages();

Quoted 6 lines

Quoted 6 lines

arch/x86/mm/pat/set_memory.c +2 -2

 	if (enable)
-		__set_pages_p(page, numpages);
+		__set_pages_p(page, numpages, CPA_DEBUG_PAGEALLOC);
 	else
-		__set_pages_np(page, numpages);
+		__set_pages_np(page, numpages, CPA_DEBUG_PAGEALLOC);

Quoted 4 lines

Quoted 2 lines

I like my version with your comments added there more as it localizes the DEBUG_PAGEALLOC exception in the lock wrappers.

So I don't like removing cpa_lock entirely; it is still serializing cpa

I meant static inline cpa_lock(), not removing it entirely. With your comment why DEBUG_PAGEALLOC is special.

Quoted 7 lines · Mike Rapoport

Quoted 4 lines

Quoted 2 lines

I like my version with your comments added there more as it localizes the DEBUG_PAGEALLOC exception in the lock wrappers.

So I don't like removing cpa_lock entirely; it is still serializing cpa

I meant static inline cpa_lock(), not removing it entirely. With your comment why DEBUG_PAGEALLOC is special.

That is, I despise the whole:

	if (!debug_pagealloc_enabled())
		spin_lock(&cpa_lock);

with a passion. It blanked kills cpa_lock as whole and changes the rules for everyone when debug_pagealloc is on.

cpa_lock ensures that concurrent cpa() calls on an overlapping region have a well defined outcome. By removing it entirely you can get random state.

In practise this isn't likely to matter, but relaxing this for a debug option is insane. Debug stuff should tighten rules, not relax them.

my take on how we should be 'fixing' this DEBUG_PAGEALLOC thing. It appears to boot in qemu with debug_pagealloc=on and such things.

Quoted 2 lines · Peter Zijlstra

my take on how we should be 'fixing' this DEBUG_PAGEALLOC thing. It appears to boot in qemu with debug_pagealloc=on and such things.

The approach looks fine to me. The only thing we might want to do is add a warning of some kind in the split/join paths for CPA_DEBUG_PAGEALLOC.

Quoted 5 lines · Dave Hansen

Quoted 2 lines · Peter Zijlstra

my take on how we should be 'fixing' this DEBUG_PAGEALLOC thing. It appears to boot in qemu with debug_pagealloc=on and such things.

The approach looks fine to me. The only thing we might want to do is add a warning of some kind in the split/join paths for CPA_DEBUG_PAGEALLOC.

So if you have lockdep enabled hitting them will scream bloody murder on cpa_lock and pgd_lock.

But if you want I can sprinkle more explicit warns 🙂