Re: [PATCH v3] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand <david@redhat.com>]

Hi Peter,

>> -	if (huge_page_size(h) == PMD_SIZE)
>> +	unsigned long size = huge_page_size(h);
>> +
>> +	VM_WARN_ON(size == PAGE_SIZE);
>> +
>> +	/*
>> +	 * hugetlb must use the exact same PT locks as core-mm page table
>> +	 * walkers would. When modifying a PTE table, hugetlb must take the
>> +	 * PTE PT lock, when modifying a PMD table, hugetlb must take the PMD
>> +	 * PT lock etc.
>> +	 *
>> +	 * The expectation is that any hugetlb folio smaller than a PMD is
>> +	 * always mapped into a single PTE table and that any hugetlb folio
>> +	 * smaller than a PUD (but at least as big as a PMD) is always mapped
>> +	 * into a single PMD table.
>> +	 *
>> +	 * If that does not hold for an architecture, then that architecture
>> +	 * must disable split PT locks such that all *_lockptr() functions
>> +	 * will give us the same result: the per-MM PT lock.
>> +	 *
>> +	 * Note that with e.g., CONFIG_PGTABLE_LEVELS=2 where
>> +	 * PGDIR_SIZE==P4D_SIZE==PUD_SIZE==PMD_SIZE, we'd use the MM PT lock
>> +	 * directly with a PMD hugetlb size, whereby core-mm would call
>> +	 * pmd_lockptr() instead. However, in such configurations split PMD
>> +	 * locks are disabled -- split locks don't make sense on a single
>> +	 * PGDIR page table -- and the end result is the same.
>> +	 */
>> +	if (size >= P4D_SIZE)
>> +		return &mm->page_table_lock;
> 
> I'd drop this so the mm lock fallback will be done below (especially in
> reality the pud lock is always mm lock for now..).  Also this line reads
> like there can be P4D size huge page but in reality PUD is the largest
> (nopxx doesn't count).  We also same some cycles in most cases if removed.

The compiler should be smart enough to optimize most of that out. But
I agree that there is no need to be that future-proof here.

These two are interesting:

arch/powerpc/mm/hugetlbpage.c:  if (!mm_pud_folded(mm) && sz >= P4D_SIZE)
arch/riscv/mm/hugetlbpage.c:    else if (sz >= P4D_SIZE)

But I assume they are only fordward-looking. (GUP would already be
pretty broken if that would exist)


>> +	else if (size >= PUD_SIZE)
>> +		return pud_lockptr(mm, (pud_t *) pte);
>> +	else if (size >= PMD_SIZE || IS_ENABLED(CONFIG_HIGHPTE))
> 
> I thought this HIGHPTE can also be dropped? Because in HIGHPTE it should
> never have lower-than-PMD huge pages or we're in trouble.  That's why I
> kept one WARN_ON() in my pesudo code but only before trying to take the pte
> lockptr.

Then the compiler won't optimize out the ptep_lockptr() call and we'll run
into a build error. And I think the HIGHPTE builderror serves good purpose.

In file included from <command-line>:
In function 'ptep_lockptr',
     inlined from 'huge_pte_lockptr' at ./include/linux/hugetlb.h:974:9,
     inlined from 'huge_pte_lock' at ./include/linux/hugetlb.h:1248:8,
     inlined from 'pagemap_scan_hugetlb_entry' at fs/proc/task_mmu.c:2581:8:
././include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_256' declared with attribute error: BUILD_BUG_ON failed: IS_ENABLED(CONFIG_HIGHPTE)
   510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
       |                                             ^
././include/linux/compiler_types.h:491:25: note: in definition of macro '__compiletime_assert'
   491 |                         prefix ## suffix();                             \
       |                         ^~~~~~
././include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
   510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
       |         ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
    39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
       |                                     ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
    50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
       |         ^~~~~~~~~~~~~~~~
./include/linux/mm.h:2874:9: note: in expansion of macro 'BUILD_BUG_ON'
  2874 |         BUILD_BUG_ON(IS_ENABLED(CONFIG_HIGHPTE));



It would be even better to have a way to know whether an arch even has
hugetlb < PMD_SIZE (config option?) and use that instead here; but I'll leave
that as an optimization.


-- 
Cheers,

David / dhildenb