Re: [PATCH RFC v2] mm: memory-tiering: Fix PGPROMOTE_CANDIDATE accounting

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

On 08/07/2025 16:56, Huang, Ying wrote:
> "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:
> 
>> On 08/07/2025 10:47, Huang, Ying wrote:
>>> "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:
>>>
>>>> On 08/07/2025 09:14, Huang, Ying wrote:
>>>>> "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> On 25/06/2025 10:13, Li Zhijian wrote:
>>>>>>> V2:
>>>>>>> Fix compiling error # Reported by LKP
>>>>>>>
>>>>>>> As Ying suggested, we need to assess whether this change causes regression.
>>>>>>> However, considering the stringent conditions this patch involves,
>>>>>>> properly evaluating it may be challenging, as the outcomes depend on your
>>>>>>> perspective. Much like in a zero-sum game, if someone benefits, another
>>>>>>> might lose.
>>>>>>>
>>>>>>> If there are subsequent results, I will update them here.
>>>>>>
>>>>>> I ran memhog + pmbench to evaluate the impact of the patch(3 runs [1] for each kernel).
>>>>>>
>>>>>> The results show an approximate 4% performance increase in pmbench after applying this patch.
>>>>>>
>>>>>> Average     pmbench-access            max-promotion-rate
>>>>>> Before:     7956805 pages/sec                168301 pages/sec
>>>>>> After:      8313666 pages/sec (+4.4%)        207149 pages/sec
>>>>>
>>>>> It's hard for me to understand why performance increases because of
>>>>> higher promotion rate, while the expected behavior is more promotion
>>>>> rate limiting.
>>>>
>>>> Good question.
>>>>
>>>> Above max-promotion-rate means the maximum rate during the WHOLE pmbench period which
>>>> can not indicate the total promoted pages.
>>>>
>>>> Allow me to present each sample [0] recorded per second during the pmbench duration, as exemplified below:
>>>>
>>>>
>>>>                |       AFTER             |VS |           BEFORE       |
>>>> ------------+-------------------------+++++------------------------|
>>>> | Timestamp |  pgprom/s   |  pgdem/s  |   |  pgprom/s  |  pgdem/s  |
>>>> |-----------|-------------|-----------|---|------------|-----------|
>>>> |     1     |   122977    |     0     |   |   123051   |     0     |
>>>> |     2     |   50171     |     0     |   |   50159    |     0     |
>>>> |     3     |     18      |     0     |   |     28     |     0     |
>>>> |     4     |   16647     |     0     |   |     0      |     0     |
>>>> |     5     | 207149.5    |     0     |   |   78895    |     0     |
>>>> |     6     | 193411      | 161521    |   |  168301    |   8702    |
>>>> |     7     |  52464      |  53989    |   |   42294    |  39108    |
>>>> |     8     |   5133      |   2627    |   |     0      |     0     |
>>>> |     9     |     24      |     8     |   |   3875     |   6213    |
>>>> |    10     |     0       |     0     |   |  45513     |  43260    |
>>>> |    11     |     0       |     0     |   |  36600     |  44982    |
>>>> |    12     |     0       |     0     |   |  21091     |  11631    |
>>>> |    13     |     0       |     0     |   |  12276     |  10719    |
>>>> |    14     |     0       |     0     |   | 149699     | 149400    |
>>>> |    15     |     0       |     0     |   |   4026     |   4933    |
>>>> |    16     |     0       |     0     |   |   3780     |     0     |
>>>> |    17     |     0       |     0     |   |     2      |     0     |
>>>> |    18     |     0       |     0     |   |     0      |     0     |
>>>> |    19     |     0       |     0     |   |     0      |     0     |
>>>> |    20     |     0       |     0     |   |     0      |     0     |
>>>> |    21     |     0       |     0     |   |    62      |     0     |
>>>> |    22     |     0       |     0     |   |   2016     |     0     |
>>>> |    23     |     0       |     0     |   |     0      |     0     |
>>>> |    24     |     0       |     0     |   |    62      |     0     |
>>>> |    25     |   8308      |     0     |   |     1      |     0     |
>>>> |    26     |   220       |     0     |   |     0      |     0     |
>>>> |    27     |     0       |     0     |   |  1995.05   |     0     |
>>>> |    28     |     0       |     0     |   |     1      |     0     |
>>>> |    29     |   5791      |     0     |   |     0      |     0     |
>>>> |    30     |     0       |     0     |   |    62      |     0     |
>>>> ------------+-------------------------+++++------------------------|
>>>> |   total   | 662313.5    | 218145    |   | 743789.05  | 318948    |
>>>> |    max    | 207149.5    | 161521    |   |  168301    | 149400    |
>>>> ------------+-------------------------+++++------------------------|
>>>> |   pmbench |        8416250          |VS |        8079500         |
>>>>
>>>>
>>>> As far as I can tell, the higher pmbench scores applied-patch may be attributed to
>>>> a reduction in the total number of promoted pages in the entire pmbench execution period.
>>>> (Similar circumstances were observed in the results of other tests conducted)
>>>>
>>>>
>>>>
>>>> [0]
>>>> before:
>>>> https://github.com/zhijianli88/misc/blob/main/20250627/promotion-evaluation/without-patch/pmbench-1750988862.log
>>>> https://github.com/zhijianli88/misc/blob/main/20250627/promotion-evaluation/without-patch/sar-1750988862.log
>>>> after:
>>>> https://github.com/zhijianli88/misc/blob/main/20250627/promotion-evaluation/with-patch/pmbench-1750988291.log
>>>> https://github.com/zhijianli88/misc/blob/main/20250627/promotion-evaluation/with-patch/sar-1750988291.log
>>>>
>>>
>>> Check the usage of PGPROMOTE_CANDIDATE again.  It is used not only by
>>> rate limiting, but also promotion threshold adjustment, please take a
>>> look at numa_promotion_adjust_threshold().  Which may have larger
>>> influence on performance.
>>>
>>> After checking the threshold adjustment code, I think the changes in
>>> this patch may confuse threshold adjustment.
>>
>>
>> Indeed, I misunderstood the comment in the previous code:
>> /* workload changed, reset hot threshold */.
>>
>> Originally, this logic only reset the threshold for the current interval.
>> For the next cycle (60 seconds by default), the threshold is
>> re-evaluated based on the historical PGPROMOTE_CANDIDATE counts.
>> Therefore, the current change may affect threshold adjustment in subsequent cycles.
>>
>>
>> Do you think there's still a case to push for this patch?
>>
>> For example, by collecting more data with longer pmbench runs (over two threshold cycles),
>> or explicitly compensating nbp_rl_nr_cand and nbp_th_nr_cand to maintain existing
>> behavior for both the rate limit and threshold logic? something like:
>>
>> if (pgdat_free_space_enough(pgdat)) {
>>       /* workload changed, reset hot threshold */
>>       pgdat->nbp_threshold = 0;
>>       
>>       mod_node_page_state(pgdat, PGPROMOTE_CANDIDATE, nr);
>>       // compensation for rate limit and threshold
>>       pgdat->nbp_rl_nr_cand += nr;
>>       pgdat->nbp_th_nr_cand += nr;
>>       
>>       return true;
>> }
> 
> I don't think that it's necessary to make the algorithm harder to be
> understood.

All right,

> 
> If you think that the original stat really makes people confusing, I
> guess that we can add a new stat (say PGPROMOTE_CANDIDATE_OTHER).

Actually, I personally don't like to introduce a new stat for this case.
Anyway, we will further discuss this approach internally first.

Thank you !

Thanks
Zhijian


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