| Message ID | 20210727061401.592616-7-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/debug_vm_pgtable: Enhancements | expand |
On 7/27/21 11:43 AM, Gavin Shan wrote: > This uses struct pgtable_debug_args in the migration and thp test > functions. It's notable that the pre-allocated page is used in > swap_migration_tests() as set_pte_at() is used there. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > mm/debug_vm_pgtable.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index bc153cad9045..9136195efde3 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -845,7 +845,7 @@ static void __init pmd_swap_tests(struct pgtable_debug_args *args) > static void __init pmd_swap_tests(struct pgtable_debug_args *args) { } > #endif /* CONFIG_ARCH_ENABLE_THP_MIGRATION */ > > -static void __init swap_migration_tests(void) > +static void __init swap_migration_tests(struct pgtable_debug_args *args) > { > struct page *page; > swp_entry_t swp; > @@ -861,9 +861,10 @@ static void __init swap_migration_tests(void) > * problematic. Lets allocate a dedicated page explicitly for this > * purpose that will be freed subsequently. > */ > - page = alloc_page(GFP_KERNEL); > + page = (args->pte_pfn != ULONG_MAX) ? > + pfn_to_page(args->pte_pfn) : NULL; > if (!page) { > - pr_err("page allocation failed\n"); > + pr_err("no page available\n"); > return; > } Please check for a valid page earlier in the function and return. Otherwise this calls out the page unavailability (after starting the test), which is inconsistent with all other functions like pxx_advanced_tests(). [ 1.051633] debug_vm_pgtable: [pte_swap_tests ]: Validating PTE swap [ 1.052697] debug_vm_pgtable: [pmd_swap_tests ]: Validating PMD swap [ 1.053765] debug_vm_pgtable: [swap_migration_tests ]: Validating swap migration <===== [ 1.054900] debug_vm_pgtable: [swap_migration_tests ]: no page available <===== Should do this just before pr_info("Validating swap migration\n"). ...... page = (args->pte_pfn != ULONG_MAX) ? pfn_to_page(args->pte_pfn) : NULL; if (!page) return; .....
Hi Anshuman, On 7/28/21 9:08 PM, Anshuman Khandual wrote: > On 7/27/21 11:43 AM, Gavin Shan wrote: >> This uses struct pgtable_debug_args in the migration and thp test >> functions. It's notable that the pre-allocated page is used in >> swap_migration_tests() as set_pte_at() is used there. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> mm/debug_vm_pgtable.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index bc153cad9045..9136195efde3 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -845,7 +845,7 @@ static void __init pmd_swap_tests(struct pgtable_debug_args *args) >> static void __init pmd_swap_tests(struct pgtable_debug_args *args) { } >> #endif /* CONFIG_ARCH_ENABLE_THP_MIGRATION */ >> >> -static void __init swap_migration_tests(void) >> +static void __init swap_migration_tests(struct pgtable_debug_args *args) >> { >> struct page *page; >> swp_entry_t swp; >> @@ -861,9 +861,10 @@ static void __init swap_migration_tests(void) >> * problematic. Lets allocate a dedicated page explicitly for this >> * purpose that will be freed subsequently. >> */ >> - page = alloc_page(GFP_KERNEL); >> + page = (args->pte_pfn != ULONG_MAX) ? >> + pfn_to_page(args->pte_pfn) : NULL; >> if (!page) { >> - pr_err("page allocation failed\n"); >> + pr_err("no page available\n"); >> return; >> } > > Please check for a valid page earlier in the function and return. Otherwise > this calls out the page unavailability (after starting the test), which is > inconsistent with all other functions like pxx_advanced_tests(). > > [ 1.051633] debug_vm_pgtable: [pte_swap_tests ]: Validating PTE swap > [ 1.052697] debug_vm_pgtable: [pmd_swap_tests ]: Validating PMD swap > [ 1.053765] debug_vm_pgtable: [swap_migration_tests ]: Validating swap migration <===== > [ 1.054900] debug_vm_pgtable: [swap_migration_tests ]: no page available <===== > > Should do this just before pr_info("Validating swap migration\n"). > > ...... > page = (args->pte_pfn != ULONG_MAX) ? pfn_to_page(args->pte_pfn) : NULL; > if (!page) > return; > ..... > Yes. The order of error messages are sticky to original implementation, but it'd better to be consistent with the new order we have in this series. I will adjust in v5. Thanks, Gavin
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index bc153cad9045..9136195efde3 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -845,7 +845,7 @@ static void __init pmd_swap_tests(struct pgtable_debug_args *args) static void __init pmd_swap_tests(struct pgtable_debug_args *args) { } #endif /* CONFIG_ARCH_ENABLE_THP_MIGRATION */ -static void __init swap_migration_tests(void) +static void __init swap_migration_tests(struct pgtable_debug_args *args) { struct page *page; swp_entry_t swp; @@ -861,9 +861,10 @@ static void __init swap_migration_tests(void) * problematic. Lets allocate a dedicated page explicitly for this * purpose that will be freed subsequently. */ - page = alloc_page(GFP_KERNEL); + page = (args->pte_pfn != ULONG_MAX) ? + pfn_to_page(args->pte_pfn) : NULL; if (!page) { - pr_err("page allocation failed\n"); + pr_err("no page available\n"); return; } @@ -884,7 +885,6 @@ static void __init swap_migration_tests(void) WARN_ON(!is_migration_entry(swp)); WARN_ON(is_writable_migration_entry(swp)); __ClearPageLocked(page); - __free_page(page); } #ifdef CONFIG_HUGETLB_PAGE @@ -916,7 +916,7 @@ static void __init hugetlb_basic_tests(struct pgtable_debug_args *args) { } #endif /* CONFIG_HUGETLB_PAGE */ #ifdef CONFIG_TRANSPARENT_HUGEPAGE -static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot) +static void __init pmd_thp_tests(struct pgtable_debug_args *args) { pmd_t pmd; @@ -935,7 +935,7 @@ static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot) * needs to return true. pmd_present() should be true whenever * pmd_trans_huge() returns true. */ - pmd = pfn_pmd(pfn, prot); + pmd = pfn_pmd(args->fixed_pmd_pfn, args->page_prot); WARN_ON(!pmd_trans_huge(pmd_mkhuge(pmd))); #ifndef __HAVE_ARCH_PMDP_INVALIDATE @@ -945,7 +945,7 @@ static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot) } #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD -static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) +static void __init pud_thp_tests(struct pgtable_debug_args *args) { pud_t pud; @@ -953,7 +953,7 @@ static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) return; pr_debug("Validating PUD based THP\n"); - pud = pfn_pud(pfn, prot); + pud = pfn_pud(args->fixed_pud_pfn, args->page_prot); WARN_ON(!pud_trans_huge(pud_mkhuge(pud))); /* @@ -965,11 +965,11 @@ static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) */ } #else /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */ -static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) { } +static void __init pud_thp_tests(struct pgtable_debug_args *args) { } #endif /* CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */ #else /* !CONFIG_TRANSPARENT_HUGEPAGE */ -static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot) { } -static void __init pud_thp_tests(unsigned long pfn, pgprot_t prot) { } +static void __init pmd_thp_tests(struct pgtable_debug_args *args) { } +static void __init pud_thp_tests(struct pgtable_debug_args *args) { } #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ static unsigned long __init get_random_vaddr(void) @@ -1345,10 +1345,10 @@ static int __init debug_vm_pgtable(void) pte_swap_tests(&args); pmd_swap_tests(&args); - swap_migration_tests(); + swap_migration_tests(&args); - pmd_thp_tests(pmd_aligned, prot); - pud_thp_tests(pud_aligned, prot); + pmd_thp_tests(&args); + pud_thp_tests(&args); hugetlb_basic_tests(&args);
This uses struct pgtable_debug_args in the migration and thp test functions. It's notable that the pre-allocated page is used in swap_migration_tests() as set_pte_at() is used there. Signed-off-by: Gavin Shan <gshan@redhat.com> --- mm/debug_vm_pgtable.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)