Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: > On 9/1/20 8:45 AM, Anshuman Khandual wrote: >> >> >> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>> 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 >>> --- >>> mm/debug_vm_pgtable.c | 13 ++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 086309fb9b6f..bbf9df0e64c6 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -44,10 +44,17 @@ >>> * 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. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
On 9/1/20 1:02 PM, Anshuman Khandual wrote: On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: On 9/1/20 8:45 AM, Anshuman Khandual wrote: On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: 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 --- mm/debug_vm_pgtable.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 086309fb9b6f..bbf9df0e64c6 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -44,10 +44,17 @@ * 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. -aneesh ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it
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
>>> ---
>>> 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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
On 9/1/20 12:20 PM, Christophe Leroy wrote: Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit : On 9/1/20 8:52 AM, Anshuman Khandual wrote: 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote: > On 9/1/20 1:02 PM, Anshuman Khandual wrote: >> >> >> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: >>> On 9/1/20 8:45 AM, Anshuman Khandual wrote: On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: > 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 > --- > mm/debug_vm_pgtable.c | 13 ++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 086309fb9b6f..bbf9df0e64c6 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -44,10 +44,17 @@ > * 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. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
Le 01/09/2020 à 09:40, Aneesh Kumar K.V a écrit : On 9/1/20 12:20 PM, Christophe Leroy wrote: Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit : On 9/1/20 8:52 AM, Anshuman Khandual wrote: 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
On 9/1/20 1:16 PM, Anshuman Khandual wrote: On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote: On 9/1/20 1:02 PM, Anshuman Khandual wrote: On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: On 9/1/20 8:45 AM, Anshuman Khandual wrote: On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: 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 --- mm/debug_vm_pgtable.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 086309fb9b6f..bbf9df0e64c6 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -44,10 +44,17 @@ * 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? -aneesh ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64
Le 01/09/2020 à 08:30, Aneesh Kumar K.V a écrit : I actually wanted to add #ifdef BROKEN. That test is completely broken. Infact I would suggest to remove that test completely. #ifdef will not be required here as there would be a stub definition for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled. spin_lock(&mm->page_table_lock); p4d_clear_tests(mm, p4dp); 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. Note that a bug has been filed at https://bugzilla.kernel.org/show_bug.cgi?id=209029 Christophe ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/13] mm/debug_vm_pgtable fixes
Le 21/08/2020 à 10:51, Anshuman Khandual a écrit : On 08/19/2020 06:30 PM, Aneesh Kumar K.V wrote: 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. Changes proposed here will impact other enabled platforms as well. Adding the following folks and mailing lists, and hoping to get a broader review and test coverage. Please do include them in the next iteration as well. + linux-arm-ker...@lists.infradead.org + linux-s...@vger.kernel.org + linux-snps-arc@lists.infradead.org + x...@kernel.org + linux-a...@vger.kernel.org + Gerald Schaefer + Christophe Leroy Please don't use anymore the above address. Only use the one below. + Christophe Leroy + Vineet Gupta + Mike Rapoport + Qian Cai Thanks Christophe ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/13] mm/debug_vm_pgtable fixes
On 09/01/2020 01:33 PM, Christophe Leroy wrote: > > > Le 21/08/2020 à 10:51, Anshuman Khandual a écrit : >> >> On 08/19/2020 06:30 PM, Aneesh Kumar K.V wrote: >>> 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. >>> > >> >> Changes proposed here will impact other enabled platforms as well. >> Adding the following folks and mailing lists, and hoping to get a >> broader review and test coverage. Please do include them in the >> next iteration as well. >> >> + linux-arm-ker...@lists.infradead.org >> + linux-s...@vger.kernel.org >> + linux-snps-arc@lists.infradead.org >> + x...@kernel.org >> + linux-a...@vger.kernel.org >> >> + Gerald Schaefer >> + Christophe Leroy > > Please don't use anymore the above address. Only use the one below. > >> + Christophe Leroy Sure, noted. >> + Vineet Gupta >> + Mike Rapoport >> + Qian Cai >> > > Thanks > Christophe > > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together
On 09/01/2020 12:08 PM, Aneesh Kumar K.V wrote: > On 9/1/20 9:11 AM, Anshuman Khandual wrote: >> >> >> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>> 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. >> >>> >>> Signed-off-by: Aneesh Kumar K.V >>> --- >>> mm/debug_vm_pgtable.c | 52 --- >>> 1 file changed, 29 insertions(+), 23 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 0ce5c6a24c5b..78c8af3445ac 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -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); >>> /* >>> * Save all the page table page addresses as the page table >>> @@ -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); >>> + /* >>> + * Page table modifying tests >>> + */ >> >> 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. >> >>> + 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); >>> + >>> + ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>> + 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_huge_tests(pmdp, pmd_aligned, prot); >>> + pud_huge_tests(pudp, pud_aligned, prot); >>> + >>> + 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); >>> + >>> + hugetlb_basic_tests(pte_aligned, prot); >> >> hugetlb_basic_tests() is a non page table modifying static test and >> should be classified accordingly. >> >>> + >>> p4d_free(mm, saved_p4dp); >>> pud_free(mm, saved_pudp); >>> pmd_free(mm, saved_pmdp); >>> >> >> Changes till this patch fails to boot on arm64 platform. This should be >> folded with the next patch. >> >> [ 17.080644] [ cut here ] >> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! >> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP >> [ 17.082977] Modules linked in: >> [ 17.083481] CPU: 79 PID: 1 Co
Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together
[ 17.080644] [ cut here ] [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP [ 17.082977] Modules linked in: [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 [ 17.084998] Hardware name: linux,dummy-virt (DT) [ 17.085745] pstate: 6045 (nZCv daif +PAN -UAO BTYPE=--) [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 [ 17.088168] sp : 80001219bcf0 [ 17.088710] x29: 80001219bcf0 x28: 8000114ac000 [ 17.089574] x27: 8000114ac000 x26: 00200fd3 [ 17.090431] x25: 002081400fd3 x24: fe00175c12c0 [ 17.091286] x23: 0005df04d1a8 x22: f18d6e035000 [ 17.092143] x21: 0005dd79 x20: 0005df18e1a8 [ 17.093003] x19: 0005df04cb80 x18: 0014 [ 17.093859] x17: b76667d0 x16: fd4e6611 [ 17.094716] x15: 0001 x14: 0002 [ 17.095572] x13: 0055d966 x12: fe001755e400 [ 17.096429] x11: 0008 x10: 0005fcb6e210 [ 17.097292] x9 : 0005fcb6e210 x8 : 002081590fd3 [ 17.098149] x7 : 0005dd71e0c0 x6 : 0005df18e1a8 [ 17.099006] x5 : 002081590fd3 x4 : 002081590fd3 [ 17.099862] x3 : 0005df18e1a8 x2 : fe00175c12c0 [ 17.100718] x1 : fe00175c1300 x0 : [ 17.101583] Call trace: [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 [ 17.102754] do_one_initcall+0x74/0x1cc [ 17.103381] kernel_init_freeable+0x1d0/0x238 [ 17.104089] kernel_init+0x14/0x118 [ 17.104658] ret_from_fork+0x10/0x34 [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d421) [ 17.106303] ---[ end trace e63471e00f8c147f ]--- 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it
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
---
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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together
On 09/01/2020 03:06 PM, Aneesh Kumar K.V wrote: > [ 17.080644] [ cut here ] [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP [ 17.082977] Modules linked in: [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 [ 17.084998] Hardware name: linux,dummy-virt (DT) [ 17.085745] pstate: 6045 (nZCv daif +PAN -UAO BTYPE=--) [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 [ 17.088168] sp : 80001219bcf0 [ 17.088710] x29: 80001219bcf0 x28: 8000114ac000 [ 17.089574] x27: 8000114ac000 x26: 00200fd3 [ 17.090431] x25: 002081400fd3 x24: fe00175c12c0 [ 17.091286] x23: 0005df04d1a8 x22: f18d6e035000 [ 17.092143] x21: 0005dd79 x20: 0005df18e1a8 [ 17.093003] x19: 0005df04cb80 x18: 0014 [ 17.093859] x17: b76667d0 x16: fd4e6611 [ 17.094716] x15: 0001 x14: 0002 [ 17.095572] x13: 0055d966 x12: fe001755e400 [ 17.096429] x11: 0008 x10: 0005fcb6e210 [ 17.097292] x9 : 0005fcb6e210 x8 : 002081590fd3 [ 17.098149] x7 : 0005dd71e0c0 x6 : 0005df18e1a8 [ 17.099006] x5 : 002081590fd3 x4 : 002081590fd3 [ 17.099862] x3 : 0005df18e1a8 x2 : fe00175c12c0 [ 17.100718] x1 : fe00175c1300 x0 : [ 17.101583] Call trace: [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 [ 17.102754] do_one_initcall+0x74/0x1cc [ 17.103381] kernel_init_freeable+0x1d0/0x238 [ 17.104089] kernel_init+0x14/0x118 [ 17.104658] ret_from_fork+0x10/0x34 [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d421) [ 17.106303] ---[ end trace e63471e00f8c147f ]--- >>> >>> 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(). ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 03/23] arc: use asm-generic/mmu_context.h for no-op implementations
Cc: Vineet Gupta
Cc: linux-snps-arc@lists.infradead.org
Signed-off-by: Nicholas Piggin
---
Please ack or nack if you object to this being mered via
Arnd's tree.
arch/arc/include/asm/mmu_context.h | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/arc/include/asm/mmu_context.h
b/arch/arc/include/asm/mmu_context.h
index 3a5e6a5b9ed6..df164066e172 100644
--- a/arch/arc/include/asm/mmu_context.h
+++ b/arch/arc/include/asm/mmu_context.h
@@ -102,6 +102,7 @@ static inline void get_new_mmu_context(struct mm_struct *mm)
* Initialize the context related info for a new mm_struct
* instance.
*/
+#define init_new_context init_new_context
static inline int
init_new_context(struct task_struct *tsk, struct mm_struct *mm)
{
@@ -113,6 +114,7 @@ init_new_context(struct task_struct *tsk, struct mm_struct
*mm)
return 0;
}
+#define destroy_context destroy_context
static inline void destroy_context(struct mm_struct *mm)
{
unsigned long flags;
@@ -153,13 +155,13 @@ static inline void switch_mm(struct mm_struct *prev,
struct mm_struct *next,
}
/*
- * Called at the time of execve() to get a new ASID
- * Note the subtlety here: get_new_mmu_context() behaves differently here
- * vs. in switch_mm(). Here it always returns a new ASID, because mm has
- * an unallocated "initial" value, while in latter, it moves to a new ASID,
- * only if it was unallocated
+ * activate_mm defaults (in asm-generic) to switch_mm and is called at the
+ * time of execve() to get a new ASID Note the subtlety here:
+ * get_new_mmu_context() behaves differently here vs. in switch_mm(). Here
+ * it always returns a new ASID, because mm has an unallocated "initial"
+ * value, while in latter, it moves to a new ASID, only if it was
+ * unallocated
*/
-#define activate_mm(prev, next)switch_mm(prev, next, NULL)
/* it seemed that deactivate_mm( ) is a reasonable place to do book-keeping
* for retiring-mm. However destroy_context( ) still needs to do that because
@@ -168,8 +170,7 @@ static inline void switch_mm(struct mm_struct *prev, struct
mm_struct *next,
* there is a good chance that task gets sched-out/in, making it's ASID valid
* again (this teased me for a whole day).
*/
-#define deactivate_mm(tsk, mm) do { } while (0)
-#define enter_lazy_tlb(mm, tsk)
+#include
#endif /* __ASM_ARC_MMU_CONTEXT_H */
--
2.23.0
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 03/23] arc: use asm-generic/mmu_context.h for no-op implementations
On 9/1/20 7:15 AM, Nicholas Piggin wrote:
> Cc: Vineet Gupta
> Cc: linux-snps-arc@lists.infradead.org
> Signed-off-by: Nicholas Piggin
Acked-by: Vineet Gupta#arch/arc
Thx,
-Vineet
> ---
>
> Please ack or nack if you object to this being mered via
> Arnd's tree.
>
> arch/arc/include/asm/mmu_context.h | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arc/include/asm/mmu_context.h
> b/arch/arc/include/asm/mmu_context.h
> index 3a5e6a5b9ed6..df164066e172 100644
> --- a/arch/arc/include/asm/mmu_context.h
> +++ b/arch/arc/include/asm/mmu_context.h
> @@ -102,6 +102,7 @@ static inline void get_new_mmu_context(struct mm_struct
> *mm)
> * Initialize the context related info for a new mm_struct
> * instance.
> */
> +#define init_new_context init_new_context
> static inline int
> init_new_context(struct task_struct *tsk, struct mm_struct *mm)
> {
> @@ -113,6 +114,7 @@ init_new_context(struct task_struct *tsk, struct
> mm_struct *mm)
> return 0;
> }
>
> +#define destroy_context destroy_context
> static inline void destroy_context(struct mm_struct *mm)
> {
> unsigned long flags;
> @@ -153,13 +155,13 @@ static inline void switch_mm(struct mm_struct *prev,
> struct mm_struct *next,
> }
>
> /*
> - * Called at the time of execve() to get a new ASID
> - * Note the subtlety here: get_new_mmu_context() behaves differently here
> - * vs. in switch_mm(). Here it always returns a new ASID, because mm has
> - * an unallocated "initial" value, while in latter, it moves to a new ASID,
> - * only if it was unallocated
> + * activate_mm defaults (in asm-generic) to switch_mm and is called at the
> + * time of execve() to get a new ASID Note the subtlety here:
> + * get_new_mmu_context() behaves differently here vs. in switch_mm(). Here
> + * it always returns a new ASID, because mm has an unallocated "initial"
> + * value, while in latter, it moves to a new ASID, only if it was
> + * unallocated
> */
> -#define activate_mm(prev, next) switch_mm(prev, next, NULL)
>
> /* it seemed that deactivate_mm( ) is a reasonable place to do book-keeping
> * for retiring-mm. However destroy_context( ) still needs to do that because
> @@ -168,8 +170,7 @@ static inline void switch_mm(struct mm_struct *prev,
> struct mm_struct *next,
> * there is a good chance that task gets sched-out/in, making it's ASID valid
> * again (this teased me for a whole day).
> */
> -#define deactivate_mm(tsk, mm) do { } while (0)
>
> -#define enter_lazy_tlb(mm, tsk)
> +#include
>
> #endif /* __ASM_ARC_MMU_CONTEXT_H */
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: [plat-hsdk]: Switch ethernet phy-mode to rgmii-id
On 7/7/20 8:38 AM, Evgeniy Didin wrote:
> HSDK board has Micrel KSZ9031, recent commit
> bcf3440c6dd ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
> caused a breakdown of Ethernet.
> Using 'phy-mode = "rgmii"' is not correct because accodring RGMII
> specification it is necessary to have delay on RX (PHY to MAX)
> which is not generated in case of "rgmii".
> Using "rgmii-id" adds necessary delay and solves the issue.
>
> Also adding name of PHY placed on HSDK board.
>
> Signed-off-by: Evgeniy Didin
> Cc: Eugeniy Paltsev
> Cc: Alexey Brodkin
@Alexey - u ok with this change ?
-Vineet
> ---
> arch/arc/boot/dts/hsdk.dts | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arc/boot/dts/hsdk.dts b/arch/arc/boot/dts/hsdk.dts
> index 9acbeba832c0..d6427d47e5a1 100644
> --- a/arch/arc/boot/dts/hsdk.dts
> +++ b/arch/arc/boot/dts/hsdk.dts
> @@ -208,7 +208,7 @@
> reg = <0x8000 0x2000>;
> interrupts = <10>;
> interrupt-names = "macirq";
> - phy-mode = "rgmii";
> + phy-mode = "rgmii-id";
> snps,pbl = <32>;
> snps,multicast-filter-bins = <256>;
> clocks = <&gmacclk>;
> @@ -226,7 +226,7 @@
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "snps,dwmac-mdio";
> - phy0: ethernet-phy@0 {
> + phy0: ethernet-phy@0 { /* Micrel KSZ9031 */
> reg = <0>;
> };
> };
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: [plat-hsdk]: Switch ethernet phy-mode to rgmii-id
Hi Vineet, Evgeniy!
> From: Vineet Gupta
> Sent: Tuesday, September 1, 2020 9:41 PM
> To: Evgeniy Didin ; linux-snps-arc@lists.infradead.org
>
> Cc: Alexey Brodkin ; Eugeniy Paltsev
>
> Subject: Re: [PATCH] ARC: [plat-hsdk]: Switch ethernet phy-mode to rgmii-id
>
> On 7/7/20 8:38 AM, Evgeniy Didin wrote:
> > HSDK board has Micrel KSZ9031, recent commit
> > bcf3440c6dd ("net: phy: micrel: add phy-mode support for the KSZ9031 PHY")
> > caused a breakdown of Ethernet.
> > Using 'phy-mode = "rgmii"' is not correct because accodring RGMII
> > specification it is necessary to have delay on RX (PHY to MAX)
> > which is not generated in case of "rgmii".
> > Using "rgmii-id" adds necessary delay and solves the issue.
> >
> > Also adding name of PHY placed on HSDK board.
> >
> > Signed-off-by: Evgeniy Didin
> > Cc: Eugeniy Paltsev
> > Cc: Alexey Brodkin
>
> @Alexey - u ok with this change ?
Sure,
Acked-by: Alexey Brodkin
>
> -Vineet
>
> > ---
> > arch/arc/boot/dts/hsdk.dts | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arc/boot/dts/hsdk.dts b/arch/arc/boot/dts/hsdk.dts
> > index 9acbeba832c0..d6427d47e5a1 100644
> > --- a/arch/arc/boot/dts/hsdk.dts
> > +++ b/arch/arc/boot/dts/hsdk.dts
> > @@ -208,7 +208,7 @@
> > reg = <0x8000 0x2000>;
> > interrupts = <10>;
> > interrupt-names = "macirq";
> > - phy-mode = "rgmii";
> > + phy-mode = "rgmii-id";
> > snps,pbl = <32>;
> > snps,multicast-filter-bins = <256>;
> > clocks = <&gmacclk>;
> > @@ -226,7 +226,7 @@
> > #address-cells = <1>;
> > #size-cells = <0>;
> > compatible = "snps,dwmac-mdio";
> > - phy0: ethernet-phy@0 {
> > + phy0: ethernet-phy@0 { /* Micrel KSZ9031 */
> > reg = <0>;
> > };
> > };
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
On 09/01/2020 01:25 PM, Aneesh Kumar K.V wrote: > On 9/1/20 1:16 PM, Anshuman Khandual wrote: >> >> >> On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote: >>> On 9/1/20 1:02 PM, Anshuman Khandual wrote: On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: > On 9/1/20 8:45 AM, Anshuman Khandual wrote: >> >> >> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>> 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 >>> --- >>> mm/debug_vm_pgtable.c | 13 ++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 086309fb9b6f..bbf9df0e64c6 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -44,10 +44,17 @@ >>> * 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. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
On 09/01/2020 01:21 PM, Christophe Leroy wrote: > > > Le 01/09/2020 à 09:40, Aneesh Kumar K.V a écrit : >> On 9/1/20 12:20 PM, Christophe Leroy wrote: >>> >>> >>> Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit : On 9/1/20 8:52 AM, Anshuman Khandual wrote: > > > > 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. Agreed. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it
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
> ---
> 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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it
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
---
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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
