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

From: Xin Hao
Date: Tue Nov 23 2021 - 06:25:59 EST

Hi Park:

On 11/23/21 6:38 PM, SeongJae Park wrote:
On Sun, 21 Nov 2021 22:07:04 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> 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, so there fix it.
Seems the above lines are not well wrapped[1].


Signed-off-by: Xin Hao <xhao@xxxxxxxxxxxxxxxxx>
include/linux/damon.h | 12 ++++++++++++
mm/damon/Kconfig | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 8a73e825e0d5..00ad96f2ec10 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -463,11 +463,23 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
void damon_va_set_primitives(struct damon_ctx *ctx);
bool damon_va_target_valid(void *t);
+static inline void damon_va_set_primitives(struct damon_ctx *ctx) {}
+static inline bool damon_va_target_valid(void *t)
+ return false;
void damon_pa_set_primitives(struct damon_ctx *ctx);
bool damon_pa_target_valid(void *t);
+static inline void damon_pa_set_primitives(struct damon_ctx *ctx) {}
+static inline bool damon_pa_target_valid(void *t)
+ return false;
#endif /* _DAMON_H */
diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
index 5bcf05851ad0..971ffc496596 100644
--- a/mm/damon/Kconfig
+++ b/mm/damon/Kconfig
@@ -54,7 +54,7 @@ config DAMON_VADDR_KUNIT_TEST
bool "DAMON debugfs interface"
Due to the lack of parentheses, this allows enabling DAMON_DBGFS while DEBUG_FS
is disabled.

Sorry to say this but this makes me unsure if this patch is sufficiently
tested. I am also unsure if this patch is carefully handling all possible
corner cases in appropriate ways. For example, on kernels that doesn't having
'CONFIG_DAMON_PADDR=y', this patch would still make
'echo paddr > $debugfs/damon/target_ids' success. Is that what you are

Yes, you are all right, I have not tested as above, sorry, i did not fully consider the impact on the code after doing this,

Please ignore this patch.

And, have you confirmed that will never make any issue, both in
kernel space and the user space? Could the behavioral change make the user
space confused?

Basically, reducing the dependency is a good idea. But, IMHO, the benefit
seems not so big. After all, efficiency was not a first goal of DAMON debugfs
interface. That's why it is implemented on debugfs rather than its own
specialized/optimized file system. Better way for the efficiency of the user
space interface might be inventing such DAMON's own efficient file system, like
tracepoints had its interface on debugfs but now uses its own file system,

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 think it makes sense to separate PADDR & VADDR, users can freely choose dbfs + vaddr or dbfs + paddr;

Doing this can make the code look like it doesn’t mix together,i will continue to try to separate this part

of the code later.


This builds the debugfs interface for DAMON. The user space admins
can use the interface for arbitrary data access monitoring.

Best Regards!
Xin Hao