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

[John Hubbard]

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@xxxxxxxxxx>


thanks,
--
John Hubbard
NVIDIA