Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1646896 > unrolled thread

[PATCH v3 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Punit Agrawal <punit.agrawal@arm.com> - 2017-05-22 15:40 +0200
    Re: [PATCH v3 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Arnd Bergmann <arnd@arndb.de> - 2017-05-22 16:00 +0200
      Re: [PATCH v3 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Punit Agrawal <punit.agrawal@arm.com> - 2017-05-22 17:50 +0200
    [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Punit Agrawal <punit.agrawal@arm.com> - 2017-05-22 18:30 +0200
      Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Arnd Bergmann <arnd@arndb.de> - 2017-05-22 22:40 +0200
        Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Punit Agrawal <punit.agrawal@arm.com> - 2017-05-23 17:00 +0200
      Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override  huge_pte_clear() Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-05-23 07:30 +0200
        Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear() Punit Agrawal <punit.agrawal@arm.com> - 2017-05-23 17:00 +0200

#1646896 — [PATCH v3 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

When unmapping a hugepage range, huge_pte_clear() is used to clear the
page table entries that are marked as not present. huge_pte_clear()
internally just ends up calling pte_clear() which does not correctly
deal with hugepages consisting of contiguous page table entries.

Add a size argument to address this issue and allow architectures to
override huge_pte_clear() in subsequent patches by making it a weak
function.

Update the s390 to use the new mechanism to override huge_pte_clear().

Note that the change only affects huge_pte_clear() - the other generic
hugetlb functions don't need any change.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
---
 arch/s390/include/asm/hugetlb.h | 10 ++--------
 arch/s390/mm/hugetlbpage.c      |  9 +++++++++
 include/asm-generic/hugetlb.h   |  7 ++-----
 mm/hugetlb.c                    |  8 +++++++-
 4 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
index cd546a245c68..aa8489c07f24 100644
--- a/arch/s390/include/asm/hugetlb.h
+++ b/arch/s390/include/asm/hugetlb.h
@@ -38,14 +38,8 @@ static inline int prepare_hugepage_range(struct file *file,
 
 #define arch_clear_hugepage_flags(page)		do { } while (0)
 
-static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
-				  pte_t *ptep)
-{
-	if ((pte_val(*ptep) & _REGION_ENTRY_TYPE_MASK) == _REGION_ENTRY_TYPE_R3)
-		pte_val(*ptep) = _REGION3_ENTRY_EMPTY;
-	else
-		pte_val(*ptep) = _SEGMENT_ENTRY_EMPTY;
-}
+void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+		    pte_t *ptep, unsigned long sz);
 
 static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
 					 unsigned long address, pte_t *ptep)
diff --git a/arch/s390/mm/hugetlbpage.c b/arch/s390/mm/hugetlbpage.c
index ae23afc18493..48e19b324017 100644
--- a/arch/s390/mm/hugetlbpage.c
+++ b/arch/s390/mm/hugetlbpage.c
@@ -144,6 +144,15 @@ pte_t huge_ptep_get(pte_t *ptep)
 	return __rste_to_pte(pte_val(*ptep));
 }
 
+void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+		    pte_t *ptep, unsigned long sz)
+{
+	if ((pte_val(*ptep) & _REGION_ENTRY_TYPE_MASK) == _REGION_ENTRY_TYPE_R3)
+		pte_val(*ptep) = _REGION3_ENTRY_EMPTY;
+	else
+		pte_val(*ptep) = _SEGMENT_ENTRY_EMPTY;
+}
+
 pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 			      unsigned long addr, pte_t *ptep)
 {
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index 99b490b4d05a..3138e126f43b 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -31,10 +31,7 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
 	return pte_modify(pte, newprot);
 }
 
-static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
-				  pte_t *ptep)
-{
-	pte_clear(mm, addr, ptep);
-}
+void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+		    pte_t *ptep, unsigned long sz);
 
 #endif /* _ASM_GENERIC_HUGETLB_H */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 0e4d1fb3122f..2b0f6f96f2c1 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3289,6 +3289,12 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 	return ret;
 }
 
+void __weak huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+			   pte_t *ptep, unsigned long sz)
+{
+	pte_clear(mm, addr, ptep);
+}
+
 void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 			    unsigned long start, unsigned long end,
 			    struct page *ref_page)
@@ -3338,7 +3344,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		 * unmapped and its refcount is dropped, so just clear pte here.
 		 */
 		if (unlikely(!pte_present(pte))) {
-			huge_pte_clear(mm, address, ptep);
+			huge_pte_clear(mm, address, ptep, sz);
 			spin_unlock(ptl);
 			continue;
 		}
-- 
2.11.0

[toc] | [next] | [standalone]

#1646926

On Mon, May 22, 2017 at 3:36 PM, Punit Agrawal <punit.agrawal@arm.com> wrote:
> diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
> index 99b490b4d05a..3138e126f43b 100644
> --- a/include/asm-generic/hugetlb.h
> +++ b/include/asm-generic/hugetlb.h
> @@ -31,10 +31,7 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
>         return pte_modify(pte, newprot);
>  }
>
> -static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
> -                                 pte_t *ptep)
> -{
> -       pte_clear(mm, addr, ptep);
> -}
> +void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
> +                   pte_t *ptep, unsigned long sz);
>
>  #endif /* _ASM_GENERIC_HUGETLB_H */
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 0e4d1fb3122f..2b0f6f96f2c1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3289,6 +3289,12 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>         return ret;
>  }
>
> +void __weak huge_pte_clear(struct mm_struct *mm, unsigned long addr,
> +                          pte_t *ptep, unsigned long sz)
> +{
> +       pte_clear(mm, addr, ptep);
> +}
> +
>  void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>                             unsigned long start, unsigned long end,
>                             struct page *ref_page)

I don't really like how this moves the inline version from asm-generic into
a __weak function here. I think it would be better to either stop
using asm-generic/hugetlb.h
on s390, or enclose the generic definition in

#ifndef huge_pte_clear

and then override by defining a macro in s390 as we do in other files
in asm-generic.

       Arnd

[toc] | [prev] | [next] | [standalone]

#1647046

Arnd Bergmann <arnd@arndb.de> writes:

> On Mon, May 22, 2017 at 3:36 PM, Punit Agrawal <punit.agrawal@arm.com> wrote:
>> diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
>> index 99b490b4d05a..3138e126f43b 100644
>> --- a/include/asm-generic/hugetlb.h
>> +++ b/include/asm-generic/hugetlb.h
>> @@ -31,10 +31,7 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
>>         return pte_modify(pte, newprot);
>>  }
>>
>> -static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>> -                                 pte_t *ptep)
>> -{
>> -       pte_clear(mm, addr, ptep);
>> -}
>> +void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>> +                   pte_t *ptep, unsigned long sz);
>>
>>  #endif /* _ASM_GENERIC_HUGETLB_H */
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 0e4d1fb3122f..2b0f6f96f2c1 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -3289,6 +3289,12 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>>         return ret;
>>  }
>>
>> +void __weak huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>> +                          pte_t *ptep, unsigned long sz)
>> +{
>> +       pte_clear(mm, addr, ptep);
>> +}
>> +
>>  void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>                             unsigned long start, unsigned long end,
>>                             struct page *ref_page)
>
> I don't really like how this moves the inline version from asm-generic into
> a __weak function here. I think it would be better to either stop
> using asm-generic/hugetlb.h
> on s390, or enclose the generic definition in
>
> #ifndef huge_pte_clear
>
> and then override by defining a macro in s390 as we do in other files
> in asm-generic.

Nice! I wasn't aware asm-generic follows this as a standard pattern.

s390 doesn't use asm-generic, but I needed to update the prototype with
an additional parameter (size) and needlessly moved the function. I'll
update the patch.

The changes is needed to enable contiguous pte hugepage support on arm64
[0].

Thanks for taking a look.

Punit

[0] https://www.spinics.net/lists/arm-kernel/msg582758.html

[toc] | [prev] | [next] | [standalone]

#1647118 — [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

When unmapping a hugepage range, huge_pte_clear() is used to clear the
page table entries that are marked as not present. huge_pte_clear()
internally just ends up calling pte_clear() which does not correctly
deal with hugepages consisting of contiguous page table entries.

Add a size argument to address this issue and allow architectures to
override huge_pte_clear() by wrapping it in a #ifndef block.

Update s390 implementation with the size parameter as well.

Note that the change only affects huge_pte_clear() - the other generic
hugetlb functions don't need any change.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
---

Changes since v3

* Drop weak function and use #ifndef block to allow architecture override
* Drop unnecessary move of s390 function definition

 arch/s390/include/asm/hugetlb.h | 2 +-
 include/asm-generic/hugetlb.h   | 4 +++-
 mm/hugetlb.c                    | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
index cd546a245c68..c0443500baec 100644
--- a/arch/s390/include/asm/hugetlb.h
+++ b/arch/s390/include/asm/hugetlb.h
@@ -39,7 +39,7 @@ static inline int prepare_hugepage_range(struct file *file,
 #define arch_clear_hugepage_flags(page)		do { } while (0)
 
 static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
-				  pte_t *ptep)
+				  pte_t *ptep, unsigned long sz)
 {
 	if ((pte_val(*ptep) & _REGION_ENTRY_TYPE_MASK) == _REGION_ENTRY_TYPE_R3)
 		pte_val(*ptep) = _REGION3_ENTRY_EMPTY;
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index 99b490b4d05a..540354f94f83 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -31,10 +31,12 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
 	return pte_modify(pte, newprot);
 }
 
+#ifndef huge_pte_clear
 static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
-				  pte_t *ptep)
+		    pte_t *ptep, unsigned long sz)
 {
 	pte_clear(mm, addr, ptep);
 }
+#endif
 
 #endif /* _ASM_GENERIC_HUGETLB_H */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 0e4d1fb3122f..ddfed20cd637 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3338,7 +3338,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		 * unmapped and its refcount is dropped, so just clear pte here.
 		 */
 		if (unlikely(!pte_present(pte))) {
-			huge_pte_clear(mm, address, ptep);
+			huge_pte_clear(mm, address, ptep, sz);
 			spin_unlock(ptl);
 			continue;
 		}
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1647351 — Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

On Mon, May 22, 2017 at 6:25 PM, Punit Agrawal <punit.agrawal@arm.com> wrote:
> When unmapping a hugepage range, huge_pte_clear() is used to clear the
> page table entries that are marked as not present. huge_pte_clear()
> internally just ends up calling pte_clear() which does not correctly
> deal with hugepages consisting of contiguous page table entries.
>
> Add a size argument to address this issue and allow architectures to
> override huge_pte_clear() by wrapping it in a #ifndef block.
>
> Update s390 implementation with the size parameter as well.
>
> Note that the change only affects huge_pte_clear() - the other generic
> hugetlb functions don't need any change.
>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[toc] | [prev] | [next] | [standalone]

#1648159 — Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

Arnd Bergmann <arnd@arndb.de> writes:

> On Mon, May 22, 2017 at 6:25 PM, Punit Agrawal <punit.agrawal@arm.com> wrote:
>> When unmapping a hugepage range, huge_pte_clear() is used to clear the
>> page table entries that are marked as not present. huge_pte_clear()
>> internally just ends up calling pte_clear() which does not correctly
>> deal with hugepages consisting of contiguous page table entries.
>>
>> Add a size argument to address this issue and allow architectures to
>> override huge_pte_clear() by wrapping it in a #ifndef block.
>>
>> Update s390 implementation with the size parameter as well.
>>
>> Note that the change only affects huge_pte_clear() - the other generic
>> hugetlb functions don't need any change.
>>
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks, Arnd. I've applied the tag locally.

>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[toc] | [prev] | [next] | [standalone]

#1647641 — Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

On Mon, 22 May 2017 17:25:55 +0100
Punit Agrawal <punit.agrawal@arm.com> wrote:

> When unmapping a hugepage range, huge_pte_clear() is used to clear the
> page table entries that are marked as not present. huge_pte_clear()
> internally just ends up calling pte_clear() which does not correctly
> deal with hugepages consisting of contiguous page table entries.
> 
> Add a size argument to address this issue and allow architectures to
> override huge_pte_clear() by wrapping it in a #ifndef block.
> 
> Update s390 implementation with the size parameter as well.
> 
> Note that the change only affects huge_pte_clear() - the other generic
> hugetlb functions don't need any change.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> ---
> 
> Changes since v3
> 
> * Drop weak function and use #ifndef block to allow architecture override
> * Drop unnecessary move of s390 function definition
> 
>  arch/s390/include/asm/hugetlb.h | 2 +-
>  include/asm-generic/hugetlb.h   | 4 +++-
>  mm/hugetlb.c                    | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
> index cd546a245c68..c0443500baec 100644
> --- a/arch/s390/include/asm/hugetlb.h
> +++ b/arch/s390/include/asm/hugetlb.h
> @@ -39,7 +39,7 @@ static inline int prepare_hugepage_range(struct file *file,
>  #define arch_clear_hugepage_flags(page)		do { } while (0)
> 
>  static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
> -				  pte_t *ptep)
> +				  pte_t *ptep, unsigned long sz)
>  {
>  	if ((pte_val(*ptep) & _REGION_ENTRY_TYPE_MASK) == _REGION_ENTRY_TYPE_R3)
>  		pte_val(*ptep) = _REGION3_ENTRY_EMPTY;

For the nop-change for s390:
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1648162 — Re: [PATCH v3.1 4/6] mm/hugetlb: Allow architectures to override huge_pte_clear()

Martin Schwidefsky <schwidefsky@de.ibm.com> writes:

> On Mon, 22 May 2017 17:25:55 +0100
> Punit Agrawal <punit.agrawal@arm.com> wrote:
>
>> When unmapping a hugepage range, huge_pte_clear() is used to clear the
>> page table entries that are marked as not present. huge_pte_clear()
>> internally just ends up calling pte_clear() which does not correctly
>> deal with hugepages consisting of contiguous page table entries.
>> 
>> Add a size argument to address this issue and allow architectures to
>> override huge_pte_clear() by wrapping it in a #ifndef block.
>> 
>> Update s390 implementation with the size parameter as well.
>> 
>> Note that the change only affects huge_pte_clear() - the other generic
>> hugetlb functions don't need any change.
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>> 
>> Changes since v3
>> 
>> * Drop weak function and use #ifndef block to allow architecture override
>> * Drop unnecessary move of s390 function definition
>> 
>>  arch/s390/include/asm/hugetlb.h | 2 +-
>>  include/asm-generic/hugetlb.h   | 4 +++-
>>  mm/hugetlb.c                    | 2 +-
>>  3 files changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
>> index cd546a245c68..c0443500baec 100644
>> --- a/arch/s390/include/asm/hugetlb.h
>> +++ b/arch/s390/include/asm/hugetlb.h
>> @@ -39,7 +39,7 @@ static inline int prepare_hugepage_range(struct file *file,
>>  #define arch_clear_hugepage_flags(page)		do { } while (0)
>> 
>>  static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>> -				  pte_t *ptep)
>> +				  pte_t *ptep, unsigned long sz)
>>  {
>>  	if ((pte_val(*ptep) & _REGION_ENTRY_TYPE_MASK) == _REGION_ENTRY_TYPE_R3)
>>  		pte_val(*ptep) = _REGION3_ENTRY_EMPTY;
>
> For the nop-change for s390:
> Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

Applied the tag locally. Thanks! 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web