Messages in this thread Patch in this message | | | Subject | Re: [RFC V2] mm/vmstat: Add events for PMD based THP migration without split | From | John Hubbard <> | Date | Mon, 18 May 2020 13:10:20 -0700 |
| |
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
| |