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

["Huang, Ying" <ying.huang@linux.alibaba.com>]

"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.

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

---
Best Regards,
Huang, Ying