[PATCH] mm: Split huge PUD on wp_huge_pud fallback

[PATCH] mm: Split huge PUD on wp_huge_pud fallback

[Gowans, James]

Currently the implementation will split the PUD when a fallback is taken
inside the create_huge_pud function. This isn't where it should be done:
the splitting should be done in wp_huge_pud, just like it's done for
PMDs. Reason being that if a callback is taken during create, there is
no PUD yet so nothing to split, whereas if a fallback is taken when
encountering a write protection fault there is something to split.

It looks like this was the original intention with the commit where the
splitting was introduced, but somehow it got moved to the wrong place
between v1 and v2 of the patch series. Rebase mistake perhaps.

Fixes: 327e9fd48972 ("mm: Split huge pages on write-notify or COW")

Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: Jan H. Schönherr <jschoenh@amazon.de>
Signed-off-by: James Gowans <jgowans@amazon.com>
---
 mm/memory.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 7a089145cad4..4cf7d4b6c950 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4802,29 +4802,30 @@ static vm_fault_t create_huge_pud(struct vm_fault *vmf)
 	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
 	/* No support for anonymous transparent PUD pages yet */
 	if (vma_is_anonymous(vmf->vma))
-		goto split;
-	if (vmf->vma->vm_ops->huge_fault) {
-		vm_fault_t ret = vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
-
-		if (!(ret & VM_FAULT_FALLBACK))
-			return ret;
-	}
-split:
-	/* COW or write-notify not handled on PUD level: split pud.*/
-	__split_huge_pud(vmf->vma, vmf->pud, vmf->address);
+		return VM_FAULT_FALLBACK;
+	if (vmf->vma->vm_ops->huge_fault)
+		return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 	return VM_FAULT_FALLBACK;
 }
 
 static vm_fault_t wp_huge_pud(struct vm_fault *vmf, pud_t orig_pud)
 {
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+#if defined(CONFIG_TRANSPARENT_HUGEPAGE) &&			\
+	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
 	/* No support for anonymous transparent PUD pages yet */
 	if (vma_is_anonymous(vmf->vma))
-		return VM_FAULT_FALLBACK;
-	if (vmf->vma->vm_ops->huge_fault)
-		return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
-#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+		goto split;
+	if (vmf->vma->vm_ops->huge_fault) {
+		vm_fault_t ret = vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
+
+		if (!(ret & VM_FAULT_FALLBACK))
+			return ret;
+	}
+split:
+	/* COW or write-notify not handled on PUD level: split pud.*/
+	__split_huge_pud(vmf->vma, vmf->pud, vmf->address);
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE && CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
 	return VM_FAULT_FALLBACK;
 }
 
-- 
2.25.1

Re: [PATCH] mm: Split huge PUD on wp_huge_pud fallback

[Thomas Hellström]

On 6/23/22 07:24, Gowans, James wrote:
> Currently the implementation will split the PUD when a fallback is taken
> inside the create_huge_pud function. This isn't where it should be done:
> the splitting should be done in wp_huge_pud, just like it's done for
> PMDs. Reason being that if a callback is taken during create, there is
> no PUD yet so nothing to split, whereas if a fallback is taken when
> encountering a write protection fault there is something to split.
>
> It looks like this was the original intention with the commit where the
> splitting was introduced, but somehow it got moved to the wrong place
> between v1 and v2 of the patch series. Rebase mistake perhaps.
>
> Fixes: 327e9fd48972 ("mm: Split huge pages on write-notify or COW")

Some time since I looked into this, but looks correct to me.

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Jan H. Schönherr <jschoenh@amazon.de>
> Signed-off-by: James Gowans <jgowans@amazon.com>
> ---
>   mm/memory.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 7a089145cad4..4cf7d4b6c950 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4802,29 +4802,30 @@ static vm_fault_t create_huge_pud(struct vm_fault *vmf)
>   	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
>   	/* No support for anonymous transparent PUD pages yet */
>   	if (vma_is_anonymous(vmf->vma))
> -		goto split;
> -	if (vmf->vma->vm_ops->huge_fault) {
> -		vm_fault_t ret = vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
> -
> -		if (!(ret & VM_FAULT_FALLBACK))
> -			return ret;
> -	}
> -split:
> -	/* COW or write-notify not handled on PUD level: split pud.*/
> -	__split_huge_pud(vmf->vma, vmf->pud, vmf->address);
> +		return VM_FAULT_FALLBACK;
> +	if (vmf->vma->vm_ops->huge_fault)
> +		return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
>   #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>   	return VM_FAULT_FALLBACK;
>   }
>   
>   static vm_fault_t wp_huge_pud(struct vm_fault *vmf, pud_t orig_pud)
>   {
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +#if defined(CONFIG_TRANSPARENT_HUGEPAGE) &&			\
> +	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
>   	/* No support for anonymous transparent PUD pages yet */
>   	if (vma_is_anonymous(vmf->vma))
> -		return VM_FAULT_FALLBACK;
> -	if (vmf->vma->vm_ops->huge_fault)
> -		return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
> -#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> +		goto split;
> +	if (vmf->vma->vm_ops->huge_fault) {
> +		vm_fault_t ret = vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
> +
> +		if (!(ret & VM_FAULT_FALLBACK))
> +			return ret;
> +	}
> +split:
> +	/* COW or write-notify not handled on PUD level: split pud.*/
> +	__split_huge_pud(vmf->vma, vmf->pud, vmf->address);
> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE && CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
>   	return VM_FAULT_FALLBACK;
>   }
>   

Re: [PATCH] mm: Split huge PUD on wp_huge_pud fallback

[Andrew Morton]

On Thu, 23 Jun 2022 05:24:03 +0000 "Gowans, James" <jgowans@amazon.com> wrote:

> Currently the implementation will split the PUD when a fallback is taken
> inside the create_huge_pud function. This isn't where it should be done:
> the splitting should be done in wp_huge_pud, just like it's done for
> PMDs. Reason being that if a callback is taken during create, there is
> no PUD yet so nothing to split, whereas if a fallback is taken when
> encountering a write protection fault there is something to split.
> 
> It looks like this was the original intention with the commit where the
> splitting was introduced, but somehow it got moved to the wrong place
> between v1 and v2 of the patch series. Rebase mistake perhaps.

Thanks.  What are the user-visible runtime effects of this change?

Re: [PATCH] mm: Split huge PUD on wp_huge_pud fallback

[David Hildenbrand]

On 23.06.22 07:24, Gowans, James wrote:
> Currently the implementation will split the PUD when a fallback is taken
> inside the create_huge_pud function. This isn't where it should be done:
> the splitting should be done in wp_huge_pud, just like it's done for
> PMDs. Reason being that if a callback is taken during create, there is
> no PUD yet so nothing to split, whereas if a fallback is taken when
> encountering a write protection fault there is something to split.
> 
> It looks like this was the original intention with the commit where the
> splitting was introduced, but somehow it got moved to the wrong place
> between v1 and v2 of the patch series. Rebase mistake perhaps.
> 
> Fixes: 327e9fd48972 ("mm: Split huge pages on write-notify or COW")

Right, the functions should just look like create_huge_pmd()/wp_huge_pmd().

I do wonder if there was a reason to do it differently, though ... I
can't spot any in current code.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb