This patch series includes fixes for debug_vm_pgtable test code so that
they follow page table updates rules correctly. The first two patches introduce
changes w.r.t ppc64. The patches are included in this series for completeness. We can
merge them via ppc64 tree if required.
Hugetlb test is disabled on ppc64 because that needs larger change to satisfy
page table update rules.
The patches are on top of 15bc20c6af4ceee97a1f90b43c0e386643c071b4 (linus/master)
Changes from v2:
* Fix build failure with different configs and architecture.
Changes from v1:
* Address review feedback
* drop test specific pfn_pte and pfn_pmd.
* Update ppc64 page table helper to add _PAGE_PTE
Aneesh Kumar K.V (13):
powerpc/mm: Add DEBUG_VM WARN for pmd_clear
powerpc/mm: Move setting pte specific flags to pfn_pte
mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
mm/debug_vm_pgtables/hugevmap: Use the arch helper to identify huge
vmap support.
mm/debug_vm_pgtable/savedwrite: Enable savedwrite test with
CONFIG_NUMA_BALANCING
mm/debug_vm_pgtable/THP: Mark the pte entry huge before using
set_pmd/pud_at
mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an
existing pte entry
mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
mm/debug_vm_pgtable/locks: Move non page table modifying test together
mm/debug_vm_pgtable/locks: Take correct page table lock
mm/debug_vm_pgtable/pmd_clear: Don't use pmd/pud_clear on pte entries
mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64
mm/debug_vm_pgtable: populate a pte entry before fetching it
arch/powerpc/include/asm/book3s/64/pgtable.h | 29 +++-
arch/powerpc/include/asm/nohash/pgtable.h | 5 -
arch/powerpc/mm/pgtable.c | 5 -
mm/debug_vm_pgtable.c | 170 ++++++++++++-------
4 files changed, 131 insertions(+), 78 deletions(-)
--
2.26.2
With the hash page table, the kernel should not use pmd_clear for clearing
huge pte entries. Add a DEBUG_VM WARN to catch the wrong usage.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
With the hash page table, the kernel should not use pmd_clear for clearing
huge pte entries. Add a DEBUG_VM WARN to catch the wrong usage.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
powerpc used to set the pte specific flags in set_pte_at(). This is different
from other architectures. To be consistent with other architecture update
pfn_pte to set _PAGE_PTE on ppc64. Also, drop now unused pte_mkpte.
We add a VM_WARN_ON() to catch the usage of calling set_pte_at() without setting
_PAGE_PTE bit. We will remove that after a few releases.
With respect to huge pmd entries, pmd_mkhuge() takes care of adding the
_PAGE_PTE bit.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 15 +++++++++------
arch/powerpc/include/asm/nohash/pgtable.h | 5 -----
arch/powerpc/mm/pgtable.c | 5 -----
3 files changed, 9 insertions(+), 16 deletions(-)
@@ -184,9 +184,6 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,*/VM_WARN_ON(pte_hw_valid(*ptep)&&!pte_protnone(*ptep));-/* Add the pte bit when trying to set a pte */-pte=pte_mkpte(pte);-/* Note: mm->context.id might not yet have been assigned as*thiscontextmightnothavebeenactivatedyetwhenthis*iscalled.
powerpc used to set the pte specific flags in set_pte_at(). This is different
from other architectures. To be consistent with other architecture update
pfn_pte to set _PAGE_PTE on ppc64. Also, drop now unused pte_mkpte.
We add a VM_WARN_ON() to catch the usage of calling set_pte_at() without setting
_PAGE_PTE bit. We will remove that after a few releases.
With respect to huge pmd entries, pmd_mkhuge() takes care of adding the
_PAGE_PTE bit.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 15 +++++++++------
arch/powerpc/include/asm/nohash/pgtable.h | 5 -----
arch/powerpc/mm/pgtable.c | 5 -----
3 files changed, 9 insertions(+), 16 deletions(-)
@@ -184,9 +184,6 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,*/VM_WARN_ON(pte_hw_valid(*ptep)&&!pte_protnone(*ptep));-/* Add the pte bit when trying to set a pte */-pte=pte_mkpte(pte);-/* Note: mm->context.id might not yet have been assigned as*thiscontextmightnothavebeenactivatedyetwhenthis*iscalled.
There is one checkpatch.pl warning for this patch.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#6:
powerpc used to set the pte specific flags in set_pte_at(). This is different
total: 0 errors, 1 warnings, 61 lines checked
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
Besides, there is also one checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
total: 0 errors, 1 warnings, 20 lines checked
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
that is needed so that it can be built on 32 bit architectures.I did
face build errors with arch-linux
static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
Besides, there is also one checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
total: 0 errors, 1 warnings, 20 lines checked
These warnings are not valid. They are mostly long lines (upto 100) . or
some details mentioned in the () as above.
-aneesh
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
* entry type. But these bits might affect the ability to clear entries with
* pxx_clear() because of how dynamic page table folding works on s390. So
* while loading up the entries do not change the lower 4 bits. It does not
- * have affect any other platform.
+ * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
+ * used to mark a pte entry.
*/
-#define S390_MASK_BITS 4
-#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
+#define S390_SKIP_MASK GENMASK(3, 0)
+#ifdef CONFIG_PPC_BOOK3S_64
+#define PPC64_SKIP_MASK GENMASK(62, 62)
+#else
+#define PPC64_SKIP_MASK 0x0
+#endif
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
Could not (#if __BITS_PER_LONG == 32) be used instead or something like
that. But should be a generic conditional check identifying 32 bit arch
not anything platform specific.
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
Could not (#if __BITS_PER_LONG == 32) be used instead or something like
that. But should be a generic conditional check identifying 32 bit arch
not anything platform specific.
that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure
why other architectures need to bothered about ignoring bit 62.
-aneesh
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
* entry type. But these bits might affect the ability to clear entries with
* pxx_clear() because of how dynamic page table folding works on s390. So
* while loading up the entries do not change the lower 4 bits. It does not
- * have affect any other platform.
+ * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
+ * used to mark a pte entry.
*/
-#define S390_MASK_BITS 4
-#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
+#define S390_SKIP_MASK GENMASK(3, 0)
+#ifdef CONFIG_PPC_BOOK3S_64
+#define PPC64_SKIP_MASK GENMASK(62, 62)
+#else
+#define PPC64_SKIP_MASK 0x0
+#endif
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
Could not (#if __BITS_PER_LONG == 32) be used instead or something like
that. But should be a generic conditional check identifying 32 bit arch
not anything platform specific.
that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62.
Thats okay as long it does not adversely affect other architectures, ignoring
some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
other platforms even if it is a s390 specific constraint. Not having platform
specific #ifdef here, is essential.
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
Could not (#if __BITS_PER_LONG == 32) be used instead or something like
that. But should be a generic conditional check identifying 32 bit arch
not anything platform specific.
that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62.
Thats okay as long it does not adversely affect other architectures, ignoring
some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
other platforms even if it is a s390 specific constraint. Not having platform
specific #ifdef here, is essential.
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
* entry type. But these bits might affect the ability to clear entries with
* pxx_clear() because of how dynamic page table folding works on s390. So
* while loading up the entries do not change the lower 4 bits. It does not
- * have affect any other platform.
+ * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
+ * used to mark a pte entry.
*/
-#define S390_MASK_BITS 4
-#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
+#define S390_SKIP_MASK GENMASK(3, 0)
+#ifdef CONFIG_PPC_BOOK3S_64
+#define PPC64_SKIP_MASK GENMASK(62, 62)
+#else
+#define PPC64_SKIP_MASK 0x0
+#endif
Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.
that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
Could not (#if __BITS_PER_LONG == 32) be used instead or something like
that. But should be a generic conditional check identifying 32 bit arch
not anything platform specific.
that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62.
Thats okay as long it does not adversely affect other architectures, ignoring
some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
other platforms even if it is a s390 specific constraint. Not having platform
specific #ifdef here, is essential.
Why is it essential?
IIRC, I might have already replied on this couple of times. But let me try once more.
It is a generic test aimed at finding inconsistencies between different architectures
in terms of the page table helper semantics. Any platform specific construct here, to
'make things work' has the potential to hide such inconsistencies and defeat the very
purpose. The test/file here follows this rule consistently i.e there is not a single
platform specific #ifdef right now and would really like to continue maintaining this
property, unless until absolutely necessary. Current situation here wrt 32 bit archs
can easily be accommodated with a generic check such as __BITS_PER_LONG.
Some small nits here.
- s/!CONFIG_HAVE_ARCH_HUGE_VMAP/CONFIG_HAVE_ARCH_HUGE_VMAP
- Drop the extra line at the end
- Move the { } down just to be consistent with existing stub for pmd_huge_tests()
quoted hunk
static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
{
@@ -320,11 +326,12 @@ static void __init pud_leaf_tests(unsigned long pfn, pgprot_t prot) WARN_ON(!pud_leaf(pud)); }+#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot) { pud_t pud;- if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))+ if (!arch_ioremap_pud_supported()) return; pr_debug("Validating PUD huge\n");
Some small nits here.
- s/!CONFIG_HAVE_ARCH_HUGE_VMAP/CONFIG_HAVE_ARCH_HUGE_VMAP
- Drop the extra line at the end
- Move the { } down just to be consistent with existing stub for pud_huge_tests()
Saved write support was added to track the write bit of a pte after marking the
pte protnone. This was done so that AUTONUMA can convert a write pte to protnone
and still track the old write bit. When converting it back we set the pte write
bit correctly thereby avoiding a write fault again. Hence enable the test only
when CONFIG_NUMA_BALANCING is enabled and use protnone protflags.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
Saved write support was added to track the write bit of a pte after marking the
pte protnone. This was done so that AUTONUMA can convert a write pte to protnone
and still track the old write bit. When converting it back we set the pte write
bit correctly thereby avoiding a write fault again. Hence enable the test only
when CONFIG_NUMA_BALANCING is enabled and use protnone protflags.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
@@ -1020,8 +1027,8 @@ static int __init debug_vm_pgtable(void)pmd_huge_tests(pmdp,pmd_aligned,prot);pud_huge_tests(pudp,pud_aligned,prot);-pte_savedwrite_tests(pte_aligned,prot);-pmd_savedwrite_tests(pmd_aligned,prot);+pte_savedwrite_tests(pte_aligned,protnone);+pmd_savedwrite_tests(pmd_aligned,protnone);pte_unmap_unlock(ptep,ptl);
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
Saved write support was added to track the write bit of a pte after marking the
total: 0 errors, 1 warnings, 33 lines checked
Otherwise this looks good. With the checkpatch.pl warning fixed
Reviewed-by: Anshuman Khandual <redacted>
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
kernel expects entries to be marked huge before we use set_pmd_at()/set_pud_at().
total: 0 errors, 1 warnings, 77 lines checked
There is no set_pmd_at() in this particular test, why change ?
because if you are building a hugepage you should use pmd_mkhuge(). That
is what is setting _PAGE_PTE with this series. We don't make pfn_pmd set
_PAGE_PTE
-aneesh
set_pte_at() should not be used to set a pte entry at locations that
already holds a valid pte entry. Architectures like ppc64 don't do TLB
invalidate in set_pte_at() and hence expect it to be used to set locations
that are not a valid PTE.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
Should the call sites here be wrapped with __HAVE_ARCH_PGTABLE_DEPOSIT and
__HAVE_ARCH_PGTABLE_WITHDRAW respectively. Though there are generic fallback
definitions, wondering whether they are indeed essential for all platforms.
quoted hunk
}
static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
Architectures like ppc64 use deposited page table while updating the huge pte
total: 0 errors, 1 warnings, 40 lines checked
Should the call sites here be wrapped with __HAVE_ARCH_PGTABLE_DEPOSIT and
__HAVE_ARCH_PGTABLE_WITHDRAW respectively. Though there are generic fallback
definitions, wondering whether they are indeed essential for all platforms.
No, because Any page table helpers operating on pmd level THP can expect
a deposited page table.
__HAVE_ARCH_PGTABLE_DEPOSIT indicates that fallback to generic version.
quoted
}
static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
Architectures like ppc64 use deposited page table while updating the huge pte
total: 0 errors, 1 warnings, 40 lines checked
I will ignore all these, because they are not really important IMHO.
-aneesh
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#7:
Architectures like ppc64 use deposited page table while updating the
huge pte
total: 0 errors, 1 warnings, 40 lines checked
I will ignore all these, because they are not really important IMHO.
When doing a git log in a 80 chars terminal window, having wrapping
lines is not really convenient. It should be easy to avoid it.
Christophe
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#7:
Architectures like ppc64 use deposited page table while updating the
huge pte
total: 0 errors, 1 warnings, 40 lines checked
I will ignore all these, because they are not really important IMHO.
When doing a git log in a 80 chars terminal window, having wrapping
lines is not really convenient. It should be easy to avoid it.
We have been ignoring that for a long time isn't it?
For example ppc64 checkpatch already had
--max-line-length=90
There was also recent discussion whether 80 character limit is valid any
more. But I do keep it restricted to 80 character where ever it is
easy/make sense.
-aneesh
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#7:
Architectures like ppc64 use deposited page table while updating the
huge pte
total: 0 errors, 1 warnings, 40 lines checked
I will ignore all these, because they are not really important IMHO.
When doing a git log in a 80 chars terminal window, having wrapping
lines is not really convenient. It should be easy to avoid it.
We have been ignoring that for a long time isn't it?
For example ppc64 checkpatch already had
--max-line-length=90
There was also recent discussion whether 80 character limit is valid any
more. But I do keep it restricted to 80 character where ever it is
easy/make sense.
Here we are not talking about the code, but the commit log.
As far as I know, the discussions about 80 character lines, 90 lines in
powerpc etc ... is for the code.
We still aim at keeping lines not longer than 75 chars in the commit log.
Christophe
Christophe
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
Architectures like ppc64 use deposited page table while updating the huge pte
total: 0 errors, 1 warnings, 40 lines checked
I will ignore all these, because they are not really important IMHO.
When doing a git log in a 80 chars terminal window, having wrapping lines is not really convenient. It should be easy to avoid it.
We have been ignoring that for a long time isn't it?
For example ppc64 checkpatch already had
--max-line-length=90
There was also recent discussion whether 80 character limit is valid any more. But I do keep it restricted to 80 character where ever it is easy/make sense.
Here we are not talking about the code, but the commit log.
As far as I know, the discussions about 80 character lines, 90 lines in powerpc etc ... is for the code.
We still aim at keeping lines not longer than 75 chars in the commit log.
This will help in adding proper locks in a later patch
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 23 deletions(-)
This will help in adding proper locks in a later patch
It really makes sense to classify these tests here as static and dynamic.
Static are the ones that test via page table entry values modification
without changing anything on the actual page table itself. Where as the
dynamic tests do change the page table. Static tests would probably never
require any lock synchronization but the dynamic ones will do.
@@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)p4dp=p4d_alloc(mm,pgdp,vaddr);pudp=pud_alloc(mm,p4dp,vaddr);pmdp=pmd_alloc(mm,pudp,vaddr);-ptep=pte_alloc_map_lock(mm,pmdp,vaddr,&ptl);+ptep=pte_alloc_map(mm,pmdp,vaddr);/**Saveallthepagetablepageaddressesasthepagetable
@@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)p4d_basic_tests(p4d_aligned,prot);pgd_basic_tests(pgd_aligned,prot);-pte_clear_tests(mm,ptep,vaddr);-pmd_clear_tests(mm,pmdp);-pud_clear_tests(mm,pudp);-p4d_clear_tests(mm,p4dp);-pgd_clear_tests(mm,pgdp);--pte_advanced_tests(mm,vma,ptep,pte_aligned,vaddr,prot);-pmd_advanced_tests(mm,vma,pmdp,pmd_aligned,vaddr,prot,saved_ptep);-pud_advanced_tests(mm,vma,pudp,pud_aligned,vaddr,prot);-hugetlb_advanced_tests(mm,vma,ptep,pte_aligned,vaddr,prot);-pmd_leaf_tests(pmd_aligned,prot);pud_leaf_tests(pud_aligned,prot);-pmd_huge_tests(pmdp,pmd_aligned,prot);-pud_huge_tests(pudp,pud_aligned,prot);-pte_savedwrite_tests(pte_aligned,protnone);pmd_savedwrite_tests(pmd_aligned,protnone);-pte_unmap_unlock(ptep,ptl);--pmd_populate_tests(mm,pmdp,saved_ptep);-pud_populate_tests(mm,pudp,saved_pmdp);-p4d_populate_tests(mm,p4dp,saved_pudp);-pgd_populate_tests(mm,pgdp,saved_p4dp);-pte_special_tests(pte_aligned,prot);pte_protnone_tests(pte_aligned,protnone);pmd_protnone_tests(pmd_aligned,protnone);
@@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)pmd_swap_tests(pmd_aligned,prot);swap_migration_tests();-hugetlb_basic_tests(pte_aligned,prot);pmd_thp_tests(pmd_aligned,prot);pud_thp_tests(pud_aligned,prot);+/*+*Pagetablemodifyingtests+*/
In this comment, please do add some more details about how these tests
need proper locking mechanism unlike the previous static ones. Also add
a similar comment section for the static tests that dont really change
page table and need not require corresponding locking mechanism. Both
comment sections will make this classification clear.
This will help in adding proper locks in a later patch
It really makes sense to classify these tests here as static and dynamic.
Static are the ones that test via page table entry values modification
without changing anything on the actual page table itself. Where as the
dynamic tests do change the page table. Static tests would probably never
require any lock synchronization but the dynamic ones will do.
@@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)p4dp=p4d_alloc(mm,pgdp,vaddr);pudp=pud_alloc(mm,p4dp,vaddr);pmdp=pmd_alloc(mm,pudp,vaddr);-ptep=pte_alloc_map_lock(mm,pmdp,vaddr,&ptl);+ptep=pte_alloc_map(mm,pmdp,vaddr);/**Saveallthepagetablepageaddressesasthepagetable
@@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)p4d_basic_tests(p4d_aligned,prot);pgd_basic_tests(pgd_aligned,prot);-pte_clear_tests(mm,ptep,vaddr);-pmd_clear_tests(mm,pmdp);-pud_clear_tests(mm,pudp);-p4d_clear_tests(mm,p4dp);-pgd_clear_tests(mm,pgdp);--pte_advanced_tests(mm,vma,ptep,pte_aligned,vaddr,prot);-pmd_advanced_tests(mm,vma,pmdp,pmd_aligned,vaddr,prot,saved_ptep);-pud_advanced_tests(mm,vma,pudp,pud_aligned,vaddr,prot);-hugetlb_advanced_tests(mm,vma,ptep,pte_aligned,vaddr,prot);-pmd_leaf_tests(pmd_aligned,prot);pud_leaf_tests(pud_aligned,prot);-pmd_huge_tests(pmdp,pmd_aligned,prot);-pud_huge_tests(pudp,pud_aligned,prot);-pte_savedwrite_tests(pte_aligned,protnone);pmd_savedwrite_tests(pmd_aligned,protnone);-pte_unmap_unlock(ptep,ptl);--pmd_populate_tests(mm,pmdp,saved_ptep);-pud_populate_tests(mm,pudp,saved_pmdp);-p4d_populate_tests(mm,p4dp,saved_pudp);-pgd_populate_tests(mm,pgdp,saved_p4dp);-pte_special_tests(pte_aligned,prot);pte_protnone_tests(pte_aligned,protnone);pmd_protnone_tests(pmd_aligned,protnone);
@@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)pmd_swap_tests(pmd_aligned,prot);swap_migration_tests();-hugetlb_basic_tests(pte_aligned,prot);pmd_thp_tests(pmd_aligned,prot);pud_thp_tests(pud_aligned,prot);+/*+*Pagetablemodifyingtests+*/
In this comment, please do add some more details about how these tests
need proper locking mechanism unlike the previous static ones. Also add
a similar comment section for the static tests that dont really change
page table and need not require corresponding locking mechanism. Both
comment sections will make this classification clear.
This will help in adding proper locks in a later patch
It really makes sense to classify these tests here as static and dynamic.
Static are the ones that test via page table entry values modification
without changing anything on the actual page table itself. Where as the
dynamic tests do change the page table. Static tests would probably never
require any lock synchronization but the dynamic ones will do.
In this comment, please do add some more details about how these tests
need proper locking mechanism unlike the previous static ones. Also add
a similar comment section for the static tests that dont really change
page table and need not require corresponding locking mechanism. Both
comment sections will make this classification clear.
IIUC, this should happen even without the series right? This is
assert_spin_locked(pmd_lockptr(mm, pmdp));
Crash does not happen without this series. A previous patch [PATCH v3 08/13]
added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
pgtable_trans_huge_deposit() which the asserts the lock.
IIUC, this should happen even without the series right? This is
assert_spin_locked(pmd_lockptr(mm, pmdp));
Crash does not happen without this series. A previous patch [PATCH v3 08/13]
added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
pgtable_trans_huge_deposit() which the asserts the lock.
I fixed that by moving the pgtable deposit after adding the pmd locks
correctly.
mm/debug_vm_pgtable/locks: Move non page table modifying test together
mm/debug_vm_pgtable/locks: Take correct page table lock
mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
-aneesh
IIUC, this should happen even without the series right? This is
assert_spin_locked(pmd_lockptr(mm, pmdp));
Crash does not happen without this series. A previous patch [PATCH v3 08/13]
added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
pgtable_trans_huge_deposit() which the asserts the lock.
I fixed that by moving the pgtable deposit after adding the pmd locks correctly.
mm/debug_vm_pgtable/locks: Move non page table modifying test together
mm/debug_vm_pgtable/locks: Take correct page table lock
mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
Right, it does fix. But then both those patches should be folded/merged in
order to preserve git bisect ability, besides test classification reasons
as mentioned in a previous response and a possible redundant movement of
hugetlb_basic_tests().
Overall, grouping together dynamic tests at various page table levels and
taking a corresponding lock, makes sense. Commit message for the resultant
patch should include (a) Test classification (b) Grouping by function for
the static tests, by page table level for the dynamic tests (c) Locking
requirement for the dynamic tests each level etc.
The seems to be missing quite a lot of details w.r.t allocating
the correct pgtable_t page (huge_pte_alloc()), holding the right
lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
Hence disable the test on ppc64.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 4 ++++
1 file changed, 4 insertions(+)
The seems to be missing quite a lot of details w.r.t allocating
the correct pgtable_t page (huge_pte_alloc()), holding the right
lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
Hence disable the test on ppc64.
Would really like this to get resolved in an uniform and better way
instead, i.e a modified hugetlb_advanced_tests() which works on all
platforms including ppc64.
In absence of a modified version, I do realize the situation here,
where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
remove hugetlb_advanced_tests() from other platforms as well.
In the worst case if we could not get a new hugetlb_advanced_tests() test
that works on all platforms, this might be the last fallback option. In
which case, it will require a proper comment section with a "FIXME: ",
explaining the current situation (and that #ifdef is temporary in nature)
and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
The seems to be missing quite a lot of details w.r.t allocating
the correct pgtable_t page (huge_pte_alloc()), holding the right
lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
Hence disable the test on ppc64.
Would really like this to get resolved in an uniform and better way
instead, i.e a modified hugetlb_advanced_tests() which works on all
platforms including ppc64.
In absence of a modified version, I do realize the situation here,
where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
remove hugetlb_advanced_tests() from other platforms as well.
In the worst case if we could not get a new hugetlb_advanced_tests() test
that works on all platforms, this might be the last fallback option. In
which case, it will require a proper comment section with a "FIXME: ",
explaining the current situation (and that #ifdef is temporary in nature)
and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
But again, we should really try and avoid taking this path.
To be frank i am kind of frustrated with how this patch series is being
looked at. We pushed a completely broken test to upstream and right now
we have a code in upstream that crash when booted on ppc64. My attempt
has been to make progress here and you definitely seems to be not in
agreement to that.
At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE
support on ppc64 because AFAIU it doesn't add any value.
-aneesh
But again, we should really try and avoid taking this path.
To be frank i am kind of frustrated with how this patch series is being
looked at. We pushed a completely broken test to upstream and right now
we have a code in upstream that crash when booted on ppc64. My attempt
has been to make progress here and you definitely seems to be not in
agreement to that.
At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE
support on ppc64 because AFAIU it doesn't add any value.
The seems to be missing quite a lot of details w.r.t allocating
the correct pgtable_t page (huge_pte_alloc()), holding the right
lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
Hence disable the test on ppc64.
Would really like this to get resolved in an uniform and better way
instead, i.e a modified hugetlb_advanced_tests() which works on all
platforms including ppc64.
In absence of a modified version, I do realize the situation here,
where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
remove hugetlb_advanced_tests() from other platforms as well.
In the worst case if we could not get a new hugetlb_advanced_tests() test
that works on all platforms, this might be the last fallback option. In
which case, it will require a proper comment section with a "FIXME: ",
explaining the current situation (and that #ifdef is temporary in nature)
and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
But again, we should really try and avoid taking this path.
To be frank i am kind of frustrated with how this patch series is being looked at. We pushed a completely broken test to upstream and right now we have a code in upstream that crash when booted on ppc64. My attempt has been to make progress here and you definitely seems to be not in agreement to that.
I am afraid, this does not accurately represent the situation.
- The second set patch series got merged in it's V5 after accommodating almost
all reviews and objections during previous discussion cycles. For a complete
development log, please refer https://patchwork.kernel.org/cover/11658627/.
- The series has been repeatedly tested on arm64 and x86 platforms for multiple
configurations but build tested on all other enabled platforms. I have always
been dependent on voluntary help from folks on the list to get this tested on
other enabled platforms as I dont have access to such systems. Always assumed
that is the way to go for anything which runs on multiple platforms. So, am I
expected to test on platforms that I dont have access to ? But I am ready to
be corrected here, if the community protocol is not what I have always assumed
it to be.
- Each and every version of the series had appropriately copied all the enabled
platform's mailing list. Also, I had explicitly asked for volunteers to test
this out on platforms apart from x86 and arm64. We had positive response from
all platforms i.e arc, s390, ppc32 but except for ppc64.
https://patchwork.kernel.org/cover/11644771/https://patchwork.kernel.org/cover/11603713/
- The development cycle provided sufficient time window for any detailed review
and test. I have always been willing to address almost all the issues brought
forward during these discussions. From past experience on this test, there is
an inherent need to understand platform specific details while trying to come
up with something generic enough that works on all platforms. It necessitates
participation from relevant folks to enable this test on a given platform. We
were able to enable this on arm64, x86, arc, s390, powerpc following a similar
model.
- I have to disagree here that the concerned test i.e hugetlb_advanced_tests()
is completely broken. As mentioned before, the idea here has always been to
emulate enough MM objects, so that a given page table helper could be tested.
hugetlb_advanced_tests() seems to be insufficient on ppc64 platform causing it
to crash, which is not the case on other platforms. But it is not perfect and
can be improved upon. Given the constraints i.e limited emulation of objects,
the test tries to do the right thing. Calling it broken is not an appropriate
description.
At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE support on ppc64 because AFAIU it doesn't add any value.
The seems to be missing quite a lot of details w.r.t allocating
the correct pgtable_t page (huge_pte_alloc()), holding the right
lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
Hence disable the test on ppc64.
Would really like this to get resolved in an uniform and better way
instead, i.e a modified hugetlb_advanced_tests() which works on all
platforms including ppc64.
In absence of a modified version, I do realize the situation here,
where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
remove hugetlb_advanced_tests() from other platforms as well.
In the worst case if we could not get a new hugetlb_advanced_tests() test
that works on all platforms, this might be the last fallback option. In
which case, it will require a proper comment section with a "FIXME: ",
explaining the current situation (and that #ifdef is temporary in nature)
and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
But again, we should really try and avoid taking this path.
To be frank i am kind of frustrated with how this patch series is being looked at. We pushed a completely broken test to upstream and right now we have a code in upstream that crash when booted on ppc64. My attempt has been to make progress here and you definitely seems to be not in agreement to that.
I am afraid, this does not accurately represent the situation.
- The second set patch series got merged in it's V5 after accommodating almost
all reviews and objections during previous discussion cycles. For a complete
development log, please refer https://patchwork.kernel.org/cover/11658627/.
- The series has been repeatedly tested on arm64 and x86 platforms for multiple
configurations but build tested on all other enabled platforms. I have always
been dependent on voluntary help from folks on the list to get this tested on
other enabled platforms as I dont have access to such systems. Always assumed
that is the way to go for anything which runs on multiple platforms. So, am I
expected to test on platforms that I dont have access to ? But I am ready to
be corrected here, if the community protocol is not what I have always assumed
it to be.
- Each and every version of the series had appropriately copied all the enabled
platform's mailing list. Also, I had explicitly asked for volunteers to test
this out on platforms apart from x86 and arm64. We had positive response from
all platforms i.e arc, s390, ppc32 but except for ppc64.
https://patchwork.kernel.org/cover/11644771/https://patchwork.kernel.org/cover/11603713/
- The development cycle provided sufficient time window for any detailed review
and test. I have always been willing to address almost all the issues brought
forward during these discussions. From past experience on this test, there is
an inherent need to understand platform specific details while trying to come
up with something generic enough that works on all platforms. It necessitates
participation from relevant folks to enable this test on a given platform. We
were able to enable this on arm64, x86, arc, s390, powerpc following a similar
model.
- I have to disagree here that the concerned test i.e hugetlb_advanced_tests()
is completely broken. As mentioned before, the idea here has always been to
emulate enough MM objects, so that a given page table helper could be tested.
hugetlb_advanced_tests() seems to be insufficient on ppc64 platform causing it
to crash, which is not the case on other platforms. But it is not perfect and
can be improved upon. Given the constraints i.e limited emulation of objects,
the test tries to do the right thing. Calling it broken is not an appropriate
description.
None of the fixes done here are specific to ppc64. I am not sure why you
keep suggesting ppc64 specific issues. One should not do page table
updates without holding locks. A hugetlb pte updates expect a vma marked
hugetlb.
As explained in the patch, I see very little value in a bunch of tests
like this and the only reason I started to fix this up is because of
multiple crash reports on ppc64.
Considering the hugetlb tests require much larger change and as it is
currently written is broken, I wanted to remove that test and let you
come up with a proper test. But since you had it "working", I disabled
this only on ppc64.
But you keep suggesting that the hugetlb test need to be fixed as part
of the patch series review. I don't have enough motivation to fix that,
because I don't see much value in a bunch of tests like these. As shown
already these tests already reported success till now without even
following any page table update rules.
-aneesh
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: kernel test robot <hidden> Date: 2020-08-27 12:18:49
Hi "Aneesh,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hnaz-linux-mm/master]
[also build test WARNING on powerpc/next v5.9-rc2 next-20200827]
[cannot apply to mmotm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/mm-debug_vm_pgtable-fixes/20200827-160758
base: https://github.com/hnaz/linux-mm master
config: x86_64-randconfig-s022-20200827 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-191-g10164920-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
sparse warnings: (new ones prefixed by >>)
mm/debug_vm_pgtable.c:509:9: sparse: sparse: incompatible types in conditional expression (different base types):
mm/debug_vm_pgtable.c:509:9: sparse: void
mm/debug_vm_pgtable.c:509:9: sparse: int
mm/debug_vm_pgtable.c:528:9: sparse: sparse: incompatible types in conditional expression (different base types):
mm/debug_vm_pgtable.c:528:9: sparse: void
mm/debug_vm_pgtable.c:528:9: sparse: int
mm/debug_vm_pgtable.c: note: in included file (through include/linux/pgtable.h, include/linux/mm.h, include/linux/highmem.h):
quoted
arch/x86/include/asm/pgtable.h:587:27: sparse: sparse: context imbalance in 'debug_vm_pgtable' - unexpected unlock
# https://github.com/0day-ci/linux/commit/9370726f47eaffdf772fdc273d180ec03b245cca
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Aneesh-Kumar-K-V/mm-debug_vm_pgtable-fixes/20200827-160758
git checkout 9370726f47eaffdf772fdc273d180ec03b245cca
vim +/debug_vm_pgtable +587 arch/x86/include/asm/pgtable.h
b534816b552d35 Jeremy Fitzhardinge 2009-02-04 586
fb43d6cb91ef57 Dave Hansen 2018-04-06 @587 static inline pgprotval_t check_pgprot(pgprot_t pgprot)
fb43d6cb91ef57 Dave Hansen 2018-04-06 588 {
fb43d6cb91ef57 Dave Hansen 2018-04-06 589 pgprotval_t massaged_val = massage_pgprot(pgprot);
fb43d6cb91ef57 Dave Hansen 2018-04-06 590
fb43d6cb91ef57 Dave Hansen 2018-04-06 591 /* mmdebug.h can not be included here because of dependencies */
fb43d6cb91ef57 Dave Hansen 2018-04-06 592 #ifdef CONFIG_DEBUG_VM
fb43d6cb91ef57 Dave Hansen 2018-04-06 593 WARN_ONCE(pgprot_val(pgprot) != massaged_val,
fb43d6cb91ef57 Dave Hansen 2018-04-06 594 "attempted to set unsupported pgprot: %016llx "
fb43d6cb91ef57 Dave Hansen 2018-04-06 595 "bits: %016llx supported: %016llx\n",
fb43d6cb91ef57 Dave Hansen 2018-04-06 596 (u64)pgprot_val(pgprot),
fb43d6cb91ef57 Dave Hansen 2018-04-06 597 (u64)pgprot_val(pgprot) ^ massaged_val,
fb43d6cb91ef57 Dave Hansen 2018-04-06 598 (u64)__supported_pte_mask);
fb43d6cb91ef57 Dave Hansen 2018-04-06 599 #endif
fb43d6cb91ef57 Dave Hansen 2018-04-06 600
fb43d6cb91ef57 Dave Hansen 2018-04-06 601 return massaged_val;
fb43d6cb91ef57 Dave Hansen 2018-04-06 602 }
fb43d6cb91ef57 Dave Hansen 2018-04-06 603
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Seems like ptep_get_and_clear() here just clears the entry in preparation
for a following set_pte_at() which otherwise would have been a problem on
ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
existing valid entry. So the commit message here is bit misleading.
Not here, creating and populating an entry must be done in respective
test functions itself. Besides, this seems bit redundant as well. The
test pte_clear_tests() with the above change added, already
- Clears the PTEP entry with ptep_get_and_clear()
- Creates and populates the PTEP with set_pte_at()
- Clears with pte_clear()
- Checks for pte_none()
If the objective is to clear the PTEP entry before calling set_pte_at(),
then only the first chunk is required and it should also be merged with
a previous patch.
[PATCH v3 07/13] mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an existing pte entry
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
total: 0 errors, 1 warnings, 24 lines checked
There is also a build failure on x86 reported from kernel test robot.
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Seems like ptep_get_and_clear() here just clears the entry in preparation
for a following set_pte_at() which otherwise would have been a problem on
ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
existing valid entry. So the commit message here is bit misleading.
and also fetch the pte value which is used further.
Not here, creating and populating an entry must be done in respective
test functions itself. Besides, this seems bit redundant as well. The
test pte_clear_tests() with the above change added, already
- Clears the PTEP entry with ptep_get_and_clear()
and fetch the old value set previously.
- Creates and populates the PTEP with set_pte_at()
- Clears with pte_clear()
- Checks for pte_none()
If the objective is to clear the PTEP entry before calling set_pte_at(),
then only the first chunk is required and it should also be merged with
a previous patch.
[PATCH v3 07/13] mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an existing pte entry
There is a checkpatch.pl warning here.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7:
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
total: 0 errors, 1 warnings, 24 lines checked
There is also a build failure on x86 reported from kernel test robot.
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Seems like ptep_get_and_clear() here just clears the entry in preparation
for a following set_pte_at() which otherwise would have been a problem on
ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
existing valid entry. So the commit message here is bit misleading.
and also fetch the pte value which is used further.
Not here, creating and populating an entry must be done in respective
test functions itself. Besides, this seems bit redundant as well. The
test pte_clear_tests() with the above change added, already
- Clears the PTEP entry with ptep_get_and_clear()
and fetch the old value set previously.
In that case, please move above two lines i.e
pte = pfn_pte(pte_aligned, prot);
set_pte_at(mm, vaddr, ptep, pte);
from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
as required.
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Seems like ptep_get_and_clear() here just clears the entry in preparation
for a following set_pte_at() which otherwise would have been a problem on
ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
existing valid entry. So the commit message here is bit misleading.
and also fetch the pte value which is used further.
Not here, creating and populating an entry must be done in respective
test functions itself. Besides, this seems bit redundant as well. The
test pte_clear_tests() with the above change added, already
- Clears the PTEP entry with ptep_get_and_clear()
and fetch the old value set previously.
In that case, please move above two lines i.e
pte = pfn_pte(pte_aligned, prot);
set_pte_at(mm, vaddr, ptep, pte);
from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
as required.
Frankly, I don't understand what these tests are testing. It all looks
like some random clear and set.
static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
unsigned long vaddr, unsigned long pfn,
pgprot_t prot)
{
pte_t pte = pfn_pte(pfn, prot);
set_pte_at(mm, vaddr, ptep, pte);
pte = ptep_get_and_clear(mm, vaddr, ptep);
pr_debug("Validating PTE clear\n");
pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
set_pte_at(mm, vaddr, ptep, pte);
barrier();
pte_clear(mm, vaddr, ptep);
pte = ptep_get(ptep);
WARN_ON(!pte_none(pte));
}
-aneesh
pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/debug_vm_pgtable.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Seems like ptep_get_and_clear() here just clears the entry in preparation
for a following set_pte_at() which otherwise would have been a problem on
ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
existing valid entry. So the commit message here is bit misleading.
and also fetch the pte value which is used further.
Not here, creating and populating an entry must be done in respective
test functions itself. Besides, this seems bit redundant as well. The
test pte_clear_tests() with the above change added, already
- Clears the PTEP entry with ptep_get_and_clear()
and fetch the old value set previously.
In that case, please move above two lines i.e
pte = pfn_pte(pte_aligned, prot);
set_pte_at(mm, vaddr, ptep, pte);
from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
as required.
Frankly, I don't understand what these tests are testing. It all looks like some random clear and set.
The idea here is to have some value with some randomness preferably, in
a given PTEP before attempting to clear the entry, in order to make sure
that pte_clear() is indeed clearing something of non-zero value.
Looking at this again, this preceding pfn_pte() followed by set_pte_at()
is not really required. Its reasonable to start with what ever was there
in the PTEP as a seed value which anyway gets added with RANDOM_ORVALUE.
s/ptep_get/ptep_get_and_clear is sufficient to take care of the powerpc
set_pte_at() constraint.