Re: [PATCH] mm/damon/core: remove unnecessary si_meminfo invoke.

[杨欢 <link@vivo.com>]

在 2023/9/19 7:26, SeongJae Park 写道:
> [Some people who received this message don't often get email from sj@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Huan,
>
> On Mon, 18 Sep 2023 12:12:01 +0000 杨欢 <link@vivo.com> wrote:
>
>> 在 2023/9/18 20:08, 杨欢 写道:
>>> 在 2023/9/18 19:11, SeongJae Park 写道:
>>>> Hi Huan,
>>>>
>>>> On Mon, 18 Sep 2023 17:49:34 +0800 Huan Yang <link@vivo.com> wrote:
>>>>
>>>>> si_meminfo() will read and assign more info not just free/ram pages.
>>>> Nice catch :)
>>>>
>>>>> For just DAMOS_WMARK_FREE_MEM_RATE use, only get free and ram pages
>>>>> is ok to save cpu.
>>>>>
>>>>> Signed-off-by: Huan Yang <link@vivo.com>
>>>>> ---
>>>>>     mm/damon/core.c | 10 ++++++----
>>>>>     1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>>>>> index bcd2bd9d6c10..1cddee9ae73b 100644
>>>>> --- a/mm/damon/core.c
>>>>> +++ b/mm/damon/core.c
>>>>> @@ -1278,14 +1278,16 @@ static bool kdamond_need_stop(struct damon_ctx *ctx)
>>>>>            return true;
>>>>>     }
>>>>>
>>>>> -static unsigned long damos_wmark_metric_value(enum damos_wmark_metric metric)
>>>>> +static unsigned long __damons_get_wmark_free_mem_rate(void)
>>>> Nit.  s/damons/damos/ would look more consistently, in my opinion?
>>> HI, SJ, sorry, what's this mean?
>> Haha, I get, yes, damos is better. If you agree with below, I will
>> resend a new, rename to
>>
>> __damos_get_wmark_free_mem_rate.
>>
>>>>>     {
>>>>> - struct sysinfo i;
>>>>> + return global_zone_page_state(NR_FREE_PAGES) * 1000 / totalram_pages();
>>>>> +}
>>>>>
>>>>> +static unsigned long damos_wmark_metric_value(enum damos_wmark_metric metric)
>>>>> +{
>>>>>            switch (metric) {
>>>>>            case DAMOS_WMARK_FREE_MEM_RATE:
>>>>> -         si_meminfo(&i);
>>>>> -         return i.freeram * 1000 / i.totalram;
>>>>> +         return __damons_get_wmark_free_mem_rate();
>>>> Since __damons_get_wmark_free_mem_rate() is just one line function and
>>>> damos_wmark_metric_value() is the only user of the code, I think we could just
>>>> writ the code here?
>>> I do this in mine first patch, but then, I fold this into
>>> "__damons_get_wmark_free_mem_rate"
>>>
>>> due to I think the "__damons_get_wmark_free_mem_rate" may change the
>>> meaning for furture,
>>>
>>> and may si_meminfo will come back soon?(If we need more info to get the
>>> rate?). And, also, the
>>>
>>> static function If just some user use, it will be inline, so, I just
>>> think fold it will be better.
>>>
>>> Do you think so?
> Unfortunately I don't think so.  What would be the future use case that would
> require changing the meaning of the metric?  I cannot imagine those off the top
Maybe care about [min, low, high] watermark? Or someting. But,
> of my head.  Even if such use case is found, such change would be a
> user-visible behavioral change, which we would like to avoid.  If such change
> is really needed, I think we would keep the current metric as is and create an
> alternative metric that having the new meaning.  Anyway, we can think about
> such case when it really happened.
Yes, you are right, if need a new case, just create an alternative metric.
>
> Also, the current code is doing the calculation in damos_wmark_metric_value().
> If there is no specific reason to split the logic out to a new function, I'd
> prefer keeping the overall structure as similar as is now.
>
> Please let me know if I'm missing something.

No sure reason to split it into function, keep it in 
damos_wmark_metric_value() is

better.


I'll send new patch.


Thanks,

Huan

>
>
> Thanks,
> SJ
>
>>> Thanks,
>>>
>>> Huan
>>>
>>>>>            default:
>>>>>                    break;
>>>>>            }
>>>>> --
>>>>> 2.34.1
>>>> Thanks,
>>>> SJ
>>