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

[John Hubbard <jhubbard@nvidia.com>]

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