From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 34FDDECAAD3 for ; Thu, 8 Sep 2022 01:55:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 974DF8D0001; Wed, 7 Sep 2022 21:55:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8FD9A6B0073; Wed, 7 Sep 2022 21:55:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 782228D0001; Wed, 7 Sep 2022 21:55:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 605076B0072 for ; Wed, 7 Sep 2022 21:55:23 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 295001601CC for ; Thu, 8 Sep 2022 01:55:23 +0000 (UTC) X-FDA: 79887250926.23.60A91D1 Received: from out30-56.freemail.mail.aliyun.com (out30-56.freemail.mail.aliyun.com [115.124.30.56]) by imf22.hostedemail.com (Postfix) with ESMTP id AE5B8C0085 for ; Thu, 8 Sep 2022 01:55:20 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R361e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018045192;MF=xhao@linux.alibaba.com;NM=1;PH=DS;RN=5;SR=0;TI=SMTPD_---0VP0CYNd_1662602116; Received: from 30.240.97.117(mailfrom:xhao@linux.alibaba.com fp:SMTPD_---0VP0CYNd_1662602116) by smtp.aliyun-inc.com; Thu, 08 Sep 2022 09:55:17 +0800 Content-Type: multipart/alternative; boundary="------------SSGeW1mIXZXJ7V7ZefHdNx99" Message-ID: Date: Thu, 8 Sep 2022 09:55:15 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.1.2 Subject: Re: [PATCH] mm/damon: Remove duplicate get_monitoring_region() definitions To: SeongJae Park Cc: akpm@linux-foundation.org, damon@lists.linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20220907172712.61006-1-sj@kernel.org> From: haoxin In-Reply-To: <20220907172712.61006-1-sj@kernel.org> ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=none; spf=pass (imf22.hostedemail.com: domain of xhao@linux.alibaba.com designates 115.124.30.56 as permitted sender) smtp.mailfrom=xhao@linux.alibaba.com; dmarc=pass (policy=none) header.from=alibaba.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662602122; a=rsa-sha256; cv=none; b=T2R//OqysaLm6fOb3MHURT6MlCXkTm/WaAJGFBoq1VZLrVBJaNAUeqVaWcJYdhPoM4cC0K m6rRJiLJ1jruDKGlzhvnafbeH7l6a66Xwpw7h+vkYntdyTV3FkWkT4PhZ/Od1CbYbdxrnq Q1Lbgy4QIcFjZ5cP24sRxcMetf1MTnc= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662602122; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/uw2FQpiTdUBeQ02VPvNN7j6Lpt85Bsa8awqo10o8m8=; b=evkowQF3pR4T7yDA6krTxSKho62H2qqcf34iWuyY03z05iler/FG2smX/GTgCaR+Pr9Zop eDJ9dzfQQShoHiNoDs38SbUXLPtG3Tb6WL/5akgHKSJSWMzk+77+wuLl/uThNdm2gH4vAU mN/ZnSHog1mqzJ/rHDaT8SoVY6n5ngA= X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: AE5B8C0085 Authentication-Results: imf22.hostedemail.com; dkim=none; spf=pass (imf22.hostedemail.com: domain of xhao@linux.alibaba.com designates 115.124.30.56 as permitted sender) smtp.mailfrom=xhao@linux.alibaba.com; dmarc=pass (policy=none) header.from=alibaba.com X-Stat-Signature: 5pd1y8xrt5hjofcig3w8mb85q3hdnqa1 X-Rspam-User: X-HE-Tag: 1662602120-879215 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: This is a multi-part message in MIME format. --------------SSGeW1mIXZXJ7V7ZefHdNx99 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 在 2022/9/8 上午1:27, SeongJae Park 写道: > Hi Xin, > >> In lru_sort.c and reclaim.c, they are all define get_monitoring_region() > s/define/defining/ Done > >> function, there is no need to define it separately. > Good point! > >> BTW, this patch remove tow struct 'damon_lru_sort_ram_walk_arg' and > s/remove/removes/ > s/tow/two/ Sorry，done > >> 'damon_reclaim_ram_walk_arg', though the two struct are removed, if we >> want to add more fields to these struct for other purposes later, it will > s/struct/structs/ Done > >> not too late for us to use them again. >> For example: >> struct damon_reclaim_ram_walk_arg { >> struct damon_addr_range raw_walk; >> xxx A; >> xxx B; >> } >> struct damon_lru_sort_ram_walk_arg { >> struct damon_addr_range raw_walk; >> xxx A; >> xxx B; >> } > I haven't read the below part yet, but seems you're gonna use 'struct > damon_addr_range' instead of 'struct damon_{reclai,lru_sort}_ram_walk_arg'. > > And I think we have already discussed about this before: > https://lore.kernel.org/damon/20220818172322.51705-1-sj@kernel.org/ > > My point is, we might add some more fileds to 'struct damon_addr_range' in a > future, though it seems very unlikely at the moment. Sorry for not making my > opinion clear enough. Ok, my fault that not get what you mean,  but we can add new common struct to set more fileds in the future. >> Signed-off-by: Xin Hao >> --- >> mm/damon/lru_sort.c | 35 ++--------------------------------- >> mm/damon/ops-common.c | 28 ++++++++++++++++++++++++++++ >> mm/damon/ops-common.h | 1 + >> mm/damon/reclaim.c | 35 ++--------------------------------- >> 4 files changed, 33 insertions(+), 66 deletions(-) >> >> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c >> index 9de6f00a71c5..5032d59d46e4 100644 >> --- a/mm/damon/lru_sort.c >> +++ b/mm/damon/lru_sort.c >> @@ -13,6 +13,8 @@ >> #include >> #include >> >> +#include "ops-common.h" >> + >> #ifdef MODULE_PARAM_PREFIX >> #undef MODULE_PARAM_PREFIX >> #endif >> @@ -257,39 +259,6 @@ module_param(nr_cold_quota_exceeds, ulong, 0400); >> static struct damon_ctx *ctx; >> static struct damon_target *target; >> >> -struct damon_lru_sort_ram_walk_arg { >> - unsigned long start; >> - unsigned long end; >> -}; >> - >> -static int walk_system_ram(struct resource *res, void *arg) >> -{ >> - struct damon_lru_sort_ram_walk_arg *a = arg; >> - >> - if (a->end - a->start < resource_size(res)) { >> - a->start = res->start; >> - a->end = res->end; >> - } >> - return 0; >> -} >> - >> -/* >> - * Find biggest 'System RAM' resource and store its start and end address in >> - * @start and @end, respectively. If no System RAM is found, returns false. >> - */ >> -static bool get_monitoring_region(unsigned long *start, unsigned long *end) >> -{ >> - struct damon_lru_sort_ram_walk_arg arg = {}; >> - >> - walk_system_ram_res(0, ULONG_MAX, &arg, walk_system_ram); >> - if (arg.end <= arg.start) >> - return false; >> - >> - *start = arg.start; >> - *end = arg.end; >> - return true; >> -} >> - >> /* Create a DAMON-based operation scheme for hot memory regions */ >> static struct damos *damon_lru_sort_new_hot_scheme(unsigned int hot_thres) >> { >> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c >> index b1335de200e7..01938f33038d 100644 >> --- a/mm/damon/ops-common.c >> +++ b/mm/damon/ops-common.c >> @@ -172,3 +172,31 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r, >> >> return hotness; >> } >> + >> +static inline int walk_system_ram(struct resource *res, void *arg) >> +{ >> + struct damon_addr_range *a = arg; >> + >> + if (a->end - a->start < resource_size(res)) { >> + a->start = res->start; >> + a->end = res->end; >> + } >> + return 0; >> +} >> + >> +/* >> + * Find biggest 'System RAM' resource and store its start and end address in >> + * @start and @end, respectively. If no System RAM is found, returns false. >> + */ >> +bool get_monitoring_region(unsigned long *start, unsigned long *end) >> +{ >> + struct damon_addr_range arg = {}; >> + >> + walk_system_ram_res(0, ULONG_MAX, &arg, walk_system_ram); >> + if (arg.end <= arg.start) >> + return false; >> + >> + *start = arg.start; >> + *end = arg.end; >> + return true; >> +} > 'ops-common.c' is for code that common in monitoring operations > implementations. I'd prefer to have yet another source file for the DAMON > modules including reclaim and lru_sort, say, 'modules-common.c'. > > Also, as 'get_monitoring_region()' is not a 'static' function anymore, let's > have a prefix to distinguish with other functions, say, 'damon_modules_'? Good idea,  i will rename the function in core.c as you suggested. > >> diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h >> index 52329ff361cd..e6f1c9b48042 100644 >> --- a/mm/damon/ops-common.h >> +++ b/mm/damon/ops-common.h >> @@ -16,3 +16,4 @@ int damon_pageout_score(struct damon_ctx *c, struct damon_region *r, >> struct damos *s); >> int damon_hot_score(struct damon_ctx *c, struct damon_region *r, >> struct damos *s); >> +bool get_monitoring_region(unsigned long *start, unsigned long *end); > Let's move it to a dedicated source file, say, 'modules-common.h' > >> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c >> index a7faf51b4bd4..20e83eee3c7d 100644 >> --- a/mm/damon/reclaim.c >> +++ b/mm/damon/reclaim.c >> @@ -13,6 +13,8 @@ >> #include >> #include >> >> +#include "ops-common.h" >> + >> #ifdef MODULE_PARAM_PREFIX >> #undef MODULE_PARAM_PREFIX >> #endif >> @@ -229,39 +231,6 @@ module_param(nr_quota_exceeds, ulong, 0400); >> static struct damon_ctx *ctx; >> static struct damon_target *target; >> >> -struct damon_reclaim_ram_walk_arg { >> - unsigned long start; >> - unsigned long end; >> -}; >> - >> -static int walk_system_ram(struct resource *res, void *arg) >> -{ >> - struct damon_reclaim_ram_walk_arg *a = arg; >> - >> - if (a->end - a->start < resource_size(res)) { >> - a->start = res->start; >> - a->end = res->end; >> - } >> - return 0; >> -} >> - >> -/* >> - * Find biggest 'System RAM' resource and store its start and end address in >> - * @start and @end, respectively. If no System RAM is found, returns false. >> - */ >> -static bool get_monitoring_region(unsigned long *start, unsigned long *end) >> -{ >> - struct damon_reclaim_ram_walk_arg arg = {}; >> - >> - walk_system_ram_res(0, ULONG_MAX, &arg, walk_system_ram); >> - if (arg.end <= arg.start) >> - return false; >> - >> - *start = arg.start; >> - *end = arg.end; >> - return true; >> -} >> - >> static struct damos *damon_reclaim_new_scheme(void) >> { >> struct damos_watermarks wmarks = { >> -- >> 2.31.0 > > Thanks, > SJ --------------SSGeW1mIXZXJ7V7ZefHdNx99 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit


在 2022/9/8 上午1:27, SeongJae Park 写道:
Hi Xin,


In lru_sort.c and reclaim.c, they are all define get_monitoring_region()

s/define/defining/
Done


function, there is no need to define it separately.

Good point!


BTW, this patch remove tow struct 'damon_lru_sort_ram_walk_arg' and

s/remove/removes/
s/tow/two/
Sorry，done


'damon_reclaim_ram_walk_arg', though the two struct are removed, if we
want to add more fields to these struct for other purposes later, it will

s/struct/structs/
Done


not too late for us to use them again.
For example:
    struct damon_reclaim_ram_walk_arg {
	struct damon_addr_range raw_walk;
	xxx  A;
	xxx  B;
    }
    struct damon_lru_sort_ram_walk_arg {
	struct damon_addr_range raw_walk;
	xxx  A;
	xxx  B;
    }

I haven't read the below part yet, but seems you're gonna use 'struct
damon_addr_range' instead of 'struct damon_{reclai,lru_sort}_ram_walk_arg'.

And I think we have already discussed about this before:
https://lore.kernel.org/damon/20220818172322.51705-1-sj@kernel.org/

My point is, we might add some more fileds to 'struct damon_addr_range' in a
future, though it seems very unlikely at the moment.  Sorry for not making my
opinion clear enough.
Ok, my fault that not get what you mean,  but we can add new common struct to set more fileds in the future.

      

        
Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/damon/lru_sort.c   | 35 ++---------------------------------
 mm/damon/ops-common.c | 28 ++++++++++++++++++++++++++++
 mm/damon/ops-common.h |  1 +
 mm/damon/reclaim.c    | 35 ++---------------------------------
 4 files changed, 33 insertions(+), 66 deletions(-)

diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 9de6f00a71c5..5032d59d46e4 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -13,6 +13,8 @@
 #include <linux/sched.h>
 #include <linux/workqueue.h>

+#include "ops-common.h"
+
 #ifdef MODULE_PARAM_PREFIX
 #undef MODULE_PARAM_PREFIX
 #endif
@@ -257,39 +259,6 @@ module_param(nr_cold_quota_exceeds, ulong, 0400);
 static struct damon_ctx *ctx;
 static struct damon_target *target;

-struct damon_lru_sort_ram_walk_arg {
-	unsigned long start;
-	unsigned long end;
-};
-
-static int walk_system_ram(struct resource *res, void *arg)
-{
-	struct damon_lru_sort_ram_walk_arg *a = arg;
-
-	if (a->end - a->start < resource_size(res)) {
-		a->start = res->start;
-		a->end = res->end;
-	}
-	return 0;
-}
-
-/*
- * Find biggest 'System RAM' resource and store its start and end address in
- * @start and @end, respectively.  If no System RAM is found, returns false.
- */
-static bool get_monitoring_region(unsigned long *start, unsigned long *end)
-{
-	struct damon_lru_sort_ram_walk_arg arg = {};
-
-	walk_system_ram_res(0, ULONG_MAX, &arg, walk_system_ram);
-	if (arg.end <= arg.start)
-		return false;
-
-	*start = arg.start;
-	*end = arg.end;
-	return true;
-}
-
 /* Create a DAMON-based operation scheme for hot memory regions */
 static struct damos *damon_lru_sort_new_hot_scheme(unsigned int hot_thres)
 {
diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index b1335de200e7..01938f33038d 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -172,3 +172,31 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,

 	return hotness;
 }
+
+static inline int walk_system_ram(struct resource *res, void *arg)
+{
+	struct damon_addr_range *a = arg;
+
+	if (a->end - a->start < resource_size(res)) {
+		a->start = res->start;
+		a->end = res->end;
+	}
+	return 0;
+}
+
+/*
+ * Find biggest 'System RAM' resource and store its start and end address in
+ * @start and @end, respectively.  If no System RAM is found, returns false.
+ */
+bool get_monitoring_region(unsigned long *start, unsigned long *end)
+{
+	struct damon_addr_range arg = {};
+
+	walk_system_ram_res(0, ULONG_MAX, &arg, walk_system_ram);
+	if (arg.end <= arg.start)
+		return false;
+
+	*start = arg.start;
+	*end = arg.end;
+	return true;
+}


      

      
'ops-common.c' is for code that common in monitoring operations
implementations.  I'd prefer to have yet another source file for the DAMON
modules including reclaim and lru_sort, say, 'modules-common.c'.

Also, as 'get_monitoring_region()' is not a 'static' function anymore, let's
have a prefix to distinguish with other functions, say, 'damon_modules_'?

    

    Good idea,  i will rename the function in core.c as you suggested.
    

      



      

        
diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h
index 52329ff361cd..e6f1c9b48042 100644
--- a/mm/damon/ops-common.h
+++ b/mm/damon/ops-common.h
@@ -16,3 +16,4 @@ int damon_pageout_score(struct damon_ctx *c, struct damon_region *r,
 			struct damos *s);
 int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
 			struct damos *s);
+bool get_monitoring_region(unsigned long *start, unsigned long *end);


      

      
Let's move it to a dedicated source file, say, 'modules-common.h'



      

        
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index a7faf51b4bd4..20e83eee3c7d 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -13,6 +13,8 @@
 #include <linux/sched.h>
 #include <linux/workqueue.h>

+#include "ops-common.h"
+
 #ifdef MODULE_PARAM_PREFIX
 #undef MODULE_PARAM_PREFIX
 #endif
@@ -229,39 +231,6 @@ module_param(nr_quota_exceeds, ulong, 0400);
 static struct damon_ctx *ctx;
 static struct damon_target *target;

-struct damon_reclaim_ram_walk_arg {
-	unsigned long start;
-	unsigned long end;
-};
-
-static int walk_system_ram(struct resource *res, void *arg)
-{
-	struct damon_reclaim_ram_walk_arg *a = arg;
-
-	if (a->end - a->start < resource_size(res)) {
-		a->start = res->start;
-		a->end = res->end;
-	}
-	return 0;
-}
-
-/*
- * Find biggest 'System RAM' resource and store its start and end address in
- * @start and @end, respectively.  If no System RAM is found, returns false.
- */
-static bool get_monitoring_region(unsigned long *start, unsigned long *end)
-{
-	struct damon_reclaim_ram_walk_arg arg = {};
-
-	walk_system_ram_res(0, ULONG_MAX, &arg, walk_system_ram);
-	if (arg.end <= arg.start)
-		return false;
-
-	*start = arg.start;
-	*end = arg.end;
-	return true;
-}
-
 static struct damos *damon_reclaim_new_scheme(void)
 {
 	struct damos_watermarks wmarks = {
--
2.31.0


      

      

Thanks,
SJ


    

  


--------------SSGeW1mIXZXJ7V7ZefHdNx99--