Re: [PATCH] mm/debug_vm_pgtable: Fix BUG_ON with pud advanced test

["Aneesh Kumar K.V" <aneesh.kumar@kernel.org>]

On 1/29/24 12:23 PM, Anshuman Khandual wrote:
> 
> 
> On 1/29/24 11:56, Aneesh Kumar K.V wrote:
>> On 1/29/24 11:52 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 1/29/24 11:30, Aneesh Kumar K.V (IBM) wrote:
>>>> Architectures like powerpc add debug checks to ensure we find only devmap
>>>> PUD pte entries. These debug checks are only done with CONFIG_DEBUG_VM.
>>>> This patch marks the ptes used for PUD advanced test devmap pte entries
>>>> so that we don't hit on debug checks on architecture like ppc64 as
>>>> below.
>>>>
>>>> WARNING: CPU: 2 PID: 1 at arch/powerpc/mm/book3s64/radix_pgtable.c:1382 radix__pud_hugepage_update+0x38/0x138
>>>> ....
>>>> NIP [c0000000000a7004] radix__pud_hugepage_update+0x38/0x138
>>>> LR [c0000000000a77a8] radix__pudp_huge_get_and_clear+0x28/0x60
>>>> Call Trace:
>>>> [c000000004a2f950] [c000000004a2f9a0] 0xc000000004a2f9a0 (unreliable)
>>>> [c000000004a2f980] [000d34c100000000] 0xd34c100000000
>>>> [c000000004a2f9a0] [c00000000206ba98] pud_advanced_tests+0x118/0x334
>>>> [c000000004a2fa40] [c00000000206db34] debug_vm_pgtable+0xcbc/0x1c48
>>>> [c000000004a2fc10] [c00000000000fd28] do_one_initcall+0x60/0x388
>>>>
>>>> Also
>>>>
>>>>  kernel BUG at arch/powerpc/mm/book3s64/pgtable.c:202!
>>>>  ....
>>>>
>>>>  NIP [c000000000096510] pudp_huge_get_and_clear_full+0x98/0x174
>>>>  LR [c00000000206bb34] pud_advanced_tests+0x1b4/0x334
>>>>  Call Trace:
>>>>  [c000000004a2f950] [000d34c100000000] 0xd34c100000000 (unreliable)
>>>>  [c000000004a2f9a0] [c00000000206bb34] pud_advanced_tests+0x1b4/0x334
>>>>  [c000000004a2fa40] [c00000000206db34] debug_vm_pgtable+0xcbc/0x1c48
>>>>  [c000000004a2fc10] [c00000000000fd28] do_one_initcall+0x60/0x388
>>>>
>>>> Fixes: 27af67f35631 ("powerpc/book3s64/mm: enable transparent pud hugepage")
>>>> Signed-off-by: Aneesh Kumar K.V (IBM) <aneesh.kumar@kernel.org>
>>>> ---
>>>>  mm/debug_vm_pgtable.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 5662e29fe253..65c19025da3d 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -362,6 +362,12 @@ static void __init pud_advanced_tests(struct pgtable_debug_args *args)
>>>>  	vaddr &= HPAGE_PUD_MASK;
>>>>  
>>>>  	pud = pfn_pud(args->pud_pfn, args->page_prot);
>>>> +	/*
>>>> +	 * Some architectures have debug checks to make sure
>>>> +	 * huge pud mapping are only found with devmap entries
>>>> +	 * For now test with only devmap entries.
>>>> +	 */
>>> Do you see this behaviour to be changed in powerpc anytime soon ? Otherwise
>>> these pud_mkdevmap() based work arounds, might be required to stick around
>>> for longer just to prevent powerpc specific triggers. Given PUD transparent
>>> huge pages i.e HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD are just supported on x86
>>> and powerpc platforms, could not this problem be solved in a more uniform
>>> manner.
>>>
>>
>>
>> IIUC pud level transparent hugepages are only supported with devmap entries even
>> on x86. We don't do anonymous pud hugepage.
> 
> There are some 'pud_trans_huge(orig_pud) || pud_devmap(orig_pud)' checks in
> core paths i.e in mm/memory.c which might suggest pud_trans_huge() to exist
> without also being a devmap. I might be missing something here, but on x86
> platform following helpers suggest pud_trans_huge() to exist without being
> a devmap as well.
> 
> #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> static inline int pud_trans_huge(pud_t pud)
> {
>         return (pud_val(pud) & (_PAGE_PSE|_PAGE_DEVMAP)) == _PAGE_PSE;
> }
> #endif
> 
> #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> static inline int pud_devmap(pud_t pud)
> {
>         return !!(pud_val(pud) & _PAGE_DEVMAP);
> }
> #else
> static inline int pud_devmap(pud_t pud)
> {
>         return 0;
> }
> #endif
> 
> We might need some more clarity on this regarding x86 platform's pud huge
> page implementation.
> 

static vm_fault_t create_huge_pud(struct vm_fault *vmf)
{
#if defined(CONFIG_TRANSPARENT_HUGEPAGE) &&			\
	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
	struct vm_area_struct *vma = vmf->vma;
	/* No support for anonymous transparent PUD pages yet */
	if (vma_is_anonymous(vma))
		return VM_FAULT_FALLBACK;
	if (vma->vm_ops->huge_fault)
		return vma->vm_ops->huge_fault(vmf, PUD_ORDER);
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
	return VM_FAULT_FALLBACK;
}



-aneesh