| Message ID | 20200827080438.315345-14-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/debug_vm_pgtable fixes | expand |
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 <lkp@intel.com>
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):
>> 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
On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: > 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 <aneesh.kumar@linux.ibm.com> > --- > mm/debug_vm_pgtable.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 21329c7d672f..8527ebb75f2c 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, > static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, > unsigned long vaddr) > { > - pte_t pte = ptep_get(ptep); > + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); 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. > > pr_debug("Validating PTE clear\n"); > pte = __pte(pte_val(pte) | RANDOM_ORVALUE); > @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) > p4d_t *p4dp, *saved_p4dp; > pud_t *pudp, *saved_pudp; > pmd_t *pmdp, *saved_pmdp, pmd; > - pte_t *ptep; > + pte_t *ptep, pte; > pgtable_t saved_ptep; > pgprot_t prot, protnone; > phys_addr_t paddr; > @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) > */ > > ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); > + pte = pfn_pte(pte_aligned, prot); > + set_pte_at(mm, vaddr, ptep, pte); 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 > pte_clear_tests(mm, ptep, vaddr); > pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); > 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: 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.
On 9/1/20 8:55 AM, Anshuman Khandual wrote: > > > On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >> 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 <aneesh.kumar@linux.ibm.com> >> --- >> mm/debug_vm_pgtable.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 21329c7d672f..8527ebb75f2c 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, >> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, >> unsigned long vaddr) >> { >> - pte_t pte = ptep_get(ptep); >> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); > > 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. >> >> pr_debug("Validating PTE clear\n"); >> pte = __pte(pte_val(pte) | RANDOM_ORVALUE); >> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) >> p4d_t *p4dp, *saved_p4dp; >> pud_t *pudp, *saved_pudp; >> pmd_t *pmdp, *saved_pmdp, pmd; >> - pte_t *ptep; >> + pte_t *ptep, pte; >> pgtable_t saved_ptep; >> pgprot_t prot, protnone; >> phys_addr_t paddr; >> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) >> */ >> >> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >> + pte = pfn_pte(pte_aligned, prot); >> + set_pte_at(mm, vaddr, ptep, pte); > > 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 > > >> pte_clear_tests(mm, ptep, vaddr); >> pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >> 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: > 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. >
On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote: > On 9/1/20 8:55 AM, Anshuman Khandual wrote: >> >> >> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>> 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 <aneesh.kumar@linux.ibm.com> >>> --- >>> mm/debug_vm_pgtable.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 21329c7d672f..8527ebb75f2c 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, >>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, >>> unsigned long vaddr) >>> { >>> - pte_t pte = ptep_get(ptep); >>> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); >> >> 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. > > >>> pr_debug("Validating PTE clear\n"); >>> pte = __pte(pte_val(pte) | RANDOM_ORVALUE); >>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) >>> p4d_t *p4dp, *saved_p4dp; >>> pud_t *pudp, *saved_pudp; >>> pmd_t *pmdp, *saved_pmdp, pmd; >>> - pte_t *ptep; >>> + pte_t *ptep, pte; >>> pgtable_t saved_ptep; >>> pgprot_t prot, protnone; >>> phys_addr_t paddr; >>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) >>> */ >>> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>> + pte = pfn_pte(pte_aligned, prot); >>> + set_pte_at(mm, vaddr, ptep, pte); >> >> 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.
On 9/1/20 1:08 PM, Anshuman Khandual wrote: > > > On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote: >> On 9/1/20 8:55 AM, Anshuman Khandual wrote: >>> >>> >>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>> 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 <aneesh.kumar@linux.ibm.com> >>>> --- >>>> mm/debug_vm_pgtable.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>> index 21329c7d672f..8527ebb75f2c 100644 >>>> --- a/mm/debug_vm_pgtable.c >>>> +++ b/mm/debug_vm_pgtable.c >>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, >>>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, >>>> unsigned long vaddr) >>>> { >>>> - pte_t pte = ptep_get(ptep); >>>> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); >>> >>> 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. >> >> >>>> pr_debug("Validating PTE clear\n"); >>>> pte = __pte(pte_val(pte) | RANDOM_ORVALUE); >>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) >>>> p4d_t *p4dp, *saved_p4dp; >>>> pud_t *pudp, *saved_pudp; >>>> pmd_t *pmdp, *saved_pmdp, pmd; >>>> - pte_t *ptep; >>>> + pte_t *ptep, pte; >>>> pgtable_t saved_ptep; >>>> pgprot_t prot, protnone; >>>> phys_addr_t paddr; >>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) >>>> */ >>>> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>>> + pte = pfn_pte(pte_aligned, prot); >>>> + set_pte_at(mm, vaddr, ptep, pte); >>> >>> 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
On 09/01/2020 03:28 PM, Aneesh Kumar K.V wrote: > On 9/1/20 1:08 PM, Anshuman Khandual wrote: >> >> >> On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote: >>> On 9/1/20 8:55 AM, Anshuman Khandual wrote: >>>> >>>> >>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>>> 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 <aneesh.kumar@linux.ibm.com> >>>>> --- >>>>> mm/debug_vm_pgtable.c | 6 ++++-- >>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>>> index 21329c7d672f..8527ebb75f2c 100644 >>>>> --- a/mm/debug_vm_pgtable.c >>>>> +++ b/mm/debug_vm_pgtable.c >>>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, >>>>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, >>>>> unsigned long vaddr) >>>>> { >>>>> - pte_t pte = ptep_get(ptep); >>>>> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); >>>> >>>> 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. >>> >>> >>>>> pr_debug("Validating PTE clear\n"); >>>>> pte = __pte(pte_val(pte) | RANDOM_ORVALUE); >>>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) >>>>> p4d_t *p4dp, *saved_p4dp; >>>>> pud_t *pudp, *saved_pudp; >>>>> pmd_t *pmdp, *saved_pmdp, pmd; >>>>> - pte_t *ptep; >>>>> + pte_t *ptep, pte; >>>>> pgtable_t saved_ptep; >>>>> pgprot_t prot, protnone; >>>>> phys_addr_t paddr; >>>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) >>>>> */ >>>>> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>>>> + pte = pfn_pte(pte_aligned, prot); >>>>> + set_pte_at(mm, vaddr, ptep, pte); >>>> >>>> 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. > > 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); 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.
On 9/2/20 9:19 AM, Anshuman Khandual wrote: > > > On 09/01/2020 03:28 PM, Aneesh Kumar K.V wrote: >> On 9/1/20 1:08 PM, Anshuman Khandual wrote: >>> >>> >>> On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote: >>>> On 9/1/20 8:55 AM, Anshuman Khandual wrote: >>>>> >>>>> >>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>>>> 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 <aneesh.kumar@linux.ibm.com> >>>>>> --- >>>>>> mm/debug_vm_pgtable.c | 6 ++++-- >>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>>>> index 21329c7d672f..8527ebb75f2c 100644 >>>>>> --- a/mm/debug_vm_pgtable.c >>>>>> +++ b/mm/debug_vm_pgtable.c >>>>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, >>>>>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, >>>>>> unsigned long vaddr) >>>>>> { >>>>>> - pte_t pte = ptep_get(ptep); >>>>>> + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); >>>>> >>>>> 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. >>>> >>>> >>>>>> pr_debug("Validating PTE clear\n"); >>>>>> pte = __pte(pte_val(pte) | RANDOM_ORVALUE); >>>>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) >>>>>> p4d_t *p4dp, *saved_p4dp; >>>>>> pud_t *pudp, *saved_pudp; >>>>>> pmd_t *pmdp, *saved_pmdp, pmd; >>>>>> - pte_t *ptep; >>>>>> + pte_t *ptep, pte; >>>>>> pgtable_t saved_ptep; >>>>>> pgprot_t prot, protnone; >>>>>> phys_addr_t paddr; >>>>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) >>>>>> */ >>>>>> ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>>>>> + pte = pfn_pte(pte_aligned, prot); >>>>>> + set_pte_at(mm, vaddr, ptep, pte); >>>>> >>>>> 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. > >> >> 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); > > 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. > But the way test is written we had none pte before. That is why I added that set_pte_at to put something there. With none pte the below sequence fails. pte = __pte(pte_val(pte) | RANDOM_ORVALUE); set_pte_at(mm, vaddr, ptep, pte); because nobody is marking a _PAGE_PTE there. pte_t pte = pfn_pte(pfn, prot); 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)); will that work for you? -aneesh
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 21329c7d672f..8527ebb75f2c 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep, unsigned long vaddr) { - pte_t pte = ptep_get(ptep); + pte_t pte = ptep_get_and_clear(mm, vaddr, ptep); pr_debug("Validating PTE clear\n"); pte = __pte(pte_val(pte) | RANDOM_ORVALUE); @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void) p4d_t *p4dp, *saved_p4dp; pud_t *pudp, *saved_pudp; pmd_t *pmdp, *saved_pmdp, pmd; - pte_t *ptep; + pte_t *ptep, pte; pgtable_t saved_ptep; pgprot_t prot, protnone; phys_addr_t paddr; @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void) */ ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); + pte = pfn_pte(pte_aligned, prot); + set_pte_at(mm, vaddr, ptep, pte); pte_clear_tests(mm, ptep, vaddr); pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); pte_unmap_unlock(ptep, ptl);
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 <aneesh.kumar@linux.ibm.com> --- mm/debug_vm_pgtable.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)