在 2023/9/19 7:26, SeongJae Park 写道: > [Some people who received this message don't often get email from sj@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > Hi Huan, > > On Mon, 18 Sep 2023 12:12:01 +0000 杨欢 <link@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxx> >>>>> --- >>>>> 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 >>
