[PATCH V2 0/2] mm/damon: Fix some little bugs in using DAMON

[PATCH V2 0/2] mm/damon: Fix some little bugs in using DAMON

[Xin Hao]

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

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

[Xin Hao]

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 <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
 	help
 	  This builds the debugfs interface for DAMON.  The user space admins
 	  can use the interface for arbitrary data access monitoring.
-- 
2.31.0

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

[SeongJae Park]

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?  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

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

[Xin Hao]

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

[PATCH V2 2/2] mm/damon: move damon_rand() definition into damon.h

[Xin Hao]

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 <xhao@linux.alibaba.com>
Reviewed-by: SeongJae Park <sj@kernel.org>
---
 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