Re: [PATCH] sched/numa, mm: Skip page promotion if cpu pid is valid

[Donet Tom <donettom@linux.ibm.com>]

On 3/31/26 2:47 PM, Huang, Ying wrote:
> Donet Tom <donettom@linux.ibm.com> writes:
>
>> Hi
>>
>> On 3/31/26 2:03 PM, Huang, Ying wrote:
>>> Hi, Donet,
>>>
>>> Donet Tom <donettom@linux.ibm.com> writes:
>>>
>>>> On 3/26/26 3:59 PM, David Hildenbrand (Arm) wrote:
>>>>> On 3/26/26 08:12, Donet Tom wrote:
>>>>>> If memory tiering is disabled, cpupid of slow memory pages may
>>>>>> contain a valid CPU and PID. If tiering is enabled at runtime,
>>>>>> there is a chance that in should_numa_migrate_memory(), this
>>>>>> valid CPU/PID is treated as a last access timestamp, leading
>>>>>> to unnecessary promotion.
>>>>> Is that measurable? Should we at least have a Fixes: ?
>>>>>
>>>>>> Prevent this by skipping promotion when cpupid is valid.
>>>>>>
>>>>>> Signed-off-by: Donet Tom <donettom@linux.ibm.com>
>>>>>> ---
>>>>>>     kernel/sched/fair.c | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>>>> index 4b43809a3fb1..f5830a5a94d5 100644
>>>>>> --- a/kernel/sched/fair.c
>>>>>> +++ b/kernel/sched/fair.c
>>>>>> @@ -2001,6 +2001,13 @@ bool should_numa_migrate_memory(struct task_struct *p, struct folio *folio,
>>>>>>     		unsigned int latency, th, def_th;
>>>>>>     		long nr = folio_nr_pages(folio);
>>>>>>     
>>>>> /*
>>>>>     * When ...
>>>>>
>>>>>> +		/* When tiering is enabled at runtime, last_cpupid may
>>>>>> +		 * hold a valid cpupid instead of an access timestamp.
>>>>>> +		 * If so, skip page promotion.
>>>>>> +		 */
>>>>>> +		if (cpupid_valid(folio_last_cpupid(folio)))
>>>>>> +			return false;
>>>>>> +
>>>>> IIUC, as timestamp we use jiffies_to_msecs(). So, soon after bootup,
>>>>> we would no longer get false positives for cpupid_valid().
>>>>> I suppose overflows are not a problem, correct?
>>>> Thank you, David, for guiding me in the right direction.
>>>>
>>>> I initially thought that overflows would not occur, and therefore
>>>> cpupid_valid() would not produce false positives. However,
>>>> after looking into it further, it appears that overflow can
>>>> happen when storing the access time.
>>>>
>>>> The last_cpupid field is used to store the last access time.
>>>>   From the code, it appears that 21 bits are used for this
>>>> (#define LAST_CPUPID_SHIFT (LAST__PID_SHIFT + LAST__CPU_SHIFT)).
>>>>
>>>> With 21 bits, the maximum value that can be stored is
>>> It can be less than 21 bits, if CONFIG_NR_CPUS is small.
>>>
>>> 	DEFINE(NR_CPUS_BITS, order_base_2(CONFIG_NR_CPUS));
>>>
>>>> 2097151ms (35Hrs) . If the access time exceeds this
>>>> range, it can overflow, which may lead to cpupid_valid()
>>>> returning false positives.
>>>>
>>>> I think we need a reliable way to determine cpupid_valid() that
>>>> does not produce false positives.
>>> Yes.  IMHO, false positives is unavoidable.  So, the patch fixes a
>>> temporal performance issue at the cost of a longstanding performance
>>> issue.  Right?
>>
>> I was trying to fix a functional issue. When memory tiering is
>>
>> enabled at runtime, treating last_cpupid as access time is incorrect, right?
> I don't think that it's a functional issue.  It has only performance
> impact.  Did you find any functionality bug?
>

Thank you for the confirmation. I thought this was a functional issue. 
In that case, we can drop this patch.

-Donet


>
> ---
> Best Regards,
> Huang, Ying