[PATCH V2 0/4] mm/damon: Do some small changes

[PATCH V2 0/4] mm/damon: Do some small changes

[Xin Hao]

These four patches are mainly to do some small changes.

V1 -> V2
	Add reviewed by SeongJae Park
	Add two new patches
V1:
https://lore.kernel.org/linux-mm/cover.1636546262.git.xhao@linux.alibaba.com/


Xin Hao (4):
  mm/damon: Unified access_check function naming rules
  mm/damon: Add 'age' of region tracepoint support
  mm/damon/core: Using function abs() instead of diff_of()
  mm/damon: Remove some no need func definitions in damon.h file

 include/linux/damon.h        | 25 ++-----------------------
 include/trace/events/damon.h |  7 +++++--
 mm/damon/core.c              |  6 ++----
 mm/damon/vaddr.c             |  8 ++++----
 4 files changed, 13 insertions(+), 33 deletions(-)

--
2.31.0

[PATCH V2 1/4] mm/damon: Unified access_check function naming rules

[Xin Hao]

In damon/paddr.c file, two functions names start with underscore,
	static void __damon_pa_prepare_access_check(struct damon_ctx *ctx,
			struct damon_region *r)
	static void __damon_pa_prepare_access_check(struct damon_ctx *ctx,
			struct damon_region *r)
In damon/vaddr.c file, there are also two functions with the same function,
	static void damon_va_prepare_access_check(struct damon_ctx *ctx,
			struct mm_struct *mm, struct damon_region *r)
	static void damon_va_check_access(struct damon_ctx *ctx,
			struct mm_struct *mm, struct damon_region *r)

It makes sense to keep consistent, and it is not easy to be confused with
the function that call them.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
Reviewed-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/vaddr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index 35fe49080ee9..905e0fc8a8ec 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -409,7 +409,7 @@ static void damon_va_mkold(struct mm_struct *mm, unsigned long addr)
  * Functions for the access checking of the regions
  */

-static void damon_va_prepare_access_check(struct damon_ctx *ctx,
+static void __damon_va_prepare_access_check(struct damon_ctx *ctx,
 			struct mm_struct *mm, struct damon_region *r)
 {
 	r->sampling_addr = damon_rand(r->ar.start, r->ar.end);
@@ -428,7 +428,7 @@ void damon_va_prepare_access_checks(struct damon_ctx *ctx)
 		if (!mm)
 			continue;
 		damon_for_each_region(r, t)
-			damon_va_prepare_access_check(ctx, mm, r);
+			__damon_va_prepare_access_check(ctx, mm, r);
 		mmput(mm);
 	}
 }
@@ -514,7 +514,7 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
  * mm	'mm_struct' for the given virtual address space
  * r	the region to be checked
  */
-static void damon_va_check_access(struct damon_ctx *ctx,
+static void __damon_va_check_access(struct damon_ctx *ctx,
 			       struct mm_struct *mm, struct damon_region *r)
 {
 	static struct mm_struct *last_mm;
@@ -550,7 +550,7 @@ unsigned int damon_va_check_accesses(struct damon_ctx *ctx)
 		if (!mm)
 			continue;
 		damon_for_each_region(r, t) {
-			damon_va_check_access(ctx, mm, r);
+			__damon_va_check_access(ctx, mm, r);
 			max_nr_accesses = max(r->nr_accesses, max_nr_accesses);
 		}
 		mmput(mm);
--
2.31.0

[PATCH V2 2/4] mm/damon: Add 'age' of region tracepoint support

[Xin Hao]

In patch "mm/damon: add a tracepoint", it adds a
tracepoint for DAMON, it can monitor each region
for each aggregation interval, Now the region add
a new 'age' variable, some primitive would calculate
the priority of each region as a weight, there put it
into tracepoint, so we can easily track the change of
its value through perf or damon-tools.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 include/trace/events/damon.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
index 2f422f4f1fb9..99ffa601e351 100644
--- a/include/trace/events/damon.h
+++ b/include/trace/events/damon.h
@@ -22,6 +22,7 @@ TRACE_EVENT(damon_aggregated,
 		__field(unsigned long, start)
 		__field(unsigned long, end)
 		__field(unsigned int, nr_accesses)
+		__field(unsigned int, age)
 	),

 	TP_fast_assign(
@@ -30,11 +31,13 @@ TRACE_EVENT(damon_aggregated,
 		__entry->start = r->ar.start;
 		__entry->end = r->ar.end;
 		__entry->nr_accesses = r->nr_accesses;
+		__entry->age = r->age;
 	),

-	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u",
+	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
 			__entry->target_id, __entry->nr_regions,
-			__entry->start, __entry->end, __entry->nr_accesses)
+			__entry->start, __entry->end,
+			__entry->nr_accesses, __entry->age)
 );

 #endif /* _TRACE_DAMON_H */
--
2.31.0

[PATCH V2 3/4] mm/damon/core: Using function abs() instead of diff_of()

[Xin Hao]

In kernel, we can use abs(a - b) to get the absolute value,
So there is no need to redefine a new one.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/damon/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index f37c17b53814..4d2c3a0c7c8a 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -757,8 +757,6 @@ static void damon_merge_two_regions(struct damon_target *t,
 	damon_destroy_region(r, t);
 }

-#define diff_of(a, b) (a > b ? a - b : b - a)
-
 /*
  * Merge adjacent regions having similar access frequencies
  *
@@ -772,13 +770,13 @@ static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
 	struct damon_region *r, *prev = NULL, *next;

 	damon_for_each_region_safe(r, next, t) {
-		if (diff_of(r->nr_accesses, r->last_nr_accesses) > thres)
+		if (abs(r->nr_accesses - r->last_nr_accesses) > thres)
 			r->age = 0;
 		else
 			r->age++;

 		if (prev && prev->ar.end == r->ar.start &&
-		    diff_of(prev->nr_accesses, r->nr_accesses) <= thres &&
+		    abs(prev->nr_accesses - r->nr_accesses) <= thres &&
 		    sz_damon_region(prev) + sz_damon_region(r) <= sz_limit)
 			damon_merge_two_regions(t, prev, r);
 		else
--
2.31.0

[PATCH V2 4/4] mm/damon: Remove some no need func definitions in damon.h file

[Xin Hao]

In the damon.h header file, some func definitions about VA & PA
can only be used in its own file, so there no need to define in
the header file, and the header file will looks cleaner.

If other files later call these functions, then put them to the
header file will not be late.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 include/linux/damon.h | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 321de9d72360..8a73e825e0d5 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -461,34 +461,13 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
 #endif	/* CONFIG_DAMON */

 #ifdef CONFIG_DAMON_VADDR
-
-/* Monitoring primitives for virtual memory address spaces */
-void damon_va_init(struct damon_ctx *ctx);
-void damon_va_update(struct damon_ctx *ctx);
-void damon_va_prepare_access_checks(struct damon_ctx *ctx);
-unsigned int damon_va_check_accesses(struct damon_ctx *ctx);
-bool damon_va_target_valid(void *t);
-void damon_va_cleanup(struct damon_ctx *ctx);
-int damon_va_apply_scheme(struct damon_ctx *context, struct damon_target *t,
-		struct damon_region *r, struct damos *scheme);
-int damon_va_scheme_score(struct damon_ctx *context, struct damon_target *t,
-		struct damon_region *r, struct damos *scheme);
 void damon_va_set_primitives(struct damon_ctx *ctx);
-
+bool damon_va_target_valid(void *t);
 #endif	/* CONFIG_DAMON_VADDR */

 #ifdef CONFIG_DAMON_PADDR
-
-/* Monitoring primitives for the physical memory address space */
-void damon_pa_prepare_access_checks(struct damon_ctx *ctx);
-unsigned int damon_pa_check_accesses(struct damon_ctx *ctx);
-bool damon_pa_target_valid(void *t);
-int damon_pa_apply_scheme(struct damon_ctx *context, struct damon_target *t,
-		struct damon_region *r, struct damos *scheme);
-int damon_pa_scheme_score(struct damon_ctx *context, struct damon_target *t,
-		struct damon_region *r, struct damos *scheme);
 void damon_pa_set_primitives(struct damon_ctx *ctx);
-
+bool damon_pa_target_valid(void *t);
 #endif	/* CONFIG_DAMON_PADDR */

 #endif	/* _DAMON_H */
--
2.31.0

Re: [PATCH V2 0/4] mm/damon: Do some small changes

[SeongJae Park]

Hello Xin,

On Thu, 11 Nov 2021 14:07:00 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> These four patches are mainly to do some small changes.
> 
> V1 -> V2
> 	Add reviewed by SeongJae Park
> 	Add two new patches
> V1:
> https://lore.kernel.org/linux-mm/cover.1636546262.git.xhao@linux.alibaba.com/
> 
> 
> Xin Hao (4):
>   mm/damon: Unified access_check function naming rules
>   mm/damon: Add 'age' of region tracepoint support
>   mm/damon/core: Using function abs() instead of diff_of()
>   mm/damon: Remove some no need func definitions in damon.h file

Overall, all patches looks good to me, though I asked[1] a trivial change in
the commit message of the second patch.

Also, I found one interesting thing.  It seems you are wrapping body of the
commit messages at <75 columns[2]?  That's obviously neither a problem, nor
even trivial nit.  But... I'd prefer the messages look more consistent with
others.

[1] https://lore.kernel.org/linux-mm/20211111082034.13323-1-sj@kernel.org/
[2] https://docs.kernel.org/process/submitting-patches.html?highlight=75#the-canonical-patch-format


Thanks,
SJ

> 
>  include/linux/damon.h        | 25 ++-----------------------
>  include/trace/events/damon.h |  7 +++++--
>  mm/damon/core.c              |  6 ++----
>  mm/damon/vaddr.c             |  8 ++++----
>  4 files changed, 13 insertions(+), 33 deletions(-)
> 
> --
> 2.31.0

Re: [PATCH V2 0/4] mm/damon: Do some small changes

[Xin Hao]

Hi, Park:

On 2021/11/11 下午5:24, SeongJae Park wrote:
> Hello Xin,
>
> On Thu, 11 Nov 2021 14:07:00 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> These four patches are mainly to do some small changes.
>>
>> V1 -> V2
>> 	Add reviewed by SeongJae Park
>> 	Add two new patches
>> V1:
>> https://lore.kernel.org/linux-mm/cover.1636546262.git.xhao@linux.alibaba.com/
>>
>>
>> Xin Hao (4):
>>    mm/damon: Unified access_check function naming rules
>>    mm/damon: Add 'age' of region tracepoint support
>>    mm/damon/core: Using function abs() instead of diff_of()
>>    mm/damon: Remove some no need func definitions in damon.h file
> Overall, all patches looks good to me, though I asked[1] a trivial change in
> the commit message of the second patch.
Ok,  I will add a detail message to explain it in commit in my next 
patch, thanks.
>
> Also, I found one interesting thing.  It seems you are wrapping body of the
> commit messages at <75 columns[2]?  That's obviously neither a problem, nor
> even trivial nit.  But... I'd prefer the messages look more consistent with
> others.
Thank you very much for your careful correction,  I will fix it in my 
next patch too.
>
> [1] https://lore.kernel.org/linux-mm/20211111082034.13323-1-sj@kernel.org/
> [2] https://docs.kernel.org/process/submitting-patches.html?highlight=75#the-canonical-patch-format
>
>
> Thanks,
> SJ
>
>>   include/linux/damon.h        | 25 ++-----------------------
>>   include/trace/events/damon.h |  7 +++++--
>>   mm/damon/core.c              |  6 ++----
>>   mm/damon/vaddr.c             |  8 ++++----
>>   4 files changed, 13 insertions(+), 33 deletions(-)
>>
>> --
>> 2.31.0

-- 
Best Regards!
Xin Hao