| Message ID | 20240501144439.1389048-1-ryan.roberts@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid() | expand |
Hello Ryan, On 5/1/24 20:14, Ryan Roberts wrote: > An invalidated pmd should still cause pmd_leaf() to return true. Let's > test for that to ensure all arches remain consistent. This test definitely makes sense. > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > --- > > Hi Andrew, > > This applies on top of v6.9-rc5. It came out of a discussion with Catalin around > the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test > on both arm64 and x86_64. Right, works on arm64. > > Thanks, > Ryan > > mm/debug_vm_pgtable.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 65c19025da3d..57e9cb0820ab 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args) > #ifndef __HAVE_ARCH_PMDP_INVALIDATE > WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd)))); > WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd)))); > + WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd)))); > #endif /* __HAVE_ARCH_PMDP_INVALIDATE */ > } Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst asserting that pmd_mkinvalid() also preserves pmd_leaf() ?
On 02/05/2024 03:43, Anshuman Khandual wrote: > Hello Ryan, > > On 5/1/24 20:14, Ryan Roberts wrote: >> An invalidated pmd should still cause pmd_leaf() to return true. Let's >> test for that to ensure all arches remain consistent. > > This test definitely makes sense. > >> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> --- >> >> Hi Andrew, >> >> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around >> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test >> on both arm64 and x86_64. > > Right, works on arm64. > >> >> Thanks, >> Ryan >> >> mm/debug_vm_pgtable.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 65c19025da3d..57e9cb0820ab 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args) >> #ifndef __HAVE_ARCH_PMDP_INVALIDATE >> WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd)))); >> WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd)))); >> + WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd)))); >> #endif /* __HAVE_ARCH_PMDP_INVALIDATE */ >> } > > Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst > asserting that pmd_mkinvalid() also preserves pmd_leaf() ? Thanks for the review! We don't document that pmd_mkinvalid() preserves pmd_present() and pmd_trans_huge() so I wasn't sure how much detail was appropriate in that document - its pretty light at the moment. If you think this is valuable (and isn't clear enough from the test) then I can add something. But as you say in the other patch, it would then start conflicting with that. I'd prefer to just put this in as-is to avoid the mess.
On 5/2/24 13:00, Ryan Roberts wrote: > On 02/05/2024 03:43, Anshuman Khandual wrote: >> Hello Ryan, >> >> On 5/1/24 20:14, Ryan Roberts wrote: >>> An invalidated pmd should still cause pmd_leaf() to return true. Let's >>> test for that to ensure all arches remain consistent. >> >> This test definitely makes sense. >> >>> >>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>> --- >>> >>> Hi Andrew, >>> >>> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around >>> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test >>> on both arm64 and x86_64. >> >> Right, works on arm64. >> >>> >>> Thanks, >>> Ryan >>> >>> mm/debug_vm_pgtable.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 65c19025da3d..57e9cb0820ab 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args) >>> #ifndef __HAVE_ARCH_PMDP_INVALIDATE >>> WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd)))); >>> WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd)))); >>> + WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd)))); >>> #endif /* __HAVE_ARCH_PMDP_INVALIDATE */ >>> } >> >> Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst >> asserting that pmd_mkinvalid() also preserves pmd_leaf() ? > > Thanks for the review! > > We don't document that pmd_mkinvalid() preserves pmd_present() and > pmd_trans_huge() so I wasn't sure how much detail was appropriate in that > document - its pretty light at the moment. For all other helpers documentation has been light but pxd_mkinvalid() is turning out to be a special case though. > > If you think this is valuable (and isn't clear enough from the test) then I can > add something. But as you say in the other patch, it would then start > conflicting with that. I'd prefer to just put this in as-is to avoid the mess. Sure, fair enough. I will try and update how pmd_mkinvalid() preserves pmd_leaf(), pmd_present(), and pmd_trans_huge() at a later point. Otherwise this patch itself LGTM and runs fine on arm64. Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
On 02/05/2024 09:03, Anshuman Khandual wrote: > > > On 5/2/24 13:00, Ryan Roberts wrote: >> On 02/05/2024 03:43, Anshuman Khandual wrote: >>> Hello Ryan, >>> >>> On 5/1/24 20:14, Ryan Roberts wrote: >>>> An invalidated pmd should still cause pmd_leaf() to return true. Let's >>>> test for that to ensure all arches remain consistent. >>> >>> This test definitely makes sense. >>> >>>> >>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>>> --- >>>> >>>> Hi Andrew, >>>> >>>> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around >>>> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test >>>> on both arm64 and x86_64. >>> >>> Right, works on arm64. >>> >>>> >>>> Thanks, >>>> Ryan >>>> >>>> mm/debug_vm_pgtable.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>> index 65c19025da3d..57e9cb0820ab 100644 >>>> --- a/mm/debug_vm_pgtable.c >>>> +++ b/mm/debug_vm_pgtable.c >>>> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args) >>>> #ifndef __HAVE_ARCH_PMDP_INVALIDATE >>>> WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd)))); >>>> WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd)))); >>>> + WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd)))); >>>> #endif /* __HAVE_ARCH_PMDP_INVALIDATE */ >>>> } >>> >>> Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst >>> asserting that pmd_mkinvalid() also preserves pmd_leaf() ? >> >> Thanks for the review! >> >> We don't document that pmd_mkinvalid() preserves pmd_present() and >> pmd_trans_huge() so I wasn't sure how much detail was appropriate in that >> document - its pretty light at the moment. > > For all other helpers documentation has been light but pxd_mkinvalid() is turning > out to be a special case though. > >> >> If you think this is valuable (and isn't clear enough from the test) then I can >> add something. But as you say in the other patch, it would then start >> conflicting with that. I'd prefer to just put this in as-is to avoid the mess. > > Sure, fair enough. I will try and update how pmd_mkinvalid() preserves pmd_leaf(), > pmd_present(), and pmd_trans_huge() at a later point. Otherwise this patch itself > LGTM and runs fine on arm64. > > Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> Thanks!
On 01.05.24 16:44, Ryan Roberts wrote: > An invalidated pmd should still cause pmd_leaf() to return true. Let's > test for that to ensure all arches remain consistent. > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> Reviewed-by: David Hildenbrand <david@redhat.com>
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 65c19025da3d..57e9cb0820ab 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args) #ifndef __HAVE_ARCH_PMDP_INVALIDATE WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd)))); WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd)))); + WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd)))); #endif /* __HAVE_ARCH_PMDP_INVALIDATE */ }
An invalidated pmd should still cause pmd_leaf() to return true. Let's test for that to ensure all arches remain consistent. Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> --- Hi Andrew, This applies on top of v6.9-rc5. It came out of a discussion with Catalin around the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test on both arm64 and x86_64. Thanks, Ryan mm/debug_vm_pgtable.c | 1 + 1 file changed, 1 insertion(+) -- 2.25.1