Re: [RFC] mm/pgtable/debug: Add test validating architecture page table helpers

[Anshuman Khandual]

On 07/25/2019 08:09 PM, Matthew Wilcox wrote:
> On Thu, Jul 25, 2019 at 12:25:23PM +0530, Anshuman Khandual wrote:
>> This adds a test module which will validate architecture page table helpers
>> and accessors regarding compliance with generic MM semantics expectations.
>> This will help various architectures in validating changes to the existing
>> page table helpers or addition of new ones.
> 
> I think this is a really good idea.
> 
>>  lib/Kconfig.debug       |  14 +++
>>  lib/Makefile            |   1 +
>>  lib/test_arch_pgtable.c | 290 ++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Is this the right place for it?  I worry that lib/ is going to get overloaded
> with test code, and this feels more like mm/ test code.

Sure this can be moved to mm/ but unlike existing test configs there (following)
lets keep some config description in mm/Kconfig. Will probably rename this as
CONFIG_DEBUG_ARCH_PGTABLE_TEST to match other tests.

CONFIG_DEBUG_KMEMLEAK_TEST
CONFIG_DEBUG_RODATA_TEST
CONFIG_MEMTEST

After moving to mm/ directory I guess it does not need a loadable module option.

> 
>> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE
>> +static void pmd_basic_tests(void)
>> +{
>> +	pmd_t pmd;
>> +
>> +	pmd = mk_pmd(page, prot);
> 
> But 'page' isn't necessarily PMD-aligned.  I don't think we can rely on
> architectures doing the right thing if asked to make a PMD for a randomly
> aligned page.
> 
> How about finding the physical address of something like kernel_init(),

Physical address corresponding to the symbol in the kernel text segment ?

> and using the corresponding pte/pmd/pud/p4d/pgd that encompasses that

So I guess this will help us use pte/pmd/pud/p4d/pgd entries from a real and
present mapping rather then making them up for test purpose. Although we are
not creating real page tables here just wondering if this could some how
affect these real mapping in anyway from some accessors. The current proposal
stays clear from anything real - allocates, evaluates and releases.

Also table entries at pmd/pud/p4d/pgd cannot be operated with accessors in the
test. THP and PUD THP will operate on leaf entries at pmd or pud levels. We need
them as leaf entries created from allocated (aligned) pfns. While determining
pte/pmd/pud/p4d/pgd for kernel_init() some of them will be table entries.

> address?  It's also better to pass in the pfn/page rather than using global
> variables to communicate to the test functions.

Sure those can be allocated and passed from the main function. Just wanted to
avoid page allocation in each individual tests.

> 
>> +	/*
>> +	 * A huge page does not point to next level page table
>> +	 * entry. Hence this must qualify as pmd_bad().
>> +	 */
>> +	WARN_ON(!pmd_bad(pmd_mkhuge(pmd)));
> 
> I didn't know that rule.  This is helpful because it gives us somewhere
> to document all these tricksy little rules.

That is another objective this test has which will help settle semantics
in a clear and documented manner.

> 
>> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>> +static void pud_basic_tests(void)
> 
> Is this the right ifdef?

IIUC THP at PUD is where the pud_t entries are directly operated upon and the
corresponding accessors are present only when HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
is enabled. Am I missing something here ?