Re: [PATCH hotfix] mm: fix pmd_special() fallback to observe huge_zero

["David Hildenbrand (Arm)" <david@kernel.org>]

>> ... and triggering the VM_WARN_ON_ONE(), although never reported so far.
>>
>> To fix it, teach vm_normal_page_pmd()/vm_normal_page_pud() whether
>> pmd_special/pud_special is actually implemented.
>>
>> Fixes: af38538801c6 ("mm/memory: factor out common code from vm_normal_page_*()")
> 
> Agreed.  My Fixes tag (where my bisection arrived) was correct for the
> common zap_huge_pmd() symptom I was seeing (Lorenzo's commit removed
> an independent is_huge_zero_pmd() check from it, so it now relies on
> vm_normal_folio_pmd() to give the right answer).  But you've chased
> up other usages, and realized it goes back further.  You could just as
> well blame the other commit mentioned in this thread, the d82d09e48219
> ("mm/huge_memory: mark PMD mappings of the huge zero folio special"),
> because in come configs it is not doing what it expects to be doing.
> But af38538801c6 is where effects start appearing, so fine to blame
> it (and both come from the same 6.18 series, do it doesn't matter).
> 
>> Signed-off-by: Hugh Dickins <hughd@google.com>>
>> Co-developed-by: David Hildenbrand (Arm) <david@kernel.org>
> 
> That's generous, but the patch is not mine at all, and I'll
> happily let you grab my two paragraphs above.  Please, just

Okay, I didn't want to undermine your involvement.

> Reported-by: Hugh Dickins <hughd@google.com>

Then, I'd also add a

Debugged-by: Hugh Dickins <hughd@google.com>

> 
>> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
>> ---
>>  mm/memory.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 7322a40e73b9..a60bc07b48b2 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -612,6 +612,21 @@ static void print_bad_page_map(struct vm_area_struct *vma,
>>  	dump_stack();
>>  	add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
>>  }
>> +
>> +static inline bool pgtable_level_has_pxx_special(enum pgtable_level level)
>> +{
>> +	switch (level) {
>> +	case PGTABLE_LEVEL_PTE:
>> +		return IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL);
>> +	case PGTABLE_LEVEL_PMD:
>> +		return IS_ENABLED(CONFIG_ARCH_SUPPORTS_PMD_PFNMAP);
>> +	case PGTABLE_LEVEL_PUD:
>> +		return IS_ENABLED(CONFIG_ARCH_SUPPORTS_PUD_PFNMAP);
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>  #define print_bad_pte(vma, addr, pte, page) \
>>  	print_bad_page_map(vma, addr, pte_val(pte), page, PGTABLE_LEVEL_PTE)
>>  
>> @@ -684,7 +699,7 @@ static inline struct page *__vm_normal_page(struct vm_area_struct *vma,
>>  		unsigned long addr, unsigned long pfn, bool special,
>>  		unsigned long long entry, enum pgtable_level level)
>>  {
>> -	if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) {
>> +	if (pgtable_level_has_pxx_special(level)) {
>>  		if (unlikely(special)) {
>>  #ifdef CONFIG_FIND_NORMAL_PAGE
>>  			if (vma->vm_ops && vma->vm_ops->find_normal_page)
> 
> That block ends with a comment on CONFIG_ARCH_HAS_PTE_SPECIAL:
> perhaps better reworded now - but I don't know what to suggest!

Good point, let me take a loook.

> 
> This patch seems okay, but TBH I have no enthusiasm for it -
> it forces me to think too hard, and I prefer my own one-liner
> (iwhich Lance found odd: odd if you're thinking pmd_special(pmd)
> means pmd has the _PAGE_SPECIAL bit set, yes, but not odd if you
> think it means that pmd is of a special folio).

If pmd_mkspecial() is a nop I'd prefer that pmd_special() is similarly a nop.

> 
> But whatever, you and Lance prefer this one: thanks for the fix!

Thank you Hugh. I'll send out my version officially, and if there are strong
opinions against it we can just use your variant.

-- 
Cheers,

David