diff mbox series

[v2,3/3] memory tiering: count PGPROMOTE_SUCCESS when mem tiering is enabled.

Message ID 20240722172917.503370-3-ziy@nvidia.com (mailing list archive)
State New
Headers show
Series [v2,1/3] memory tiering: read last_cpupid correctly in do_huge_pmd_numa_page() | expand

Commit Message

Zi Yan July 22, 2024, 5:29 p.m. UTC
memory tiering can be enabled/disabled at runtime and
sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
it. In migrate_misplaced_folio(), the check is missing when
PGPROMOTE_SUCCESS is incremented. Add the missing check.

Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/migrate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Kefeng Wang July 23, 2024, 1:48 a.m. UTC | #1
On 2024/7/23 1:29, Zi Yan wrote:
> memory tiering can be enabled/disabled at runtime and
> sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
> it. In migrate_misplaced_folio(), the check is missing when
> PGPROMOTE_SUCCESS is incremented. Add the missing check.
> 
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
> Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
> Signed-off-by: Zi Yan <ziy@nvidia.com>

Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

> ---
>   mm/migrate.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index bdbb5bb04c91..b819809da470 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
>   		putback_movable_pages(&migratepages);
>   	if (nr_succeeded) {
>   		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
> -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
> +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
> +		    && !node_is_toptier(folio_nid(folio))
> +		    && node_is_toptier(node))
>   			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
>   					    nr_succeeded);

The should be in advance of patch2, and change above to use 
folio_has_cpupid() helper() too.

>   	}
Zi Yan July 23, 2024, 1:54 a.m. UTC | #2
On Mon Jul 22, 2024 at 9:48 PM EDT, Kefeng Wang wrote:
>
>
> On 2024/7/23 1:29, Zi Yan wrote:
> > memory tiering can be enabled/disabled at runtime and
> > sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
> > it. In migrate_misplaced_folio(), the check is missing when
> > PGPROMOTE_SUCCESS is incremented. Add the missing check.
> > 
> > Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
> > Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
> > Signed-off-by: Zi Yan <ziy@nvidia.com>
>
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>
Thanks.

> > ---
> >   mm/migrate.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index bdbb5bb04c91..b819809da470 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
> >   		putback_movable_pages(&migratepages);
> >   	if (nr_succeeded) {
> >   		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
> > -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
> > +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
> > +		    && !node_is_toptier(folio_nid(folio))
> > +		    && node_is_toptier(node))
> >   			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
> >   					    nr_succeeded);
>
> The should be in advance of patch2, and change above to use 
> folio_has_cpupid() helper() too.

It shares the same logic of !folio_has_cpupid() but it might be confusing to
put !folio_has_cpupid(folio) && node_is_toptier(node) here. folio's
cpupid has nothing to do with the stats here, thus I did not use the
function.
Kefeng Wang July 23, 2024, 3:24 a.m. UTC | #3
On 2024/7/23 9:54, Zi Yan wrote:
> On Mon Jul 22, 2024 at 9:48 PM EDT, Kefeng Wang wrote:
>>
>>
>> On 2024/7/23 1:29, Zi Yan wrote:
>>> memory tiering can be enabled/disabled at runtime and
>>> sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
>>> it. In migrate_misplaced_folio(), the check is missing when
>>> PGPROMOTE_SUCCESS is incremented. Add the missing check.
>>>
>>> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
>>> Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>
>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>
> Thanks.
> 
>>> ---
>>>    mm/migrate.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index bdbb5bb04c91..b819809da470 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
>>>    		putback_movable_pages(&migratepages);
>>>    	if (nr_succeeded) {
>>>    		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
>>> -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
>>> +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
>>> +		    && !node_is_toptier(folio_nid(folio))
>>> +		    && node_is_toptier(node))
>>>    			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
>>>    					    nr_succeeded);
>>
>> The should be in advance of patch2, and change above to use
>> folio_has_cpupid() helper() too.
> 
> It shares the same logic of !folio_has_cpupid() but it might be confusing to
> put !folio_has_cpupid(folio) && node_is_toptier(node) here. folio's
> cpupid has nothing to do with the stats here, thus I did not use the
> function.

If folio don't include access time, we do migrate it but it isn't a 
promotion, so don't count it, other comments?

PS: Could we rename folio_has_cpupid() to folio_has_access_time(), even 
without memory_tiering, we still have cpupid in folio, right?
>
Huang, Ying July 23, 2024, 5:46 a.m. UTC | #4
Kefeng Wang <wangkefeng.wang@huawei.com> writes:

> On 2024/7/23 9:54, Zi Yan wrote:
>> On Mon Jul 22, 2024 at 9:48 PM EDT, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/7/23 1:29, Zi Yan wrote:
>>>> memory tiering can be enabled/disabled at runtime and
>>>> sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
>>>> it. In migrate_misplaced_folio(), the check is missing when
>>>> PGPROMOTE_SUCCESS is incremented. Add the missing check.
>>>>
>>>> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
>>>> Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>
>>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>
>> Thanks.
>> 
>>>> ---
>>>>    mm/migrate.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index bdbb5bb04c91..b819809da470 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
>>>>    		putback_movable_pages(&migratepages);
>>>>    	if (nr_succeeded) {
>>>>    		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
>>>> -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
>>>> +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
>>>> +		    && !node_is_toptier(folio_nid(folio))
>>>> +		    && node_is_toptier(node))
>>>>    			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
>>>>    					    nr_succeeded);
>>>
>>> The should be in advance of patch2, and change above to use
>>> folio_has_cpupid() helper() too.
>> It shares the same logic of !folio_has_cpupid() but it might be
>> confusing to
>> put !folio_has_cpupid(folio) && node_is_toptier(node) here. folio's
>> cpupid has nothing to do with the stats here, thus I did not use the
>> function.
>
> If folio don't include access time, we do migrate it but it isn't a
> promotion, so don't count it, other comments?

Personally, I prefer to use !node_is_toptier() && node_is_toptier()
here.  That sounds more natural for me.

> PS: Could we rename folio_has_cpupid() to folio_has_access_time(),
> even without memory_tiering, we still have cpupid in folio, right?

--
Best Regards,
Huang, Ying
David Hildenbrand July 23, 2024, 10:17 a.m. UTC | #5
On 23.07.24 05:24, Kefeng Wang wrote:
> 
> 
> On 2024/7/23 9:54, Zi Yan wrote:
>> On Mon Jul 22, 2024 at 9:48 PM EDT, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/7/23 1:29, Zi Yan wrote:
>>>> memory tiering can be enabled/disabled at runtime and
>>>> sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
>>>> it. In migrate_misplaced_folio(), the check is missing when
>>>> PGPROMOTE_SUCCESS is incremented. Add the missing check.
>>>>
>>>> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
>>>> Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>
>>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>
>> Thanks.
>>
>>>> ---
>>>>     mm/migrate.c | 4 +++-
>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index bdbb5bb04c91..b819809da470 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
>>>>     		putback_movable_pages(&migratepages);
>>>>     	if (nr_succeeded) {
>>>>     		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
>>>> -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
>>>> +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
>>>> +		    && !node_is_toptier(folio_nid(folio))
>>>> +		    && node_is_toptier(node))
>>>>     			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
>>>>     					    nr_succeeded);
>>>
>>> The should be in advance of patch2, and change above to use
>>> folio_has_cpupid() helper() too.
>>
>> It shares the same logic of !folio_has_cpupid() but it might be confusing to
>> put !folio_has_cpupid(folio) && node_is_toptier(node) here. folio's
>> cpupid has nothing to do with the stats here, thus I did not use the
>> function.
> 
> If folio don't include access time, we do migrate it but it isn't a
> promotion, so don't count it, other comments?
> 
> PS: Could we rename folio_has_cpupid() to folio_has_access_time(), even
> without memory_tiering, we still have cpupid in folio, right?

Maybe call it "folio_use_cpupid()" or sth like that? The "has" is a bit 
misleading, because the folio has a cpuid in any case, no?
Zi Yan July 23, 2024, 1:03 p.m. UTC | #6
On Tue Jul 23, 2024 at 6:17 AM EDT, David Hildenbrand wrote:
> On 23.07.24 05:24, Kefeng Wang wrote:
> > 
> > 
> > On 2024/7/23 9:54, Zi Yan wrote:
> >> On Mon Jul 22, 2024 at 9:48 PM EDT, Kefeng Wang wrote:
> >>>
> >>>
> >>> On 2024/7/23 1:29, Zi Yan wrote:
> >>>> memory tiering can be enabled/disabled at runtime and
> >>>> sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
> >>>> it. In migrate_misplaced_folio(), the check is missing when
> >>>> PGPROMOTE_SUCCESS is incremented. Add the missing check.
> >>>>
> >>>> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> >>>> Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
> >>>> Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
> >>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
> >>>
> >>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> >>>
> >> Thanks.
> >>
> >>>> ---
> >>>>     mm/migrate.c | 4 +++-
> >>>>     1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/mm/migrate.c b/mm/migrate.c
> >>>> index bdbb5bb04c91..b819809da470 100644
> >>>> --- a/mm/migrate.c
> >>>> +++ b/mm/migrate.c
> >>>> @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
> >>>>     		putback_movable_pages(&migratepages);
> >>>>     	if (nr_succeeded) {
> >>>>     		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
> >>>> -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
> >>>> +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
> >>>> +		    && !node_is_toptier(folio_nid(folio))
> >>>> +		    && node_is_toptier(node))
> >>>>     			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
> >>>>     					    nr_succeeded);
> >>>
> >>> The should be in advance of patch2, and change above to use
> >>> folio_has_cpupid() helper() too.
> >>
> >> It shares the same logic of !folio_has_cpupid() but it might be confusing to
> >> put !folio_has_cpupid(folio) && node_is_toptier(node) here. folio's
> >> cpupid has nothing to do with the stats here, thus I did not use the
> >> function.
> > 
> > If folio don't include access time, we do migrate it but it isn't a
> > promotion, so don't count it, other comments?
> > 
> > PS: Could we rename folio_has_cpupid() to folio_has_access_time(), even
> > without memory_tiering, we still have cpupid in folio, right?

folio_has_access_time() would be the opposite of folio_has_cpupid().
If memory tiering is off (either at compile time or dynamically), a
folio has cpupid all the time.

>
> Maybe call it "folio_use_cpupid()" or sth like that? The "has" is a bit 
> misleading, because the folio has a cpuid in any case, no?

The folio's cpupid field is reused to record page access time, when the folio
is !node_is_toptier() and memory tiering mode is on.

In sum, using folio_use_access_time() as !folio_has_cpupid() seems
better to me, since it covers the special use of folio's cpupid field.

Let me know your thoughts.
Kefeng Wang July 24, 2024, 1:22 a.m. UTC | #7
On 2024/7/23 21:03, Zi Yan wrote:
> On Tue Jul 23, 2024 at 6:17 AM EDT, David Hildenbrand wrote:
>> On 23.07.24 05:24, Kefeng Wang wrote:
>>>
>>>
>>> On 2024/7/23 9:54, Zi Yan wrote:
>>>> On Mon Jul 22, 2024 at 9:48 PM EDT, Kefeng Wang wrote:
>>>>>
>>>>>
>>>>> On 2024/7/23 1:29, Zi Yan wrote:
>>>>>> memory tiering can be enabled/disabled at runtime and
>>>>>> sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING is used to check
>>>>>> it. In migrate_misplaced_folio(), the check is missing when
>>>>>> PGPROMOTE_SUCCESS is incremented. Add the missing check.
>>>>>>
>>>>>> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>>> Closes: https://lore.kernel.org/linux-mm/f4ae2c9c-fe40-4807-bdb2-64cf2d716c1a@huawei.com/
>>>>>> Fixes: 33024536bafd ("memory tiering: hot page selection with hint page fault latency")
>>>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>>>
>>>>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>>
>>>> Thanks.
>>>>
>>>>>> ---
>>>>>>      mm/migrate.c | 4 +++-
>>>>>>      1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>> index bdbb5bb04c91..b819809da470 100644
>>>>>> --- a/mm/migrate.c
>>>>>> +++ b/mm/migrate.c
>>>>>> @@ -2630,7 +2630,9 @@ int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
>>>>>>      		putback_movable_pages(&migratepages);
>>>>>>      	if (nr_succeeded) {
>>>>>>      		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
>>>>>> -		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
>>>>>> +		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
>>>>>> +		    && !node_is_toptier(folio_nid(folio))
>>>>>> +		    && node_is_toptier(node))
>>>>>>      			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
>>>>>>      					    nr_succeeded);
>>>>>
>>>>> The should be in advance of patch2, and change above to use
>>>>> folio_has_cpupid() helper() too.
>>>>
>>>> It shares the same logic of !folio_has_cpupid() but it might be confusing to
>>>> put !folio_has_cpupid(folio) && node_is_toptier(node) here. folio's
>>>> cpupid has nothing to do with the stats here, thus I did not use the
>>>> function.
>>>
>>> If folio don't include access time, we do migrate it but it isn't a
>>> promotion, so don't count it, other comments?
>>>
>>> PS: Could we rename folio_has_cpupid() to folio_has_access_time(), even
>>> without memory_tiering, we still have cpupid in folio, right?
> 
> folio_has_access_time() would be the opposite of folio_has_cpupid().
> If memory tiering is off (either at compile time or dynamically), a
> folio has cpupid all the time.
> 
>>
>> Maybe call it "folio_use_cpupid()" or sth like that? The "has" is a bit
>> misleading, because the folio has a cpuid in any case, no?
> 
> The folio's cpupid field is reused to record page access time, when the folio
> is !node_is_toptier() and memory tiering mode is on.
> 
> In sum, using folio_use_access_time() as !folio_has_cpupid() seems
> better to me, since it covers the special use of folio's cpupid field.
> 

It sounds good, thanks.

> Let me know your thoughts.
>
diff mbox series

Patch

diff --git a/mm/migrate.c b/mm/migrate.c
index bdbb5bb04c91..b819809da470 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2630,7 +2630,9 @@  int migrate_misplaced_folio(struct folio *folio, struct vm_area_struct *vma,
 		putback_movable_pages(&migratepages);
 	if (nr_succeeded) {
 		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
-		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
+		if ((sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
+		    && !node_is_toptier(folio_nid(folio))
+		    && node_is_toptier(node))
 			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
 					    nr_succeeded);
 	}