Messages in this thread | | | From | Xin Hao <> | Subject | Re: [PATCH V2 1/2] mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig | Date | Tue, 23 Nov 2021 19:25:52 +0800 |
| |
Hi Park:
On 11/23/21 6:38 PM, SeongJae Park wrote: > On Sun, 21 Nov 2021 22:07:04 +0800 Xin Hao <xhao@linux.alibaba.com> 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]. > > [1] https://docs.kernel.org/process/submitting-patches.html?highlight=75#the-canonical-patch-format > >> Signed-off-by: Xin Hao <xhao@linux.alibaba.com> >> --- >> 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); >> #ifdef CONFIG_DAMON_VADDR >> void damon_va_set_primitives(struct damon_ctx *ctx); >> bool damon_va_target_valid(void *t); >> +#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 >> void damon_pa_set_primitives(struct damon_ctx *ctx); >> bool damon_pa_target_valid(void *t); >> +#else >> +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 */ >> 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 >> >> config DAMON_DBGFS >> bool "DAMON debugfs interface" >> - depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS >> + depends on DAMON_VADDR || DAMON_PADDR && DEBUG_FS > 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 > intending?
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, > tracefs. > > 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.
> > > Thanks, > SJ > >> help >> This builds the debugfs interface for DAMON. The user space admins >> can use the interface for arbitrary data access monitoring. >> -- >> 2.31.0
-- Best Regards! Xin Hao
| |