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

\
 
 \ /
  Last update: 2020-05-20 07:17    [W:0.072 / U:0.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site