diff mbox series

mm/damon/dbgfs: add region_stat interface

Message ID 20211012054948.90381-1-xhao@linux.alibaba.com (mailing list archive)
State New
Headers show
Series mm/damon/dbgfs: add region_stat interface | expand

Commit Message

haoxin Oct. 12, 2021, 5:49 a.m. UTC
Using damon-dbgfs has brought great convenience to user-mode
operation damon, but sometimes if i want to be able to view
the division of task regions, nr_access values etc,but i found
that it is impossible to view directly through the dbgfs interface,
so there i add a interface "region_stat", it displays like this.

 # cat region_stat
 last_aggregation=120.87s
 target_id=5148
 nr_regions=10
 400000-258c000(34352 KiB): 1
 258c000-4719000(34356 KiB): 0
 4719000-abbf000(103064 KiB): 0
 abbf000-c4d4000(25684 KiB): 11
 c4d4000-ff5c000(59936 KiB): 15
 ff5c000-152f9000(85620 KiB): 20
 152f9000-1599e000(6804 KiB): 10
 1599e000-19573000(61268 KiB): 0
 19573000-1f92c000(102116 KiB): 0
 1f92c000-22a4c000(50304 KiB): 0

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

Comments

SeongJae Park Oct. 12, 2021, 7:11 a.m. UTC | #1
Hello Xin, thank you for this patch!

On Tue, 12 Oct 2021 13:49:48 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> Using damon-dbgfs has brought great convenience to user-mode
> operation damon, but sometimes if i want to be able to view
> the division of task regions, nr_access values etc,but i found
> that it is impossible to view directly through the dbgfs interface,
> so there i add a interface "region_stat", it displays like this.
> 
>  # cat region_stat
>  last_aggregation=120.87s
>  target_id=5148
>  nr_regions=10
>  400000-258c000(34352 KiB): 1
>  258c000-4719000(34356 KiB): 0
>  4719000-abbf000(103064 KiB): 0
>  abbf000-c4d4000(25684 KiB): 11
>  c4d4000-ff5c000(59936 KiB): 15
>  ff5c000-152f9000(85620 KiB): 20
>  152f9000-1599e000(6804 KiB): 10
>  1599e000-19573000(61268 KiB): 0
>  19573000-1f92c000(102116 KiB): 0
>  1f92c000-22a4c000(50304 KiB): 0

I think similar information could also be collected via the 'damon_aggregated'
tracepoint[1], which is merged in the mainline, or 'DAMOS_STAT'[2], which is
merged in -mm.  The recording feature[3] could also be used, though it would
take some time before it is merged in the mainline.  Have you considered using
those but found some problem?

[1] https://git.kernel.org/torvalds/c/2fcb93629ad8
[2] https://lore.kernel.org/linux-mm/20211001125604.29660-6-sj@kernel.org/
[3] https://lore.kernel.org/linux-mm/20211008094509.16179-1-sj@kernel.org/


Thanks,
SJ

[...]
haoxin Oct. 12, 2021, 7:57 a.m. UTC | #2
在 2021/10/12 下午3:11, SeongJae Park 写道:
> Hello Xin, thank you for this patch!
>
> On Tue, 12 Oct 2021 13:49:48 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> Using damon-dbgfs has brought great convenience to user-mode
>> operation damon, but sometimes if i want to be able to view
>> the division of task regions, nr_access values etc,but i found
>> that it is impossible to view directly through the dbgfs interface,
>> so there i add a interface "region_stat", it displays like this.
>>
>>   # cat region_stat
>>   last_aggregation=120.87s
>>   target_id=5148
>>   nr_regions=10
>>   400000-258c000(34352 KiB): 1
>>   258c000-4719000(34356 KiB): 0
>>   4719000-abbf000(103064 KiB): 0
>>   abbf000-c4d4000(25684 KiB): 11
>>   c4d4000-ff5c000(59936 KiB): 15
>>   ff5c000-152f9000(85620 KiB): 20
>>   152f9000-1599e000(6804 KiB): 10
>>   1599e000-19573000(61268 KiB): 0
>>   19573000-1f92c000(102116 KiB): 0
>>   1f92c000-22a4c000(50304 KiB): 0
> I think similar information could also be collected via the 'damon_aggregated'
> tracepoint[1], which is merged in the mainline, or 'DAMOS_STAT'[2], which is
> merged in -mm.  The recording feature[3] could also be used, though it would
> take some time before it is merged in the mainline.  Have you considered using
> those but found some problem?

Yes, i know we can use damon_aggregated tracepoint, but i think, add a 
"region_stat" will be more

convenient and intuitive, especially when we use damon-dbgfs interface.

>
> [1] https://git.kernel.org/torvalds/c/2fcb93629ad8
> [2] https://lore.kernel.org/linux-mm/20211001125604.29660-6-sj@kernel.org/
> [3] https://lore.kernel.org/linux-mm/20211008094509.16179-1-sj@kernel.org/
>
>
> Thanks,
> SJ
>
> [...]
SeongJae Park Oct. 12, 2021, 9:37 a.m. UTC | #3
On Tue, 12 Oct 2021 15:57:17 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> 
> 在 2021/10/12 下午3:11, SeongJae Park 写道:
> > Hello Xin, thank you for this patch!
> >
> > On Tue, 12 Oct 2021 13:49:48 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
> >
> >> Using damon-dbgfs has brought great convenience to user-mode
> >> operation damon, but sometimes if i want to be able to view
> >> the division of task regions, nr_access values etc,but i found
> >> that it is impossible to view directly through the dbgfs interface,
> >> so there i add a interface "region_stat", it displays like this.
> >>
> >>   # cat region_stat
> >>   last_aggregation=120.87s
> >>   target_id=5148
> >>   nr_regions=10
> >>   400000-258c000(34352 KiB): 1
> >>   258c000-4719000(34356 KiB): 0
> >>   4719000-abbf000(103064 KiB): 0
> >>   abbf000-c4d4000(25684 KiB): 11
> >>   c4d4000-ff5c000(59936 KiB): 15
> >>   ff5c000-152f9000(85620 KiB): 20
> >>   152f9000-1599e000(6804 KiB): 10
> >>   1599e000-19573000(61268 KiB): 0
> >>   19573000-1f92c000(102116 KiB): 0
> >>   1f92c000-22a4c000(50304 KiB): 0
> > I think similar information could also be collected via the 'damon_aggregated'
> > tracepoint[1], which is merged in the mainline, or 'DAMOS_STAT'[2], which is
> > merged in -mm.  The recording feature[3] could also be used, though it would
> > take some time before it is merged in the mainline.  Have you considered using
> > those but found some problem?
> 
> Yes, i know we can use damon_aggregated tracepoint, but i think, add a 
> "region_stat" will be more
> 
> convenient and intuitive, especially when we use damon-dbgfs interface.

Thanks for the answer.

For more conveniend and intuitive interfaces, I recommend DAMON user-sapce
tool[1] rather than the debugfs interface.  You can also implement your own
user-space tool on top of the debugfs interface.  For example, the reference
implementation[1] implements 'record' feature on top of the tracepoint.  The
feature can provide below information, which is quite similar to what you want.
For more information on this, please refer to the document[2].

    # damo record $(pidof $workload)
    # damo report raw
    base_time_absolute: 8 m 59.809 s

    monitoring_start:                0 ns
    monitoring_end:            104.599 ms
    monitoring_duration:       104.599 ms
    target_id: 18446623438842320000
    nr_regions: 3
    563ebaa00000-563ebc99e000(  31.617 MiB):        1
    7f938d7e1000-7f938ddfc000(   6.105 MiB):        0
    7fff66b0a000-7fff66bb2000( 672.000 KiB):        0

    monitoring_start:          104.599 ms
    monitoring_end:            208.590 ms
    monitoring_duration:       103.991 ms
    target_id: 18446623438842320000
    nr_regions: 4
    563ebaa00000-563ebc99e000(  31.617 MiB):        1
    7f938d7e1000-7f938d9b5000(   1.828 MiB):        0
    7f938d9b5000-7f938ddfc000(   4.277 MiB):        0
    7fff66b0a000-7fff66bb2000( 672.000 KiB):        5

If this is still not so useful for you, could you please elaborate more on your
detailed requirements and use cases?

[1] https://github.com/awslabs/damo
[2] https://github.com/awslabs/damo/blob/next/USAGE.md#raw


Thanks,
SJ

> 
> >
> > [1] https://git.kernel.org/torvalds/c/2fcb93629ad8
> > [2] https://lore.kernel.org/linux-mm/20211001125604.29660-6-sj@kernel.org/
> > [3] https://lore.kernel.org/linux-mm/20211008094509.16179-1-sj@kernel.org/
> >
> >
> > Thanks,
> > SJ
> >
> > [...]
> 
> -- 
> Best Regards!
> Xin Hao
kernel test robot Oct. 12, 2021, 5:16 p.m. UTC | #4
Hi Xin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.15-rc5]
[cannot apply to hnaz-mm/master next-20211012]
[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]

url:    https://github.com/0day-ci/linux/commits/Xin-Hao/mm-damon-dbgfs-add-region_stat-interface/20211012-135023
base:    64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
config: arm64-randconfig-r014-20211012 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c3dcf39554dbea780d6cb7e12239451ba47a2668)
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
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/256bdd48bd7719684cd8adbbda8a9df3dc9c1008
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Xin-Hao/mm-damon-dbgfs-add-region_stat-interface/20211012-135023
        git checkout 256bdd48bd7719684cd8adbbda8a9df3dc9c1008
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> mm/damon/dbgfs.c:311:3: warning: variable 'len' is uninitialized when used here [-Wuninitialized]
                   len += simple_read_from_buffer(buf, count, ppos, kbuf, written);
                   ^~~
   mm/damon/dbgfs.c:275:13: note: initialize the variable 'len' to silence this warning
           ssize_t len;
                      ^
                       = 0
   1 warning generated.


vim +/len +311 mm/damon/dbgfs.c

   268	
   269	static ssize_t dbgfs_region_stat_read(struct file *file,
   270			char __user *buf, size_t count, loff_t *ppos)
   271	{
   272		struct damon_ctx *ctx = file->private_data;
   273		struct damon_target *t;
   274		char *kbuf;
   275		ssize_t len;
   276		int id, rc, written = 0;
   277	
   278		kbuf = kmalloc(count, GFP_KERNEL);
   279		if (!kbuf)
   280			return -ENOMEM;
   281	
   282		mutex_lock(&ctx->kdamond_lock);
   283		damon_for_each_target(t, ctx) {
   284			struct damon_region *r;
   285	
   286			if (targetid_is_pid(ctx))
   287				id = (int)pid_vnr((struct pid *)t->id);
   288	
   289			rc = scnprintf(&kbuf[written], count - written,
   290					"last_aggregation=%lld.%lds\ntarget_id=%d\nnr_regions=%u\n",
   291					ctx->last_aggregation.tv_sec,
   292					ctx->last_aggregation.tv_nsec / 1000000,
   293					id, t->nr_regions);
   294			if (!rc)
   295				goto out;
   296	
   297			written += rc;
   298	
   299			damon_for_each_region(r, t) {
   300				rc = scnprintf(&kbuf[written], count - written,
   301					       "%lx-%lx(%lu KiB): %u\n",
   302					       r->ar.start, r->ar.end,
   303						   (r->ar.end - r->ar.start) >> 10,
   304						   r->nr_accesses);
   305				if (!rc)
   306					goto out;
   307	
   308				written += rc;
   309			}
   310	
 > 311			len += simple_read_from_buffer(buf, count, ppos, kbuf, written);
   312		}
   313	
   314	out:
   315		mutex_unlock(&ctx->kdamond_lock);
   316		kfree(kbuf);
   317		return len;
   318	}
   319	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index faee070977d8..269a336e92f0 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -266,6 +266,57 @@  static ssize_t dbgfs_kdamond_pid_read(struct file *file,
 	return len;
 }
 
+static ssize_t dbgfs_region_stat_read(struct file *file,
+		char __user *buf, size_t count, loff_t *ppos)
+{
+	struct damon_ctx *ctx = file->private_data;
+	struct damon_target *t;
+	char *kbuf;
+	ssize_t len;
+	int id, rc, written = 0;
+
+	kbuf = kmalloc(count, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	mutex_lock(&ctx->kdamond_lock);
+	damon_for_each_target(t, ctx) {
+		struct damon_region *r;
+
+		if (targetid_is_pid(ctx))
+			id = (int)pid_vnr((struct pid *)t->id);
+
+		rc = scnprintf(&kbuf[written], count - written,
+				"last_aggregation=%lld.%lds\ntarget_id=%d\nnr_regions=%u\n",
+				ctx->last_aggregation.tv_sec,
+				ctx->last_aggregation.tv_nsec / 1000000,
+				id, t->nr_regions);
+		if (!rc)
+			goto out;
+
+		written += rc;
+
+		damon_for_each_region(r, t) {
+			rc = scnprintf(&kbuf[written], count - written,
+				       "%lx-%lx(%lu KiB): %u\n",
+				       r->ar.start, r->ar.end,
+					   (r->ar.end - r->ar.start) >> 10,
+					   r->nr_accesses);
+			if (!rc)
+				goto out;
+
+			written += rc;
+		}
+
+		len += simple_read_from_buffer(buf, count, ppos, kbuf, written);
+	}
+
+out:
+	mutex_unlock(&ctx->kdamond_lock);
+	kfree(kbuf);
+	return len;
+}
+
 static int damon_dbgfs_open(struct inode *inode, struct file *file)
 {
 	file->private_data = inode->i_private;
@@ -290,12 +341,17 @@  static const struct file_operations kdamond_pid_fops = {
 	.read = dbgfs_kdamond_pid_read,
 };
 
+static const struct file_operations region_stat_fops = {
+	.open = damon_dbgfs_open,
+	.read = dbgfs_region_stat_read,
+};
+
 static void dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
 {
 	const char * const file_names[] = {"attrs", "target_ids",
-		"kdamond_pid"};
+		"kdamond_pid", "region_stat"};
 	const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops,
-		&kdamond_pid_fops};
+		&kdamond_pid_fops, &region_stat_fops};
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(file_names); i++)