Re: [RFC V2] mm/vmstat: Add events for PMD based THP migration without split

[John Hubbard <jhubbard@nvidia.com>]

On 2020-05-17 23:42, Anshuman Khandual wrote:
...
> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
> index ffef0f279747..23d8f9884c2b 100644
> --- a/include/linux/vm_event_item.h
> +++ b/include/linux/vm_event_item.h
> @@ -91,6 +91,10 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>   		THP_ZERO_PAGE_ALLOC_FAILED,
>   		THP_SWPOUT,
>   		THP_SWPOUT_FALLBACK,
> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION


Hi Anshuman,

These new events look great to me. I have some nits below, plus one
lightly controversial suggestion which I'd really like to have someone
more experienced weigh in on, which is:

How about not being quite so granular on the THP config options, and
just guarding these events with the overall CONFIG_TRANSPARENT_HUGEPAGE
option, instead of the sub-option CONFIG_ARCH_ENABLE_THP_MIGRATION?

I tentatively think it's harmless and not really misleading to have
/proc/vmstat showing this in all THP-enabled configurations:

thp_pmd_migration_success 0
thp_pmd_migration_failure 0

...if THP is enabled, and *whether or not* _THP_MIGRATION is enabled.
And this simplifies things a bit. Given how the .config options can get,
I think simplifying would be nice.

However, I'm ready to be corrected on that, if it's a bad idea for
other API reasons perhaps.  Can anyone please comment?


> +		THP_PMD_MIGRATION_SUCCESS,
> +		THP_PMD_MIGRATION_FAILURE,
> +#endif
>   #endif
>   #ifdef CONFIG_MEMORY_BALLOON
>   		BALLOON_INFLATE,
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 7160c1556f79..5325700a3e90 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1170,6 +1170,18 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
>   #define ICE_noinline
>   #endif
>   
> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> +static inline void thp_migration_success(bool success)


I think this should be named

     thp_pmd_migration_success()

, since that's what you're really counting. Or, you could
name the events THP_MIGRATION_SUCCESS|FAILURE. Either way,
just so the function name matches the events it's counting.


> +{
> +	if (success)
> +		count_vm_event(THP_PMD_MIGRATION_SUCCESS);
> +	else
> +		count_vm_event(THP_PMD_MIGRATION_FAILURE);
> +}
> +#else
> +static inline void thp_migration_success(bool success) { }


This whole ifdef clause would disappear if my suggestion above is
accepted. However, if not, then I believe the convention for this
kind of situation is:

static inline void thp_migration_success(bool success)
{
}


> +#endif
> +
>   /*
>    * Obtain the lock on page, remove all ptes and migrate the page
>    * to the newly allocated page in newpage.
> @@ -1232,6 +1244,8 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>   	 * we want to retry.
>   	 */
>   	if (rc == MIGRATEPAGE_SUCCESS) {
> +		if (PageTransHuge(newpage))
> +			thp_migration_success(true);
>   		put_page(page);
>   		if (reason == MR_MEMORY_FAILURE) {
>   			/*
> @@ -1474,6 +1488,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>   					unlock_page(page);
>   					if (!rc) {
>   						list_safe_reset_next(page, page2, lru);
> +						thp_migration_success(false);
>   						goto retry;
>   					}
>   				}
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 96d21a792b57..e258c782fd3a 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1274,6 +1274,10 @@ const char * const vmstat_text[] = {
>   	"thp_zero_page_alloc_failed",
>   	"thp_swpout",
>   	"thp_swpout_fallback",
> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> +	"thp_pmd_migration_success",
> +	"thp_pmd_migration_failure",
> +#endif
>   #endif
>   #ifdef CONFIG_MEMORY_BALLOON
>   	"balloon_inflate",
> 

thanks,
-- 
John Hubbard
NVIDIA