lkml.org 
[lkml]   [2020]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [RFC V2] mm/vmstat: Add events for PMD based THP migration without split
From
Date
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

\
 
 \ /
  Last update: 2022-09-17 16:07    [W:0.063 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site