mm/damon/dbgfs: Put "paddr" check into damon_pa_check() func

PATCH In review Xin Hao 2 participants 6 messages Jan 20, 2022 – Jan 21, 2022 1 message not in the archive

Subsystems (2)

Memory management DAMON

lore

Share & tools

lore

Xin Hao

author

2022-01-20 16:04 UTC #3 lore raw

PATCH 2/2 mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig

In reply to a message not in the archive

If you want to support "DAMON_DBGFS" in config file, it only depends on any one of "DAMON_VADDR" and "DAMON_PADDR", and sometimes we just want to use damon virtual address function, but it is unreasonable to include "DAMON_PADDR" in config file which cause the damon/paddr.c be compiled.

So there just do little change to fix it.

Signed-off-by Xin Hao <[email protected]>

---
 include/linux/damon.h | 16 ++++++++++++++++
 mm/damon/Kconfig      |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

include/linux/damon.h +16 -0

index 794b5f453662..c83e4588aa45 100644
@@ -501,12 +501,28 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
 #ifdef CONFIG_DAMON_VADDR
 bool damon_va_target_valid(void *t);
 void damon_va_set_primitives(struct damon_ctx *ctx);
+#else
+static inline void damon_va_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_va_target_valid(void *t)
+{
+	return false;
+}
 #endif	/* CONFIG_DAMON_VADDR */
 
 #ifdef CONFIG_DAMON_PADDR
 bool damon_pa_check(char *buf, size_t count);
 bool damon_pa_target_valid(void *t);
 void damon_pa_set_primitives(struct damon_ctx *ctx);
+#else
+static inline bool damon_pa_check(char *buf, size_t count)
+{
+	return false;
+}
+static inline void damon_pa_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_pa_target_valid(void *t)
+{
+	return false;
+}
 #endif	/* CONFIG_DAMON_PADDR */
 
 #endif	/* _DAMON_H */

mm/damon/Kconfig +1 -1

index 5bcf05851ad0..a8952af3c8bf 100644
@@ -54,7 +54,7 @@ config DAMON_VADDR_KUNIT_TEST
 
 config DAMON_DBGFS
 	bool "DAMON debugfs interface"
-	depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS
+	depends on (DAMON_VADDR || DAMON_PADDR) && DEBUG_FS
 	help
 	  This builds the debugfs interface for DAMON.  The user space admins
 	  can use the interface for arbitrary data access monitoring.

--
2.27.0

SeongJae Park

maintainer

2022-01-20 17:46 UTC #4 lore raw

Re: [PATCH V1 2/2] mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig

Hi Xin,

I guess this should be at least v3, as you posted v1[1] and v2[2] before. Could you please also summarize what changes are made from v2?

Quoted 4 lines · On Fri, 21 Jan 2022 00:04:28 +0800 Xin Hao <[email protected]> wrote:

If you want to support "DAMON_DBGFS" in config file, it only depends on any one of "DAMON_VADDR" and "DAMON_PADDR", and sometimes we just want to use damon virtual address function, but it is unreasonable to include "DAMON_PADDR" in config file which cause the damon/paddr.c be compiled.

My comment[3] to the v2 was,

So, unless you make me believe the benefit is big enough and all possible corner cases are well handled and sufficiently tested, I'm sorry but I would not be convinced with this change.

I don't find answers to the requests here. Could you please add those if you have?

[1] https://lore.kernel.org/linux-mm/fbf27c39e8f23d12d5474b9b659d3d45f8dd38ca.1637429074.git.xhao@lin
ux.alibaba.com/
[2] https://lore.kernel.org/linux-mm/[email protected]/
[3] https://lore.kernel.org/linux-mm/[email protected]/

Thanks,
SJ

Quoted 58 lines

So there just do little change to fix it.

Signed-off-by Xin Hao <[email protected]>

---
 include/linux/damon.h | 16 ++++++++++++++++
 mm/damon/Kconfig      |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

include/linux/damon.h +16 -0

index 794b5f453662..c83e4588aa45 100644
@@ -501,12 +501,28 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
 #ifdef CONFIG_DAMON_VADDR
 bool damon_va_target_valid(void *t);
 void damon_va_set_primitives(struct damon_ctx *ctx);
+#else
+static inline void damon_va_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_va_target_valid(void *t)
+{
+	return false;
+}
 #endif	/* CONFIG_DAMON_VADDR */
 
 #ifdef CONFIG_DAMON_PADDR
 bool damon_pa_check(char *buf, size_t count);
 bool damon_pa_target_valid(void *t);
 void damon_pa_set_primitives(struct damon_ctx *ctx);
+#else
+static inline bool damon_pa_check(char *buf, size_t count)
+{
+	return false;
+}
+static inline void damon_pa_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_pa_target_valid(void *t)
+{
+	return false;
+}
 #endif	/* CONFIG_DAMON_PADDR */
 
 #endif	/* _DAMON_H */

mm/damon/Kconfig +1 -2

index 5bcf05851ad0..a8952af3c8bf 100644
@@ -54,7 +54,7 @@ config DAMON_VADDR_KUNIT_TEST
 
 config DAMON_DBGFS
 	bool "DAMON debugfs interface"
-	depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS
+	depends on (DAMON_VADDR || DAMON_PADDR) && DEBUG_FS
 	help
 	  This builds the debugfs interface for DAMON.  The user space admins
 	  can use the interface for arbitrary data access monitoring.
--

2.27.0

SeongJae Park

maintainer

2022-01-20 17:47 UTC #2 lore raw

In reply to Xin Hao

Hi Xin,

Quoted 3 lines · On Fri, 21 Jan 2022 00:04:27 +0800 Xin Hao <[email protected]> wrote:

The purpose of putting "paddr" check into damon_pa_check() func is to better solve "DAMON_DBGFS" dependency on both "DAMON_VADDR" and "DAMON_PADDR".

Syntax of "paddr" is dbgfs's own. I don't think handling it in paddr is a right thing to do.

Thanks,
SJ

Quoted 56 lines

Signed-off-by Xin Hao <[email protected]>

---
 include/linux/damon.h | 1 +
 mm/damon/dbgfs.c      | 2 +-
 mm/damon/paddr.c      | 8 ++++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

include/linux/damon.h +1 -0

index 5e1e3a128b77..794b5f453662 100644
@@ -504,6 +504,7 @@ void damon_va_set_primitives(struct damon_ctx *ctx);
 #endif	/* CONFIG_DAMON_VADDR */
 
 #ifdef CONFIG_DAMON_PADDR
+bool damon_pa_check(char *buf, size_t count);
 bool damon_pa_target_valid(void *t);
 void damon_pa_set_primitives(struct damon_ctx *ctx);
 #endif	/* CONFIG_DAMON_PADDR */

mm/damon/dbgfs.c +1 -1

index 5b899601e56c..cdb9c1e2ab29 100644
@@ -374,7 +374,7 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
 	if (IS_ERR(kbuf))
 		return PTR_ERR(kbuf);
 
-	if (!strncmp(kbuf, "paddr\n", count)) {
+	if (damon_pa_check(kbuf, count)) {
 		id_is_pid = false;
 		/* target id is meaningless here, but we set it just for fun */
 		scnprintf(kbuf, count, "42    ");

mm/damon/paddr.c +8 -1

index 5e8244f65a1a..14a6bf2a2adb 100644
@@ -208,6 +208,14 @@ static unsigned int damon_pa_check_accesses(struct damon_ctx *ctx)
 	return max_nr_accesses;
 }
 
+bool damon_pa_check(char *buf, size_t count)
+{
+	if (!strncmp(buf, "paddr\n", count))
+		return true;
+
+	return false;
+}
+
 bool damon_pa_target_valid(void *t)
 {
 	return true;
--

2.27.0

Xin Hao

author

2022-01-21 02:14 UTC #5 lore raw

Re: [PATCH V1 2/2] mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig

In reply to SeongJae Park

Quoted 12 lines · SeongJae Park

Hi Xin,
I guess this should be at least v3, as you posted v1[1] and v2[2] before. Could you please also summarize what changes are made from v2?
Quoted 4 lines · On Fri, 21 Jan 2022 00:04:28 +0800 Xin Hao <[email protected]> wrote:
If you want to support "DAMON_DBGFS" in config file, it only depends on any one of "DAMON_VADDR" and "DAMON_PADDR", and sometimes we just want to use damon virtual address function, but it is unreasonable to include "DAMON_PADDR" in config file which cause the damon/paddr.c be compiled.

Quoted 5 lines

My comment[3] to the v2 was,
So, unless you make me believe the benefit is big enough and all possible corner cases are well handled and sufficiently tested, I'm sorry but I would not be convinced with this change.

This has really bothered me for a long time. I don't send patches just for the sake of sending patches. I integrate DAMON patches into my kernel, and one obvious problem is that I don't use paddr at all. But my config had to select DAMON_PADDR so that my DAMON codes would compile and work. I wanted DAMON code to be as simple and clean as possible, which is why I sent the patch for three times.

I quite agree with what you said, it is necessary to conduct sufficient test and verification, which I have not done enough. However, I will conduct further test later. I don't think we should not modify the previous test case just because it will affect it. I can guarantee the tools/testing/selftests/Damon and kunit Damon test right before i send a new version.

Quoted 70 lines

I don't find answers to the requests here. Could you please add those if you have?

Thanks,
SJ

Quoted 57 lines

So there just do little change to fix it.

Signed-off-by Xin Hao <[email protected]>

---
  include/linux/damon.h | 16 ++++++++++++++++
  mm/damon/Kconfig      |  2 +-
  2 files changed, 17 insertions(+), 1 deletion(-)

include/linux/damon.h +16 -0

index 794b5f453662..c83e4588aa45 100644
@@ -501,12 +501,28 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
  #ifdef CONFIG_DAMON_VADDR
  bool damon_va_target_valid(void *t);
  void damon_va_set_primitives(struct damon_ctx *ctx);
+#else
+static inline void damon_va_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_va_target_valid(void *t)
+{
+	return false;
+}
  #endif	/* CONFIG_DAMON_VADDR */
 
  #ifdef CONFIG_DAMON_PADDR
  bool damon_pa_check(char *buf, size_t count);
  bool damon_pa_target_valid(void *t);
  void damon_pa_set_primitives(struct damon_ctx *ctx);
+#else
+static inline bool damon_pa_check(char *buf, size_t count)
+{
+	return false;
+}
+static inline void damon_pa_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_pa_target_valid(void *t)
+{
+	return false;
+}
  #endif	/* CONFIG_DAMON_PADDR */
 
  #endif	/* _DAMON_H */

mm/damon/Kconfig +1 -2

index 5bcf05851ad0..a8952af3c8bf 100644
@@ -54,7 +54,7 @@ config DAMON_VADDR_KUNIT_TEST
 
  config DAMON_DBGFS
  	bool "DAMON debugfs interface"
-	depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS
+	depends on (DAMON_VADDR || DAMON_PADDR) && DEBUG_FS
  	help
  	  This builds the debugfs interface for DAMON.  The user space admins
  	  can use the interface for arbitrary data access monitoring.
--

2.27.0

Signature · 2 lines

Best Regards!
Xin Hao

SeongJae Park

maintainer

2022-01-21 09:45 UTC #6 lore raw

Re: [PATCH V1 2/2] mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig

Hi Xin,

Quoted 20 lines · On Fri, 21 Jan 2022 10:14:36 +0800 Xin Hao <[email protected]> wrote:

Quoted 12 lines · SeongJae Park
Hi Xin,
I guess this should be at least v3, as you posted v1[1] and v2[2] before. Could you please also summarize what changes are made from v2?
Quoted 4 lines · On Fri, 21 Jan 2022 00:04:28 +0800 Xin Hao <[email protected]> wrote:
If you want to support "DAMON_DBGFS" in config file, it only depends on any one of "DAMON_VADDR" and "DAMON_PADDR", and sometimes we just want to use damon virtual address function, but it is unreasonable to include "DAMON_PADDR" in config file which cause the damon/paddr.c be compiled.
Quoted 5 lines
My comment[3] to the v2 was,
So, unless you make me believe the benefit is big enough and all possible corner cases are well handled and sufficiently tested, I'm sorry but I would not be convinced with this change.

Quoted 6 lines

This has really bothered me for a long time. I don't send patches just for the sake of sending patches. I integrate DAMON patches into my kernel, and one obvious problem is that I don't use paddr at all. But my config had to select DAMON_PADDR so that my DAMON codes would compile and work. I wanted DAMON code to be as simple and clean as possible, which is why I sent the patch for three times.

Thank you for the clarification. I know you really want to improve DAMON and made great progress already. I really appreciate that. I can also understand your frustration. Of course, it's optimal to compile only what we need. However, this code is for the community, not for specific users. And, I still unsure how much pain is incurred by the compilation of one additional source file for you, and others in the community.

And, sometimes we can compile some of unnecessary code and thus make the binary a tiny bit bigger to make the code simple. If it seems the benefit of dropping DAMON_PADDR dependency is clearly bigger than increased complexity of dbgfs and the change doesn't break any other things to me, I will happily give you my 'Reviewed-by:'. Else, I'm sorry but I wouldn't.

Also, please note that I have a plan[1] to make more reliable DAMON user interface based on sysfs, and it would not have such unnecessary dependencies.

[1] https://lore.kernel.org/linux-mm/[email protected]/

Quoted 8 lines

I quite agree with what you said, it is necessary to conduct sufficient test and verification, which I have not done enough. However, I will conduct further test later. I don't think we should not modify the previous test case just because it will affect it. I can guarantee the tools/testing/selftests/Damon and kunit Damon test right before i send a new version.

Those are for minimal tests. I'd like to suggest you to do
https://github.com/awslabs/damon-tests/tree/master/corr, or at least
https://github.com/awslabs/damo/blob/next/tests/run.sh in addition to those.

Also, please ensure you are caring not only DAMON_VADDR && DEBUG_FS, but also DAMON_PADDR && DEBUG_FS. This also reveals another downside of this patch. This requires DAMON tests to cover the one more configuration case. And actually it's currently affecting you. I hope the benefit of next patch clearly outweighs the effort for that.

Of course, it would be even greter if you can clearly guarantee the patch will not break other things with the code or comments.

Thanks,
SJ

Quoted 75 lines

Quoted 71 lines

I don't find answers to the requests here. Could you please add those if you have?

Thanks,
SJ

Quoted 57 lines

So there just do little change to fix it.

Signed-off-by Xin Hao <[email protected]>

---
  include/linux/damon.h | 16 ++++++++++++++++
  mm/damon/Kconfig      |  2 +-
  2 files changed, 17 insertions(+), 1 deletion(-)

include/linux/damon.h +16 -0

index 794b5f453662..c83e4588aa45 100644
@@ -501,12 +501,28 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
  #ifdef CONFIG_DAMON_VADDR
  bool damon_va_target_valid(void *t);
  void damon_va_set_primitives(struct damon_ctx *ctx);
+#else
+static inline void damon_va_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_va_target_valid(void *t)
+{
+	return false;
+}
  #endif	/* CONFIG_DAMON_VADDR */
 
  #ifdef CONFIG_DAMON_PADDR
  bool damon_pa_check(char *buf, size_t count);
  bool damon_pa_target_valid(void *t);
  void damon_pa_set_primitives(struct damon_ctx *ctx);
+#else
+static inline bool damon_pa_check(char *buf, size_t count)
+{
+	return false;
+}
+static inline void damon_pa_set_primitives(struct damon_ctx *ctx) { }
+static inline bool damon_pa_target_valid(void *t)
+{
+	return false;
+}
  #endif	/* CONFIG_DAMON_PADDR */
 
  #endif	/* _DAMON_H */

mm/damon/Kconfig +1 -2

index 5bcf05851ad0..a8952af3c8bf 100644
@@ -54,7 +54,7 @@ config DAMON_VADDR_KUNIT_TEST
 
  config DAMON_DBGFS
  	bool "DAMON debugfs interface"
-	depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS
+	depends on (DAMON_VADDR || DAMON_PADDR) && DEBUG_FS
  	help
  	  This builds the debugfs interface for DAMON.  The user space admins
  	  can use the interface for arbitrary data access monitoring.
--

2.27.0

Signature · 2 lines

Best Regards!
Xin Hao