In lru_sort.c and reclaim.c, they are all define get_monitoring_region()
function, there is no need to define it separately.
BTW, this patch remove tow struct 'damon_lru_sort_ram_walk_arg' and
'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
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;
}
Signed-off-by: Xin Hao <[email protected]>
---
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;
+}
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);
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
Hi Xin,
> In lru_sort.c and reclaim.c, they are all define get_monitoring_region()
s/define/defining/
> 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/
> '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/
> 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/[email protected]/
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.
>
> Signed-off-by: Xin Hao <[email protected]>
> ---
> 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_'?
> 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
On Wed, 7 Sep 2022 17:27:12 +0000 SeongJae Park <[email protected]> wrote:
[...]
> > 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'.
Or, putting it in damon/core.c might make more sense.
Thanks,
SJ
[...]
Hi Xin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.0-rc4 next-20220907]
[cannot apply to akpm-mm/mm-everything]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xin-Hao/mm-damon-Remove-duplicate-get_monitoring_region-definitions/20220907-193059
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0066f1b0e27556381402db3ff31f85d2a2265858
config: hexagon-randconfig-r041-20220907 (https://download.01.org/0day-ci/archive/20220908/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c55b41d5199d2394dd6cdb8f52180d8b81d809d4)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/28cd0cc7549075eccc95a2c48fbe3bc58f17caee
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Xin-Hao/mm-damon-Remove-duplicate-get_monitoring_region-definitions/20220907-193059
git checkout 28cd0cc7549075eccc95a2c48fbe3bc58f17caee
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash mm/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from mm/damon/reclaim.c:16:
>> mm/damon/ops-common.h:13:23: error: unknown type name 'pmd_t'
void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr);
^
1 error generated.
vim +/pmd_t +13 mm/damon/ops-common.h
46c3a0accdc48c mm/damon/prmtv-common.h SeongJae Park 2021-11-05 11
46c3a0accdc48c mm/damon/prmtv-common.h SeongJae Park 2021-11-05 12 void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr);
46c3a0accdc48c mm/damon/prmtv-common.h SeongJae Park 2021-11-05 @13 void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr);
198f0f4c58b9f4 mm/damon/prmtv-common.h SeongJae Park 2021-11-05 14
--
0-DAY CI Kernel Test Service
https://01.org/lkp
在 2022/9/8 上午4:59, SeongJae Park 写道:
> On Wed, 7 Sep 2022 17:27:12 +0000 SeongJae Park <[email protected]> wrote:
>
> [...]
>>> 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'.
> Or, putting it in damon/core.c might make more sense.
Ok, i will fix them in my next patch, thanks.
>
>
> Thanks,
> SJ
>
> [...]