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

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

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;
}

Thanks
Zhijian

> 
> [snip]
> 
> ---
> Best Regards,
> Huang, Ying