Messages in this thread | | | Subject | Re: [RFC V2] mm/vmstat: Add events for PMD based THP migration without split | From | John Hubbard <> | Date | Tue, 19 May 2020 22:17:20 -0700 |
| |
On 2020-05-19 20:32, Anshuman Khandual wrote: ... >> 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? > > There is no THP migration events to track unless it is enabled. Why to > show these statistics (as 0) when its not even possible. If the config > simplicity is the only intended rationale here, it might not be the > case either. These events and their tracking would still need to be > wrapped with CONFIG_TRANSPARENT_HUGEPAGE otherwise. > > If your concern is more towards CONFIG_ARCH_ENABLE_THP_MIGRATION being > unsuitable or with complex dependencies, then that is something how THP > migration feature itself is implemented currently and adding VM events > does not address that. A possible patch in the future patch could solve > all these (together). > > But sure, let's hear it for what others have to say on this.
Well, I don't want to hold up progress. If it's not very convincing to you, let's just drop the idea/ It was kind of weak. :)
>>> + 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. > > Makes sense but IMHO we should keep _pmd_ to be more specific. > Will change the name here as thp_pmd_migration_success(). > >> >> >>> +{ >>> + 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 > > We will have to protect these with CONFIG_TRANSPARENT_HUGEPAGE as > the events are still conditionally available. >
Yes you are right, of course. And I even worked through that, but then when I sat down to write a response my fingers typed v1 of my understanding instead of v2. No one knows why. :) Sorry about the misinformation there.
>> accepted. However, if not, then I believe the convention for this >> kind of situation is: >> >> static inline void thp_migration_success(bool success) >> { >> } > > AFAIK, we have examples both ways but will change if this is preferred. >
Not worth worrying about, but I do recall a few recent code reviews that all preferred the multi-line version, which is why I suggested it.
Anyway, either way, with the thp_pmd_migration_success() name change, you can add:
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks, -- John Hubbard NVIDIA
| |