[PATCH V1 0/2] mm/damon: Do little changes

[PATCH V1 0/2] mm/damon: Do little changes

[Xin Hao]

These patches are mainly used for fine adjustments.

Xin Hao (2):
  mm/damon/dbgfs: Avoid target_ids display wrong pid value
  mm/damon: Modify the display form of damon tracepoint

 include/trace/events/damon.h | 6 +++---
 mm/damon/dbgfs.c             | 8 +++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

--
2.31.0

[PATCH V1 1/2] mm/damon/dbgfs: Avoid target_ids display wrong pid value

[Xin Hao]

The return value of pid_vnr() is pid_t type which is defined by 'int', this may
get a wrong pid value, if we set the return value type as 'unsigned long'.
So there fix it.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/damon/dbgfs.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index 991b7fa0e858..97eebfb70ddf 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -281,17 +281,15 @@ static inline bool targetid_is_pid(const struct damon_ctx *ctx)
 static ssize_t sprint_target_ids(struct damon_ctx *ctx, char *buf, ssize_t len)
 {
 	struct damon_target *t;
-	unsigned long id;
-	int written = 0;
+	int id, written = 0;
 	int rc;
 
 	damon_for_each_target(t, ctx) {
-		id = t->id;
 		if (targetid_is_pid(ctx))
 			/* Show pid numbers to debugfs users */
-			id = (unsigned long)pid_vnr((struct pid *)id);
+			id = (int)pid_vnr((struct pid *)t->id);
 
-		rc = scnprintf(&buf[written], len - written, "%lu ", id);
+		rc = scnprintf(&buf[written], len - written, "%d ", id);
 		if (!rc)
 			return -ENOMEM;
 		written += rc;
-- 
2.31.0

[PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint

[Xin Hao]

When I use the perf command to record damon monitor data, like below.
    # perf record -e damon:damon_aggregated
    # perf script
    ...target_id=18446462667479739520 nr_regions=13 281472805928960-281472942936064...
    ...target_id=18446462667479739520 nr_regions=13 281472942936064-281473080008704...
    ...target_id=18446462667479739520 nr_regions=13 281473080008704-281473216634880...

From a user's point of view, the 'target_id' and 'damon_region' which displays in decimal
are not very friendly, So there do some changes, keep the 'target_id' display consistent
with 'dbgfs/target_ids' interface and 'damon_region' is displayed in hexadecimal, just like
below.
    # perf record -e damon:damon_aggregated
    # perf script
    ...target_id=5522 nr_regions=14 ffff716a3000-ffff79893000...
    ...target_id=5522 nr_regions=14 ffff79893000-ffff819dc000...
    ...target_id=5522 nr_regions=14 ffff819dc000-ffff89bd9000...

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

diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
index 99ffa601e351..67de51814f4c 100644
--- a/include/trace/events/damon.h
+++ b/include/trace/events/damon.h
@@ -17,7 +17,7 @@ TRACE_EVENT(damon_aggregated,
 	TP_ARGS(t, r, nr_regions),

 	TP_STRUCT__entry(
-		__field(unsigned long, target_id)
+		__field(int, target_id)
 		__field(unsigned int, nr_regions)
 		__field(unsigned long, start)
 		__field(unsigned long, end)
@@ -26,7 +26,7 @@ TRACE_EVENT(damon_aggregated,
 	),

 	TP_fast_assign(
-		__entry->target_id = t->id;
+		__entry->target_id = (int)pid_vnr((struct pid *)t->id);
 		__entry->nr_regions = nr_regions;
 		__entry->start = r->ar.start;
 		__entry->end = r->ar.end;
@@ -34,7 +34,7 @@ TRACE_EVENT(damon_aggregated,
 		__entry->age = r->age;
 	),

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

Re: [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint

[SeongJae Park]

On Fri, 10 Dec 2021 00:33:17 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> When I use the perf command to record damon monitor data, like below.
>     # perf record -e damon:damon_aggregated
>     # perf script
>     ...target_id=18446462667479739520 nr_regions=13 281472805928960-281472942936064...
>     ...target_id=18446462667479739520 nr_regions=13 281472942936064-281473080008704...
>     ...target_id=18446462667479739520 nr_regions=13 281473080008704-281473216634880...
> 
> From a user's point of view, the 'target_id' and 'damon_region' which displays in decimal
> are not very friendly, So there do some changes, keep the 'target_id' display consistent
> with 'dbgfs/target_ids' interface and 'damon_region' is displayed in hexadecimal, just like
> below.
>     # perf record -e damon:damon_aggregated
>     # perf script
>     ...target_id=5522 nr_regions=14 ffff716a3000-ffff79893000...
>     ...target_id=5522 nr_regions=14 ffff79893000-ffff819dc000...
>     ...target_id=5522 nr_regions=14 ffff819dc000-ffff89bd9000...
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  include/trace/events/damon.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> index 99ffa601e351..67de51814f4c 100644
> --- a/include/trace/events/damon.h
> +++ b/include/trace/events/damon.h
> @@ -17,7 +17,7 @@ TRACE_EVENT(damon_aggregated,
>  	TP_ARGS(t, r, nr_regions),
> 
>  	TP_STRUCT__entry(
> -		__field(unsigned long, target_id)
> +		__field(int, target_id)
>  		__field(unsigned int, nr_regions)
>  		__field(unsigned long, start)
>  		__field(unsigned long, end)
> @@ -26,7 +26,7 @@ TRACE_EVENT(damon_aggregated,
>  	),
> 
>  	TP_fast_assign(
> -		__entry->target_id = t->id;
> +		__entry->target_id = (int)pid_vnr((struct pid *)t->id);

I think this would break physical address space monitoring.  Have you tested
this change for that?


Thanks,
SJ

>  		__entry->nr_regions = nr_regions;
>  		__entry->start = r->ar.start;
>  		__entry->end = r->ar.end;
> @@ -34,7 +34,7 @@ TRACE_EVENT(damon_aggregated,
>  		__entry->age = r->age;
>  	),
> 
> -	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
> +	TP_printk("target_id=%u nr_regions=%u %lx-%lx: %u %u",
>  			__entry->target_id, __entry->nr_regions,
>  			__entry->start, __entry->end,
>  			__entry->nr_accesses, __entry->age)
> --
> 2.31.0

Re: [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint

[Xin Hao]

[-- Attachment #1: Type: text/plain, Size: 4038 bytes --]

Hi SeongJae：

On 12/10/21 12:46 AM, SeongJae Park wrote:
> On Fri, 10 Dec 2021 00:33:17 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> When I use the perf command to record damon monitor data, like below.
>>      # perf record -e damon:damon_aggregated
>>      # perf script
>>      ...target_id=18446462667479739520 nr_regions=13 281472805928960-281472942936064...
>>      ...target_id=18446462667479739520 nr_regions=13 281472942936064-281473080008704...
>>      ...target_id=18446462667479739520 nr_regions=13 281473080008704-281473216634880...
>>
>>  From a user's point of view, the 'target_id' and 'damon_region' which displays in decimal
>> are not very friendly, So there do some changes, keep the 'target_id' display consistent
>> with 'dbgfs/target_ids' interface and 'damon_region' is displayed in hexadecimal, just like
>> below.
>>      # perf record -e damon:damon_aggregated
>>      # perf script
>>      ...target_id=5522 nr_regions=14 ffff716a3000-ffff79893000...
>>      ...target_id=5522 nr_regions=14 ffff79893000-ffff819dc000...
>>      ...target_id=5522 nr_regions=14 ffff819dc000-ffff89bd9000...
>>
>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>> ---
>>   include/trace/events/damon.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
>> index 99ffa601e351..67de51814f4c 100644
>> --- a/include/trace/events/damon.h
>> +++ b/include/trace/events/damon.h
>> @@ -17,7 +17,7 @@ TRACE_EVENT(damon_aggregated,
>>   	TP_ARGS(t, r, nr_regions),
>>
>>   	TP_STRUCT__entry(
>> -		__field(unsigned long, target_id)
>> +		__field(int, target_id)
>>   		__field(unsigned int, nr_regions)
>>   		__field(unsigned long, start)
>>   		__field(unsigned long, end)
>> @@ -26,7 +26,7 @@ TRACE_EVENT(damon_aggregated,
>>   	),
>>
>>   	TP_fast_assign(
>> -		__entry->target_id = t->id;
>> +		__entry->target_id = (int)pid_vnr((struct pid *)t->id);
> I think this would break physical address space monitoring.  Have you tested
> this change for that?

Yes, you are right,  But I encountered some problems while testing 
physical address, it seems that my operation did not work

I did some test like this:

     # echo "42 0x0000000840000000 0x000000103fffffff" > init_regions

     # echo paddr > target_ids

     # echo on > monitor_on

i get the physical address from my kernel startup log.

15 [ 0.000000] Early memory node ranges
16 [ 0.000000] node 0: [mem 0x0000000040000000-0x000000083bc7ffff]
17 [ 0.000000] node 0: [mem 0x000000083bc80000-0x000000083bffffff]
18 [ 0.000000] node 0: [mem 0x000000083c000000-0x000000083c03ffff]
19 [ 0.000000] node 0: [mem 0x000000083c040000-0x000000083c0fffff]
20 [ 0.000000] node 0: [mem 0x000000083c100000-0x000000083f3dffff]
21 [ 0.000000] node 0: [mem 0x000000083f3e0000-0x000000083f46ffff]
22 [ 0.000000] node 0: [mem 0x000000083f470000-0x000000083f47ffff]
23 [ 0.000000] node 0: [mem 0x000000083f480000-0x000000083f59ffff]
24 [ 0.000000] node 0: [mem 0x000000083f5a0000-0x000000083fffffff]
25 [ 0.000000] node 1: [mem 0x0000000840000000-0x000000103fffffff]
26 [ 0.000000] node 2: [mem 0x0000001040000000-0x000000183fffffff]
27 [ 0.000000] node 3: [mem 0x0000001840000000-0x000000203fffffff]
28 [ 0.000000] Initmem setup node 0 [mem 
0x0000000040000000-0x000000083fffffff]
29 [ 0.000000] On node 0 totalpages: 8388608

Is there anything wrong ?

>
>
> Thanks,
> SJ
>
>>   		__entry->nr_regions = nr_regions;
>>   		__entry->start = r->ar.start;
>>   		__entry->end = r->ar.end;
>> @@ -34,7 +34,7 @@ TRACE_EVENT(damon_aggregated,
>>   		__entry->age = r->age;
>>   	),
>>
>> -	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
>> +	TP_printk("target_id=%u nr_regions=%u %lx-%lx: %u %u",
>>   			__entry->target_id, __entry->nr_regions,
>>   			__entry->start, __entry->end,
>>   			__entry->nr_accesses, __entry->age)
>> --
>> 2.31.0

-- 
Best Regards!
Xin Hao


[-- Attachment #2: Type: text/html, Size: 24913 bytes --]

Re: [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint

[SeongJae Park]

Hi Xin,

On Fri, 10 Dec 2021 11:36:23 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> [-- Attachment #1: Type: text/plain, Size: 4038 bytes --]
> 
> Hi SeongJae：
> 
> On 12/10/21 12:46 AM, SeongJae Park wrote:
> > On Fri, 10 Dec 2021 00:33:17 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
> >
> >> When I use the perf command to record damon monitor data, like below.
> >>      # perf record -e damon:damon_aggregated
> >>      # perf script
> >>      ...target_id=18446462667479739520 nr_regions=13 281472805928960-281472942936064...
> >>      ...target_id=18446462667479739520 nr_regions=13 281472942936064-281473080008704...
> >>      ...target_id=18446462667479739520 nr_regions=13 281473080008704-281473216634880...
> >>
> >>  From a user's point of view, the 'target_id' and 'damon_region' which displays in decimal
> >> are not very friendly, So there do some changes, keep the 'target_id' display consistent
> >> with 'dbgfs/target_ids' interface and 'damon_region' is displayed in hexadecimal, just like
> >> below.
> >>      # perf record -e damon:damon_aggregated
> >>      # perf script
> >>      ...target_id=5522 nr_regions=14 ffff716a3000-ffff79893000...
> >>      ...target_id=5522 nr_regions=14 ffff79893000-ffff819dc000...
> >>      ...target_id=5522 nr_regions=14 ffff819dc000-ffff89bd9000...
> >>
> >> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> >> ---
> >>   include/trace/events/damon.h | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> >> index 99ffa601e351..67de51814f4c 100644
> >> --- a/include/trace/events/damon.h
> >> +++ b/include/trace/events/damon.h
[...]
> >>   	TP_fast_assign(
> >> -		__entry->target_id = t->id;
> >> +		__entry->target_id = (int)pid_vnr((struct pid *)t->id);
> > I think this would break physical address space monitoring.  Have you tested
> > this change for that?
> 
> Yes, you are right,  But I encountered some problems while testing 
> physical address, it seems that my operation did not work
> 
> I did some test like this:
> 
>      # echo "42 0x0000000840000000 0x000000103fffffff" > init_regions
> 
>      # echo paddr > target_ids
> 
>      # echo on > monitor_on
> 
> i get the physical address from my kernel startup log.
> 
> 15 [ 0.000000] Early memory node ranges
> 16 [ 0.000000] node 0: [mem 0x0000000040000000-0x000000083bc7ffff]
> 17 [ 0.000000] node 0: [mem 0x000000083bc80000-0x000000083bffffff]
> 18 [ 0.000000] node 0: [mem 0x000000083c000000-0x000000083c03ffff]
> 19 [ 0.000000] node 0: [mem 0x000000083c040000-0x000000083c0fffff]
> 20 [ 0.000000] node 0: [mem 0x000000083c100000-0x000000083f3dffff]
> 21 [ 0.000000] node 0: [mem 0x000000083f3e0000-0x000000083f46ffff]
> 22 [ 0.000000] node 0: [mem 0x000000083f470000-0x000000083f47ffff]
> 23 [ 0.000000] node 0: [mem 0x000000083f480000-0x000000083f59ffff]
> 24 [ 0.000000] node 0: [mem 0x000000083f5a0000-0x000000083fffffff]
> 25 [ 0.000000] node 1: [mem 0x0000000840000000-0x000000103fffffff]
> 26 [ 0.000000] node 2: [mem 0x0000001040000000-0x000000183fffffff]
> 27 [ 0.000000] node 3: [mem 0x0000001840000000-0x000000203fffffff]
> 28 [ 0.000000] Initmem setup node 0 [mem 
> 0x0000000040000000-0x000000083fffffff]
> 29 [ 0.000000] On node 0 totalpages: 8388608
> 
> Is there anything wrong ?

"The target id should already in target_ids file"[1].

For proper use of DAMON, I'd like to recommend you to refer to, or use the
official DAMON user space tool[2] instead of the debugfs interface.  As the
document[3] says "DAMON user space tool. This is for privileged people such as
system administrators who want a just-working human-friendly interface".

Also, some of your patches including this break the user space tool.  As it is
an important part of DAMON echosystem, it would be great if you could consider
taking care in keeping it unbroken, too.

[1] https://docs.kernel.org/admin-guide/mm/damon/usage.html#initial-monitoring-target-regions
[2] https://github.com/awslabs/damo
[3] https://docs.kernel.org/admin-guide/mm/damon/usage.html#detailed-usages


Thanks,
SJ

[...]