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

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

[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?

>  {
> -	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?

>  	default:
>  		break;
>  	}
> -- 
> 2.34.1


Thanks,
SJ

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

[杨欢]

在 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?
>
>>   {
>> -	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?

Thanks,

Huan

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

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

[杨欢]

在 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?
>
> Thanks,
>
> Huan
>
>>>    	default:
>>>    		break;
>>>    	}
>>> -- 
>>> 2.34.1
>> Thanks,
>> SJ
>

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

[SeongJae Park]

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

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.


Thanks,
SJ

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

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

[杨欢]

在 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
>>