* [PATCH V1 0/2] mm/damon: Do little changes @ 2021-12-09 16:33 Xin Hao 2021-12-09 16:33 ` [PATCH V1 1/2] mm/damon/dbgfs: Avoid target_ids display wrong pid value Xin Hao 2021-12-09 16:33 ` [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint Xin Hao 0 siblings, 2 replies; 6+ messages in thread From: Xin Hao @ 2021-12-09 16:33 UTC (permalink / raw) To: sj; +Cc: xhao, akpm, linux-mm, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V1 1/2] mm/damon/dbgfs: Avoid target_ids display wrong pid value 2021-12-09 16:33 [PATCH V1 0/2] mm/damon: Do little changes Xin Hao @ 2021-12-09 16:33 ` Xin Hao 2021-12-09 16:33 ` [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint Xin Hao 1 sibling, 0 replies; 6+ messages in thread From: Xin Hao @ 2021-12-09 16:33 UTC (permalink / raw) To: sj; +Cc: xhao, akpm, linux-mm, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint 2021-12-09 16:33 [PATCH V1 0/2] mm/damon: Do little changes Xin Hao 2021-12-09 16:33 ` [PATCH V1 1/2] mm/damon/dbgfs: Avoid target_ids display wrong pid value Xin Hao @ 2021-12-09 16:33 ` Xin Hao 2021-12-09 16:46 ` SeongJae Park 1 sibling, 1 reply; 6+ messages in thread From: Xin Hao @ 2021-12-09 16:33 UTC (permalink / raw) To: sj; +Cc: xhao, akpm, linux-mm, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint 2021-12-09 16:33 ` [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint Xin Hao @ 2021-12-09 16:46 ` SeongJae Park 2021-12-10 3:36 ` Xin Hao 0 siblings, 1 reply; 6+ messages in thread From: SeongJae Park @ 2021-12-09 16:46 UTC (permalink / raw) To: Xin Hao; +Cc: sj, akpm, linux-mm, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint 2021-12-09 16:46 ` SeongJae Park @ 2021-12-10 3:36 ` Xin Hao 2021-12-10 8:18 ` SeongJae Park 0 siblings, 1 reply; 6+ messages in thread From: Xin Hao @ 2021-12-10 3:36 UTC (permalink / raw) To: SeongJae Park; +Cc: akpm, linux-mm, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint 2021-12-10 3:36 ` Xin Hao @ 2021-12-10 8:18 ` SeongJae Park 0 siblings, 0 replies; 6+ messages in thread From: SeongJae Park @ 2021-12-10 8:18 UTC (permalink / raw) To: Xin Hao; +Cc: SeongJae Park, akpm, linux-mm, linux-kernel 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 [...] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-10 8:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-09 16:33 [PATCH V1 0/2] mm/damon: Do little changes Xin Hao 2021-12-09 16:33 ` [PATCH V1 1/2] mm/damon/dbgfs: Avoid target_ids display wrong pid value Xin Hao 2021-12-09 16:33 ` [PATCH V1 2/2] mm/damon: Modify the display form of damon tracepoint Xin Hao 2021-12-09 16:46 ` SeongJae Park 2021-12-10 3:36 ` Xin Hao 2021-12-10 8:18 ` SeongJae Park
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox