diff mbox series

[v3,12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

Message ID 20200827080438.315345-13-aneesh.kumar@linux.ibm.com
State New
Headers show
Series mm/debug_vm_pgtable fixes | expand

Commit Message

Aneesh Kumar K.V Aug. 27, 2020, 8:04 a.m. UTC
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 <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Anshuman Khandual Sept. 1, 2020, 4:03 a.m. UTC | #1
On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> 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.

> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index a188b6e4e37e..21329c7d672f 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>  }
>  
> +#ifndef CONFIG_PPC_BOOK3S_64
>  static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>  					  struct vm_area_struct *vma,
>  					  pte_t *ptep, unsigned long pfn,
> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>  	pte = huge_ptep_get(ptep);
>  	WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>  }
> +#endif

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.

>  #else  /* !CONFIG_HUGETLB_PAGE */
>  static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>  static void __init hugetlb_advanced_tests(struct mm_struct *mm,
> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>  	pud_populate_tests(mm, pudp, saved_pmdp);
>  	spin_unlock(ptl);
>  
> +#ifndef CONFIG_PPC_BOOK3S_64
>  	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> +#endif

#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.
Aneesh Kumar K.V Sept. 1, 2020, 6:30 a.m. UTC | #2
On 9/1/20 9:33 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> 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.
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index a188b6e4e37e..21329c7d672f 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>   #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>   }
>>   
>> +#ifndef CONFIG_PPC_BOOK3S_64
>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>   					  struct vm_area_struct *vma,
>>   					  pte_t *ptep, unsigned long pfn,
>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>   	pte = huge_ptep_get(ptep);
>>   	WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>   }
>> +#endif
> 
> 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.
> 
>>   #else  /* !CONFIG_HUGETLB_PAGE */
>>   static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>   	pud_populate_tests(mm, pudp, saved_pmdp);
>>   	spin_unlock(ptl);
>>   
>> +#ifndef CONFIG_PPC_BOOK3S_64
>>   	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> +#endif
>

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.


-aneesh
Christophe Leroy Sept. 1, 2020, 7:59 a.m. UTC | #3
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
Anshuman Khandual Sept. 2, 2020, 1:05 p.m. UTC | #4
On 09/01/2020 12:00 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 9:33 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> 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.
>>
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   mm/debug_vm_pgtable.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index a188b6e4e37e..21329c7d672f 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>>   #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>>   }
>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>                         struct vm_area_struct *vma,
>>>                         pte_t *ptep, unsigned long pfn,
>>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>       pte = huge_ptep_get(ptep);
>>>       WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>>   }
>>> +#endif
>>
>> 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.
>>
>>>   #else  /* !CONFIG_HUGETLB_PAGE */
>>>   static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>>       pud_populate_tests(mm, pudp, saved_pmdp);
>>>       spin_unlock(ptl);
>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>       hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> +#endif
>>
> 
> 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.
> 

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.

Now that this has been proposed [1], along with it any possible urgency factor
out of the way, we could review the proposal with a more long term view making
sure that it improves existing tests and benefits all enabled platforms. Will
look forward to it.

[1] https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200901094423.100149-1-aneesh.kumar@linux.ibm.com/

- Anshuman
Aneesh Kumar K.V Sept. 2, 2020, 1:20 p.m. UTC | #5
Anshuman Khandual <anshuman.khandual@arm.com> writes:

> On 09/01/2020 12:00 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 9:33 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> 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.
>>>
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>   mm/debug_vm_pgtable.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index a188b6e4e37e..21329c7d672f 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>>>   #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>>>   }
>>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>>                         struct vm_area_struct *vma,
>>>>                         pte_t *ptep, unsigned long pfn,
>>>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>>       pte = huge_ptep_get(ptep);
>>>>       WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>>>   }
>>>> +#endif
>>>
>>> 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.
>>>
>>>>   #else  /* !CONFIG_HUGETLB_PAGE */
>>>>   static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>>>       pud_populate_tests(mm, pudp, saved_pmdp);
>>>>       spin_unlock(ptl);
>>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>>       hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>>> +#endif
>>>
>> 
>> 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.
>> 
>
> 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
diff mbox series

Patch

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index a188b6e4e37e..21329c7d672f 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -813,6 +813,7 @@  static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
 #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
 }
 
+#ifndef CONFIG_PPC_BOOK3S_64
 static void __init hugetlb_advanced_tests(struct mm_struct *mm,
 					  struct vm_area_struct *vma,
 					  pte_t *ptep, unsigned long pfn,
@@ -855,6 +856,7 @@  static void __init hugetlb_advanced_tests(struct mm_struct *mm,
 	pte = huge_ptep_get(ptep);
 	WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
 }
+#endif
 #else  /* !CONFIG_HUGETLB_PAGE */
 static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
 static void __init hugetlb_advanced_tests(struct mm_struct *mm,
@@ -1065,7 +1067,9 @@  static int __init debug_vm_pgtable(void)
 	pud_populate_tests(mm, pudp, saved_pmdp);
 	spin_unlock(ptl);
 
+#ifndef CONFIG_PPC_BOOK3S_64
 	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
+#endif
 
 	spin_lock(&mm->page_table_lock);
 	p4d_clear_tests(mm, p4dp);