From: Dave Hansen <dave.hansen@linux.intel.com>

This block of code used to be:

	if (SHARED_KERNEL_PMD)

But it was zapped when 32-bit kernels transitioned to private
(non-shared) PMDs. It also made it rather unclear what the block
of code is doing in the first place.

Remove the #ifdef and replace it with IS_ENABLED(). Unindent the
code block and add an actually useful comment about what it is
doing.

Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/mm/pat/set_memory.c |   41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff -puN arch/x86/mm/pat/set_memory.c~kill-CONFIG_X86_32-ifdef arch/x86/mm/pat/set_memory.c
--- a/arch/x86/mm/pat/set_memory.c~kill-CONFIG_X86_32-ifdef	2025-04-18 08:37:32.149932662 -0700
+++ b/arch/x86/mm/pat/set_memory.c	2025-04-18 08:37:32.152932772 -0700
@@ -881,31 +881,32 @@ phys_addr_t slow_virt_to_phys(void *__vi
 }
 EXPORT_SYMBOL_GPL(slow_virt_to_phys);
 
-/*
- * Set the new pmd in all the pgds we know about:
- */
 static void __set_pmd_pte(pte_t *kpte, unsigned long address, pte_t pte)
 {
+	struct page *page;
+
 	/* change init_mm */
 	set_pte_atomic(kpte, pte);
-#ifdef CONFIG_X86_32
-	{
-		struct page *page;
-
-		list_for_each_entry(page, &pgd_list, lru) {
-			pgd_t *pgd;
-			p4d_t *p4d;
-			pud_t *pud;
-			pmd_t *pmd;
-
-			pgd = (pgd_t *)page_address(page) + pgd_index(address);
-			p4d = p4d_offset(pgd, address);
-			pud = pud_offset(p4d, address);
-			pmd = pmd_offset(pud, address);
-			set_pte_atomic((pte_t *)pmd, pte);
-		}
+
+	if (IS_ENABLED(CONFIG_X86_64))
+		return;
+
+	/*
+	 * 32-bit mm_structs don't share kernel PMD pages.
+	 * Propagate the change to each relevant PMD entry:
+	 */
+	list_for_each_entry(page, &pgd_list, lru) {
+		pgd_t *pgd;
+		p4d_t *p4d;
+		pud_t *pud;
+		pmd_t *pmd;
+
+		pgd = (pgd_t *)page_address(page) + pgd_index(address);
+		p4d = p4d_offset(pgd, address);
+		pud = pud_offset(p4d, address);
+		pmd = pmd_offset(pud, address);
+		set_pte_atomic((pte_t *)pmd, pte);
 	}
-#endif
 }
 
 static pgprot_t pgprot_clear_protnone_bits(pgprot_t prot)
_

On Fri, 2025-04-18 at 08:56 -0700, Dave Hansen wrote:
> +
> +	if (IS_ENABLED(CONFIG_X86_64))
> +		return;

Nit to throw away if you don't like it, but the below code the conditional is
about special 32 bit requirements, not, not being 64 bit. So I'd have done:

	if (!IS_ENABLED(CONFIG_X86_32))
		return;

Probably anyone reading this is going to know CONFIG_X86_64 and CONFIG_X86_32
are exclusive, and there are only two options. But to me the check is a tiny bit
harder to read this way. In either case:

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

> +
> +	/*
> +	 * 32-bit mm_structs don't share kernel PMD pages.
> +	 * Propagate the change to each relevant PMD entry:
> +	 */
> +	list_for_each_entry(page, &pgd_list, lru) {
> +		pgd_t *pgd;
> +		p4d_t *p4d;
> +		pud_t *pud;
> +		pmd_t *pmd;
> +
> +		pgd = (pgd_t *)page_address(page) + pgd_index(address);
> +		p4d = p4d_offset(pgd, address);
> +		pud = pud_offset(p4d, address);
> +		pmd = pmd_offset(pud, address);
> +		set_pte_atomic((pte_t *)pmd, pte);
>  	}

On 4/18/25 11:18, Edgecombe, Rick P wrote:
> On Fri, 2025-04-18 at 08:56 -0700, Dave Hansen wrote:
>> +
>> +	if (IS_ENABLED(CONFIG_X86_64))
>> +		return;
> Nit to throw away if you don't like it, but the below code the conditional is
> about special 32 bit requirements, not, not being 64 bit. So I'd have done:
> 
> 	if (!IS_ENABLED(CONFIG_X86_32))
> 		return;
> 
> Probably anyone reading this is going to know CONFIG_X86_64 and CONFIG_X86_32
> are exclusive, and there are only two options. But to me the check is a tiny bit
> harder to read this way. In either case:

I like the suggestion. I think I even wrote it that way originally.

I eventually decided to try and optimize for the lucky guy who comes
through some day and is removing all the non-64-bit code. It would be
easier for them to intuit that the cruft can go away here:

	if (IS_ENABLED(CONFIG_X86_64))
		return;
	// cruft

Does that make sense, or am I optimizing for the wrong thing?

On Fri, 2025-04-18 at 11:49 -0700, Dave Hansen wrote:
> I like the suggestion. I think I even wrote it that way originally.
> 
> I eventually decided to try and optimize for the lucky guy who comes
> through some day and is removing all the non-64-bit code. It would be
> easier for them to intuit that the cruft can go away here:
> 
> 	if (IS_ENABLED(CONFIG_X86_64))
> 		return;
> 	// cruft
> 
> Does that make sense, or am I optimizing for the wrong thing?

Hmm, we might expect such a person to be in a gleeful, tolerant mood. Also well
practiced in reasoning about similar conditionals. I see your point though.