Anshuman Khandual <anshuman.khandual@xxxxxxx> 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@xxxxxxxxxxxxx>
>>>> ---
>>>> 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
