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(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 794b5f453662..c83e4588aa45 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -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 */
diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
index 5bcf05851ad0..a8952af3c8bf 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
help
This builds the debugfs interface for DAMON. The user space admins
can use the interface for arbitrary data access monitoring.
--
2.27.0
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?
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
>
> 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(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 794b5f453662..c83e4588aa45 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -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 */
> diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
> index 5bcf05851ad0..a8952af3c8bf 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
> help
> This builds the debugfs interface for DAMON. The user space admins
> can use the interface for arbitrary data access monitoring.
> --
> 2.27.0
>
On 1/21/22 1:46 AM, SeongJae Park wrote:
> 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?
>
> 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.
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.
>
> 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
>
>> 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(-)
>>
>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>> index 794b5f453662..c83e4588aa45 100644
>> --- a/include/linux/damon.h
>> +++ b/include/linux/damon.h
>> @@ -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 */
>> diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
>> index 5bcf05851ad0..a8952af3c8bf 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
>> help
>> This builds the debugfs interface for DAMON. The user space admins
>> can use the interface for arbitrary data access monitoring.
>> --
>> 2.27.0
>>
--
Best Regards!
Xin Hao
Hi Xin,
On Fri, 21 Jan 2022 10:14:36 +0800 Xin Hao <[email protected]> wrote:
>
> On 1/21/22 1:46 AM, SeongJae Park wrote:
> > 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?
> >
> > 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.
>
> 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]/
>
>
> 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
>
> >
> > 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
> >
> >> 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(-)
> >>
> >> diff --git a/include/linux/damon.h b/include/linux/damon.h
> >> index 794b5f453662..c83e4588aa45 100644
> >> --- a/include/linux/damon.h
> >> +++ b/include/linux/damon.h
> >> @@ -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 */
> >> diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
> >> index 5bcf05851ad0..a8952af3c8bf 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
> >> help
> >> This builds the debugfs interface for DAMON. The user space admins
> >> can use the interface for arbitrary data access monitoring.
> >> --
> >> 2.27.0
> >>
> --
> Best Regards!
> Xin Hao
>