diff mbox series

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

Message ID 1e019e8ffe8c040376ec59e918d301058cc58ade.1639066954.git.xhao@linux.alibaba.com (mailing list archive)
State New
Headers show
Series mm/damon: Do little changes | expand

Commit Message

haoxin Dec. 9, 2021, 4:33 p.m. UTC 
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(-)

--
2.31.0

Comments

SeongJae Park Dec. 9, 2021, 4:46 p.m. UTC | #1 
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
haoxin Dec. 10, 2021, 3:36 a.m. UTC | #2 
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
SeongJae Park Dec. 10, 2021, 8:18 a.m. UTC | #3 
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

[...]
diff mbox series

Patch

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)