V1 -> V2
Fix patch1 compile problem.
Add review by SeongJae Park in patch2
V1:
https://lkml.org/lkml/2021/11/20/191
Xin Hao (2):
mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig
mm/damon: move damon_rand() definition into damon.h
include/linux/damon.h | 16 ++++++++++++++++
mm/damon/Kconfig | 2 +-
mm/damon/core.c | 4 ----
mm/damon/prmtv-common.h | 4 ----
4 files changed, 17 insertions(+), 9 deletions(-)
--
2.31.0
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.
Signed-off-by: Xin Hao <[email protected]>
---
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
help
This builds the debugfs interface for DAMON. The user space admins
can use the interface for arbitrary data access monitoring.
--
2.31.0
damon_rand() is called in three files:damon/core.c, damon/
paddr.c, damon/vaddr.c, i think there is no need to redefine
this twice, So move it to damon.h will be a good choice.
Signed-off-by: Xin Hao <[email protected]>
Reviewed-by: SeongJae Park <[email protected]>
---
include/linux/damon.h | 4 ++++
mm/damon/core.c | 4 ----
mm/damon/prmtv-common.h | 4 ----
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 00ad96f2ec10..c6df025d8704 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -11,12 +11,16 @@
#include <linux/mutex.h>
#include <linux/time64.h>
#include <linux/types.h>
+#include <linux/random.h>
/* Minimal region size. Every damon_region is aligned by this. */
#define DAMON_MIN_REGION PAGE_SIZE
/* Max priority score for DAMON-based operation schemes */
#define DAMOS_MAX_SCORE (99)
+/* Get a random number in [l, r) */
+#define damon_rand(l, r) (l + prandom_u32_max(r - l))
+
/**
* struct damon_addr_range - Represents an address region of [@start, @end).
* @start: Start address of the region (inclusive).
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 4d2c3a0c7c8a..bdec32ef78c0 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -11,7 +11,6 @@
#include <linux/delay.h>
#include <linux/kthread.h>
#include <linux/mm.h>
-#include <linux/random.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -23,9 +22,6 @@
#define DAMON_MIN_REGION 1
#endif
-/* Get a random number in [l, r) */
-#define damon_rand(l, r) (l + prandom_u32_max(r - l))
-
static DEFINE_MUTEX(damon_lock);
static int nr_running_ctxs;
diff --git a/mm/damon/prmtv-common.h b/mm/damon/prmtv-common.h
index 61f27037603e..e790cb5f8fe0 100644
--- a/mm/damon/prmtv-common.h
+++ b/mm/damon/prmtv-common.h
@@ -6,10 +6,6 @@
*/
#include <linux/damon.h>
-#include <linux/random.h>
-
-/* Get a random number in [l, r) */
-#define damon_rand(l, r) (l + prandom_u32_max(r - l))
struct page *damon_get_page(unsigned long pfn);
--
2.31.0
On Sun, 21 Nov 2021 22:07:04 +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, 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 <[email protected]>
> ---
> 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? 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.
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
Hi Park:
On 11/23/21 6:38 PM, SeongJae Park wrote:
> On Sun, 21 Nov 2021 22:07:04 +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, 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 <[email protected]>
>> ---
>> 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