Re: [PATCH] mm/damon: Add option to monitor only writes

Re: [PATCH] mm/damon: Add option to monitor only writes

[SeongJae Park]

Hi Pedro,


Thank you for this patch!

On Thu, 3 Feb 2022 10:12:36 -0300 Pedro Demarchi Gomes <pedrodemargomes@gmail.com> wrote:

> When "writes" is written to /sys/kernel/debug/damon/counter_type damon will
> monitor only writes.

I think this would be better to be implemented as another monitoring primitive
based on the virtual address space monitoring primitives, e.g., vaddr-writes?
Then the implementation would be simpler and changes to other files will be
minimized.  For the user space interface, we could use a prefix to target_ids
debugfs file.  For example,

    # echo "vaddr-writes $(pidof workload)" > $debugfs/damon/target_ids


> This patch also adds the actions mergeable and unmergeable to damos schemes.
> These actions are used by KSM as explained in [1].

Please do that in a separate patch, and also update the document
(Documentation/admin-guide/mm/damon/usage.rst).  And, what's the expected usage
of the action and benefits?


Thanks,
SJ

> 
> [1] https://www.kernel.org/doc/html/latest/admin-guide/mm/ksm.html#controlling-ksm-with-madvise
> 
> Signed-off-by: Pedro Demarchi Gomes <pedrodemargomes@gmail.com>
> ---
>  include/linux/damon.h        | 13 ++++++
>  include/trace/events/damon.h | 10 ++--
>  mm/damon/core.c              | 91 +++++++++++++++++++++++++-----------
>  mm/damon/dbgfs.c             | 72 +++++++++++++++++++++++++++-
>  mm/damon/prmtv-common.c      | 88 ++++++++++++++++++++++++++++++++++
>  mm/damon/prmtv-common.h      |  5 ++
>  mm/damon/vaddr.c             | 80 ++++++++++++++++++++++++++++---
>  7 files changed, 318 insertions(+), 41 deletions(-)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index b4d4be3cc987..9efe89bbcec8 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -44,11 +44,13 @@ struct damon_region {
>  	struct damon_addr_range ar;
>  	unsigned long sampling_addr;
>  	unsigned int nr_accesses;
> +	unsigned int nr_writes;
>  	struct list_head list;
>  
>  	unsigned int age;
>  /* private: Internal value for age calculation. */
>  	unsigned int last_nr_accesses;
> +	unsigned int last_nr_writes;
>  };
>  
>  /**
> @@ -88,6 +90,8 @@ enum damos_action {
>  	DAMOS_PAGEOUT,
>  	DAMOS_HUGEPAGE,
>  	DAMOS_NOHUGEPAGE,
> +	DAMOS_MERGEABLE,
> +	DAMOS_UNMERGEABLE,
>  	DAMOS_STAT,		/* Do nothing but only record the stat */
>  };
>  
> @@ -185,6 +189,11 @@ struct damos_watermarks {
>  	bool activated;
>  };
>  
> +enum damos_counter_type {
> +	DAMOS_NUMBER_ACCESSES,
> +	DAMOS_NUMBER_WRITES,
> +};
> +
>  /**
>   * struct damos - Represents a Data Access Monitoring-based Operation Scheme.
>   * @min_sz_region:	Minimum size of target regions.
> @@ -223,6 +232,9 @@ struct damos {
>  	unsigned long max_sz_region;
>  	unsigned int min_nr_accesses;
>  	unsigned int max_nr_accesses;
> +	unsigned int min_nr_writes;
> +	unsigned int max_nr_writes;
> +	enum damos_counter_type counter_type;
>  	unsigned int min_age_region;
>  	unsigned int max_age_region;
>  	enum damos_action action;
> @@ -386,6 +398,7 @@ struct damon_ctx {
>  	struct damon_primitive primitive;
>  	struct damon_callback callback;
>  
> +	enum damos_counter_type counter_type;
>  	unsigned long min_nr_regions;
>  	unsigned long max_nr_regions;
>  	struct list_head adaptive_targets;
> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> index 2f422f4f1fb9..45573f421707 100644
> --- a/include/trace/events/damon.h
> +++ b/include/trace/events/damon.h
> @@ -11,17 +11,17 @@
>  
>  TRACE_EVENT(damon_aggregated,
>  
> -	TP_PROTO(struct damon_target *t, struct damon_region *r,
> +	TP_PROTO(struct damon_ctx *c, struct damon_target *t, struct damon_region *r,
>  		unsigned int nr_regions),
>  
> -	TP_ARGS(t, r, nr_regions),
> +	TP_ARGS(c, t, r, nr_regions),
>  
>  	TP_STRUCT__entry(
>  		__field(unsigned long, target_id)
>  		__field(unsigned int, nr_regions)
>  		__field(unsigned long, start)
>  		__field(unsigned long, end)
> -		__field(unsigned int, nr_accesses)
> +		__field(unsigned int, nr)
>  	),
>  
>  	TP_fast_assign(
> @@ -29,12 +29,12 @@ TRACE_EVENT(damon_aggregated,
>  		__entry->nr_regions = nr_regions;
>  		__entry->start = r->ar.start;
>  		__entry->end = r->ar.end;
> -		__entry->nr_accesses = r->nr_accesses;
> +		__entry->nr = c->counter_type == DAMOS_NUMBER_WRITES ? r->nr_writes : r->nr_accesses;
>  	),
>  
>  	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u",
>  			__entry->target_id, __entry->nr_regions,
> -			__entry->start, __entry->end, __entry->nr_accesses)
> +			__entry->start, __entry->end, __entry->nr)
>  );
>  
>  #endif /* _TRACE_DAMON_H */
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index e92497895202..e827231366db 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -45,10 +45,12 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
>  	region->ar.start = start;
>  	region->ar.end = end;
>  	region->nr_accesses = 0;
> +	region->nr_writes = 0;
>  	INIT_LIST_HEAD(&region->list);
>  
>  	region->age = 0;
>  	region->last_nr_accesses = 0;
> +	region->last_nr_writes = 0;
>  
>  	return region;
>  }
> @@ -89,7 +91,7 @@ void damon_destroy_region(struct damon_region *r, struct damon_target *t)
>  
>  struct damos *damon_new_scheme(
>  		unsigned long min_sz_region, unsigned long max_sz_region,
> -		unsigned int min_nr_accesses, unsigned int max_nr_accesses,
> +		unsigned int min_nr, unsigned int max_nr,
>  		unsigned int min_age_region, unsigned int max_age_region,
>  		enum damos_action action, struct damos_quota *quota,
>  		struct damos_watermarks *wmarks)
> @@ -101,8 +103,10 @@ struct damos *damon_new_scheme(
>  		return NULL;
>  	scheme->min_sz_region = min_sz_region;
>  	scheme->max_sz_region = max_sz_region;
> -	scheme->min_nr_accesses = min_nr_accesses;
> -	scheme->max_nr_accesses = max_nr_accesses;
> +	scheme->max_nr_writes = max_nr;
> +	scheme->min_nr_writes = min_nr;
> +	scheme->min_nr_accesses = min_nr;
> +	scheme->max_nr_accesses = max_nr;
>  	scheme->min_age_region = min_age_region;
>  	scheme->max_age_region = max_age_region;
>  	scheme->action = action;
> @@ -221,6 +225,7 @@ struct damon_ctx *damon_new_ctx(void)
>  	ctx->sample_interval = 5 * 1000;
>  	ctx->aggr_interval = 100 * 1000;
>  	ctx->primitive_update_interval = 60 * 1000 * 1000;
> +	ctx->counter_type = DAMOS_NUMBER_ACCESSES;
>  
>  	ktime_get_coarse_ts64(&ctx->last_aggregation);
>  	ctx->last_primitive_update = ctx->last_aggregation;
> @@ -535,9 +540,11 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
>  		struct damon_region *r;
>  
>  		damon_for_each_region(r, t) {
> -			trace_damon_aggregated(t, r, damon_nr_regions(t));
> +			trace_damon_aggregated(c, t, r, damon_nr_regions(t));
>  			r->last_nr_accesses = r->nr_accesses;
>  			r->nr_accesses = 0;
> +			r->last_nr_writes = r->nr_writes;
> +			r->nr_writes = 0;
>  		}
>  	}
>  }
> @@ -546,21 +553,27 @@ static void damon_split_region_at(struct damon_ctx *ctx,
>  		struct damon_target *t, struct damon_region *r,
>  		unsigned long sz_r);
>  
> -static bool __damos_valid_target(struct damon_region *r, struct damos *s)
> +static bool __damos_valid_target(struct damon_ctx *ctx, struct damon_region *r, struct damos *s)
>  {
>  	unsigned long sz;
> -
>  	sz = r->ar.end - r->ar.start;
> -	return s->min_sz_region <= sz && sz <= s->max_sz_region &&
> -		s->min_nr_accesses <= r->nr_accesses &&
> -		r->nr_accesses <= s->max_nr_accesses &&
> -		s->min_age_region <= r->age && r->age <= s->max_age_region;
> +
> +	if (ctx->counter_type == DAMOS_NUMBER_WRITES)
> +		return s->min_sz_region <= sz && sz <= s->max_sz_region &&
> +			s->min_nr_writes <= r->nr_writes &&
> +			r->nr_writes <= s->max_nr_writes &&
> +			s->min_age_region <= r->age && r->age <= s->max_age_region;
> +	else
> +		return s->min_sz_region <= sz && sz <= s->max_sz_region &&
> +			s->min_nr_accesses <= r->nr_accesses &&
> +			r->nr_accesses <= s->max_nr_accesses &&
> +			s->min_age_region <= r->age && r->age <= s->max_age_region;
>  }
>  
>  static bool damos_valid_target(struct damon_ctx *c, struct damon_target *t,
>  		struct damon_region *r, struct damos *s)
>  {
> -	bool ret = __damos_valid_target(r, s);
> +	bool ret = __damos_valid_target(c, r, s);
>  
>  	if (!ret || !s->quota.esz || !c->primitive.get_scheme_score)
>  		return ret;
> @@ -707,7 +720,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
>  		memset(quota->histogram, 0, sizeof(quota->histogram));
>  		damon_for_each_target(t, c) {
>  			damon_for_each_region(r, t) {
> -				if (!__damos_valid_target(r, s))
> +				if (!__damos_valid_target(c, r, s))
>  					continue;
>  				score = c->primitive.get_scheme_score(
>  						c, t, r, s);
> @@ -738,13 +751,18 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
>  /*
>   * Merge two adjacent regions into one region
>   */
> -static void damon_merge_two_regions(struct damon_target *t,
> +static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
>  		struct damon_region *l, struct damon_region *r)
>  {
>  	unsigned long sz_l = sz_damon_region(l), sz_r = sz_damon_region(r);
>  
> -	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
> +	if (c->counter_type == DAMOS_NUMBER_WRITES)
> +		l->nr_writes = (l->nr_writes * sz_l + r->nr_writes * sz_r) /
>  			(sz_l + sz_r);
> +	else
> +		l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
> +				(sz_l + sz_r);
> +
>  	l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
>  	l->ar.end = r->ar.end;
>  	damon_destroy_region(r, t);
> @@ -759,23 +777,39 @@ static void damon_merge_two_regions(struct damon_target *t,
>   * thres	'->nr_accesses' diff threshold for the merge
>   * sz_limit	size upper limit of each region
>   */
> -static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
> +static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
>  				   unsigned long sz_limit)
>  {
>  	struct damon_region *r, *prev = NULL, *next;
>  
> -	damon_for_each_region_safe(r, next, t) {
> -		if (diff_of(r->nr_accesses, r->last_nr_accesses) > thres)
> -			r->age = 0;
> -		else
> -			r->age++;
> -
> -		if (prev && prev->ar.end == r->ar.start &&
> -		    diff_of(prev->nr_accesses, r->nr_accesses) <= thres &&
> -		    sz_damon_region(prev) + sz_damon_region(r) <= sz_limit)
> -			damon_merge_two_regions(t, prev, r);
> -		else
> -			prev = r;
> +	if (c->counter_type == DAMOS_NUMBER_WRITES) {
> +		damon_for_each_region_safe(r, next, t) {
> +			if (diff_of(r->nr_writes, r->last_nr_writes) > thres)
> +				r->age = 0;
> +			else
> +				r->age++;
> +
> +			if (prev && prev->ar.end == r->ar.start &&
> +				diff_of(prev->nr_writes, r->nr_writes) <= thres &&
> +				sz_damon_region(prev) + sz_damon_region(r) <= sz_limit)
> +				damon_merge_two_regions(c, t, prev, r);
> +			else
> +				prev = r;
> +		}
> +	} else {
> +		damon_for_each_region_safe(r, next, t) {
> +			if (diff_of(r->nr_accesses, r->last_nr_accesses) > thres)
> +				r->age = 0;
> +			else
> +				r->age++;
> +
> +			if (prev && prev->ar.end == r->ar.start &&
> +				diff_of(prev->nr_accesses, r->nr_accesses) <= thres &&
> +				sz_damon_region(prev) + sz_damon_region(r) <= sz_limit)
> +				damon_merge_two_regions(c, t, prev, r);
> +			else
> +				prev = r;
> +		}
>  	}
>  }
>  
> @@ -796,7 +830,7 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
>  	struct damon_target *t;
>  
>  	damon_for_each_target(t, c)
> -		damon_merge_regions_of(t, threshold, sz_limit);
> +		damon_merge_regions_of(c, t, threshold, sz_limit);
>  }
>  
>  /*
> @@ -819,6 +853,7 @@ static void damon_split_region_at(struct damon_ctx *ctx,
>  
>  	new->age = r->age;
>  	new->last_nr_accesses = r->last_nr_accesses;
> +	new->last_nr_writes = r->last_nr_writes;
>  
>  	damon_insert_region(new, r, damon_next_region(r), t);
>  }
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index ad65436756af..6cf2501148f5 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -166,6 +166,8 @@ static bool damos_action_valid(int action)
>  	case DAMOS_PAGEOUT:
>  	case DAMOS_HUGEPAGE:
>  	case DAMOS_NOHUGEPAGE:
> +	case DAMOS_MERGEABLE:
> +	case DAMOS_UNMERGEABLE:
>  	case DAMOS_STAT:
>  		return true;
>  	default:
> @@ -477,6 +479,66 @@ static ssize_t dbgfs_init_regions_read(struct file *file, char __user *buf,
>  	return len;
>  }
>  
> +static ssize_t dbgfs_counter_type_write(struct file *file,
> +		const char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct damon_ctx *ctx = file->private_data;
> +	char *kbuf;
> +	ssize_t ret;
> +
> +	mutex_lock(&ctx->kdamond_lock);
> +	if (ctx->kdamond) {
> +		mutex_unlock(&ctx->kdamond_lock);
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +	ret = count;
> +	kbuf = user_input_str(buf, count, ppos);
> +	if (IS_ERR(kbuf))
> +		return PTR_ERR(kbuf);
> +
> +	if (!strncmp(kbuf, "writes\n", count))
> +		ctx->counter_type = DAMOS_NUMBER_WRITES;
> +	else
> +		ctx->counter_type = DAMOS_NUMBER_ACCESSES;
> +
> +	mutex_unlock(&ctx->kdamond_lock);
> +out:
> +	kfree(kbuf);
> +	return ret;
> +}
> +
> +static ssize_t dbgfs_counter_type_read(struct file *file, char __user *buf,
> +		size_t count, loff_t *ppos)
> +{
> +	struct damon_ctx *ctx = file->private_data;
> +	char *kbuf;
> +	ssize_t len;
> +
> +	kbuf = kmalloc(count, GFP_KERNEL | __GFP_NOWARN);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	mutex_lock(&ctx->kdamond_lock);
> +	if (ctx->kdamond) {
> +		mutex_unlock(&ctx->kdamond_lock);
> +		len = -EBUSY;
> +		goto out;
> +	}
> +
> +	if (ctx->counter_type == DAMOS_NUMBER_WRITES)
> +		len = scnprintf(kbuf, count, "writes");
> +	else
> +		len = scnprintf(kbuf, count, "accesses");
> +	mutex_unlock(&ctx->kdamond_lock);
> +	len = simple_read_from_buffer(buf, count, ppos, kbuf, len);
> +
> +out:
> +	kfree(kbuf);
> +	return len;
> +}
> +
> +
>  static int add_init_region(struct damon_ctx *c,
>  			 unsigned long target_id, struct damon_addr_range *ar)
>  {
> @@ -636,12 +698,18 @@ static const struct file_operations kdamond_pid_fops = {
>  	.read = dbgfs_kdamond_pid_read,
>  };
>  
> +static const struct file_operations counter_type_fops = {
> +	.open = damon_dbgfs_open,
> +	.read = dbgfs_counter_type_read,
> +	.write = dbgfs_counter_type_write,
> +};
> +
>  static void dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
>  {
>  	const char * const file_names[] = {"attrs", "schemes", "target_ids",
> -		"init_regions", "kdamond_pid"};
> +		"init_regions", "kdamond_pid", "counter_type"};
>  	const struct file_operations *fops[] = {&attrs_fops, &schemes_fops,
> -		&target_ids_fops, &init_regions_fops, &kdamond_pid_fops};
> +		&target_ids_fops, &init_regions_fops, &kdamond_pid_fops, &counter_type_fops};
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(file_names); i++)
> diff --git a/mm/damon/prmtv-common.c b/mm/damon/prmtv-common.c
> index 92a04f5831d6..09ba2b5b895e 100644
> --- a/mm/damon/prmtv-common.c
> +++ b/mm/damon/prmtv-common.c
> @@ -9,6 +9,8 @@
>  #include <linux/page_idle.h>
>  #include <linux/pagemap.h>
>  #include <linux/rmap.h>
> +#include <linux/swap.h>
> +#include <linux/swapops.h>
>  
>  #include "prmtv-common.h"
>  
> @@ -58,6 +60,92 @@ void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr)
>  	put_page(page);
>  }
>  
> +static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +{
> +	struct page *page;
> +
> +	if (!pte_write(pte))
> +		return false;
> +	if (!is_cow_mapping(vma->vm_flags))
> +		return false;
> +	if (likely(!test_bit(MMF_HAS_PINNED, &vma->vm_mm->flags)))
> +		return false;
> +	page = vm_normal_page(vma, addr, pte);
> +	if (!page)
> +		return false;
> +	return page_maybe_dma_pinned(page);
> +}
> +
> +static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> +		unsigned long addr, pmd_t *pmdp)
> +{
> +	pmd_t old, pmd = *pmdp;
> +
> +	if (pmd_present(pmd)) {
> +		/* See comment in change_huge_pmd() */
> +		old = pmdp_invalidate(vma, addr, pmdp);
> +		if (pmd_dirty(old))
> +			pmd = pmd_mkdirty(pmd);
> +		if (pmd_young(old))
> +			pmd = pmd_mkyoung(pmd);
> +
> +		pmd = pmd_wrprotect(pmd);
> +		pmd = pmd_clear_soft_dirty(pmd);
> +
> +		set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
> +	} else if (is_migration_entry(pmd_to_swp_entry(pmd))) {
> +		pmd = pmd_swp_clear_soft_dirty(pmd);
> +		set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
> +	}
> +}
> +
> +static inline void clear_soft_dirty(struct vm_area_struct *vma,
> +		unsigned long addr, pte_t *pte)
> +{
> +	/*
> +	 * The soft-dirty tracker uses #PF-s to catch writes
> +	 * to pages, so write-protect the pte as well. See the
> +	 * Documentation/admin-guide/mm/soft-dirty.rst for full description
> +	 * of how soft-dirty works.
> +	 */
> +	pte_t ptent = *pte;
> +
> +	if (pte_present(ptent)) {
> +		pte_t old_pte;
> +
> +		if (pte_is_pinned(vma, addr, ptent))
> +			return;
> +		old_pte = ptep_modify_prot_start(vma, addr, pte);
> +		ptent = pte_wrprotect(old_pte);
> +		ptent = pte_clear_soft_dirty(ptent);
> +		ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
> +	} else if (is_swap_pte(ptent)) {
> +		ptent = pte_swp_clear_soft_dirty(ptent);
> +		set_pte_at(vma->vm_mm, addr, pte, ptent);
> +	}
> +}
> +
> +void damon_pmd_clean_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> +		pmd_t *pmd)
> +{
> +	vma->vm_flags &= ~VM_SOFTDIRTY;
> +	vma_set_page_prot(vma);
> +
> +	if (pmd_soft_dirty(*pmd))
> +		clear_soft_dirty_pmd(vma, addr, pmd);
> +
> +}
> +
> +void damon_ptep_clean_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> +		pte_t *pte)
> +{
> +	vma->vm_flags &= ~VM_SOFTDIRTY;
> +	vma_set_page_prot(vma);
> +
> +	if (pte_soft_dirty(*pte))
> +		clear_soft_dirty(vma, addr, pte);
> +}
> +
>  void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr)
>  {
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> diff --git a/mm/damon/prmtv-common.h b/mm/damon/prmtv-common.h
> index 61f27037603e..03847d149f0e 100644
> --- a/mm/damon/prmtv-common.h
> +++ b/mm/damon/prmtv-common.h
> @@ -13,6 +13,11 @@
>  
>  struct page *damon_get_page(unsigned long pfn);
>  
> +void damon_ptep_clean_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> +		pte_t *pte);
> +void damon_pmd_clean_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> +		pmd_t *pmd);
> +
>  void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr);
>  void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr);
>  
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 20a9a9d69eb1..a7d9c9563d1d 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -368,6 +368,44 @@ void damon_va_update(struct damon_ctx *ctx)
>  	}
>  }
>  
> +static int damon_clean_soft_dirty_pmd_entry(pmd_t *pmd, unsigned long addr,
> +		unsigned long next, struct mm_walk *walk)
> +{
> +	pte_t *pte;
> +	spinlock_t *ptl;
> +
> +	if (pmd_huge(*pmd)) {
> +		ptl = pmd_lock(walk->mm, pmd);
> +		if (pmd_huge(*pmd)) {
> +			damon_pmd_clean_soft_dirty(walk->vma, addr, pmd);
> +			spin_unlock(ptl);
> +			return 0;
> +		}
> +		spin_unlock(ptl);
> +	}
> +
> +	if (pmd_none(*pmd) || unlikely(pmd_bad(*pmd)))
> +		return 0;
> +	pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl);
> +	if (!pte_present(*pte))
> +		goto out;
> +	damon_ptep_clean_soft_dirty(walk->vma, addr, pte);
> +out:
> +	pte_unmap_unlock(pte, ptl);
> +	return 0;
> +}
> +
> +static const struct mm_walk_ops damon_clean_soft_dirty_ops = {
> +	.pmd_entry = damon_clean_soft_dirty_pmd_entry,
> +};
> +
> +static void damon_va_clean_soft_dirty(struct mm_struct *mm, unsigned long addr)
> +{
> +	mmap_read_lock(mm);
> +	walk_page_range(mm, addr, addr + 1, &damon_clean_soft_dirty_ops, NULL);
> +	mmap_read_unlock(mm);
> +}
> +
>  static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		unsigned long next, struct mm_walk *walk)
>  {
> @@ -415,7 +453,10 @@ static void damon_va_prepare_access_check(struct damon_ctx *ctx,
>  {
>  	r->sampling_addr = damon_rand(r->ar.start, r->ar.end);
>  
> -	damon_va_mkold(mm, r->sampling_addr);
> +	if (ctx->counter_type == DAMOS_NUMBER_WRITES)
> +		damon_va_clean_soft_dirty(mm, r->sampling_addr);
> +	else
> +		damon_va_mkold(mm, r->sampling_addr);
>  }
>  
>  void damon_va_prepare_access_checks(struct damon_ctx *ctx)
> @@ -437,6 +478,7 @@ void damon_va_prepare_access_checks(struct damon_ctx *ctx)
>  struct damon_young_walk_private {
>  	unsigned long *page_sz;
>  	bool young;
> +	bool dirty;
>  };
>  
>  static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
> @@ -463,6 +505,10 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
>  			*priv->page_sz = ((1UL) << HPAGE_PMD_SHIFT);
>  			priv->young = true;
>  		}
> +		if (pmd_soft_dirty(*pmd)) {
> +			*priv->page_sz = ((1UL) << HPAGE_PMD_SHIFT);
> +			priv->dirty = true;
> +		}
>  		put_page(page);
>  huge_out:
>  		spin_unlock(ptl);
> @@ -485,6 +531,10 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		*priv->page_sz = PAGE_SIZE;
>  		priv->young = true;
>  	}
> +	if (pte_soft_dirty(*pte)) {
> +		*priv->page_sz = PAGE_SIZE;
> +		priv->dirty = true;
> +	}
>  	put_page(page);
>  out:
>  	pte_unmap_unlock(pte, ptl);
> @@ -496,16 +546,19 @@ static const struct mm_walk_ops damon_young_ops = {
>  };
>  
>  static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
> -		unsigned long *page_sz)
> +		unsigned long *page_sz, bool *dirty)
>  {
>  	struct damon_young_walk_private arg = {
>  		.page_sz = page_sz,
>  		.young = false,
> +		.dirty = false,
>  	};
>  
>  	mmap_read_lock(mm);
>  	walk_page_range(mm, addr, addr + 1, &damon_young_ops, &arg);
>  	mmap_read_unlock(mm);
> +
> +	*dirty = arg.dirty;
>  	return arg.young;
>  }
>  
> @@ -522,18 +575,23 @@ static void damon_va_check_access(struct damon_ctx *ctx,
>  	static unsigned long last_addr;
>  	static unsigned long last_page_sz = PAGE_SIZE;
>  	static bool last_accessed;
> +	static bool last_written;
>  
>  	/* If the region is in the last checked page, reuse the result */
>  	if (mm == last_mm && (ALIGN_DOWN(last_addr, last_page_sz) ==
>  				ALIGN_DOWN(r->sampling_addr, last_page_sz))) {
>  		if (last_accessed)
>  			r->nr_accesses++;
> +		if (last_written)
> +			r->nr_writes++;
>  		return;
>  	}
>  
> -	last_accessed = damon_va_young(mm, r->sampling_addr, &last_page_sz);
> +	last_accessed = damon_va_young(mm, r->sampling_addr, &last_page_sz, &last_written);
>  	if (last_accessed)
>  		r->nr_accesses++;
> +	if (last_written)
> +		r->nr_writes++;
>  
>  	last_mm = mm;
>  	last_addr = r->sampling_addr;
> @@ -544,7 +602,7 @@ unsigned int damon_va_check_accesses(struct damon_ctx *ctx)
>  	struct damon_target *t;
>  	struct mm_struct *mm;
>  	struct damon_region *r;
> -	unsigned int max_nr_accesses = 0;
> +	unsigned int max_nr = 0;
>  
>  	damon_for_each_target(t, ctx) {
>  		mm = damon_get_mm(t);
> @@ -552,12 +610,15 @@ unsigned int damon_va_check_accesses(struct damon_ctx *ctx)
>  			continue;
>  		damon_for_each_region(r, t) {
>  			damon_va_check_access(ctx, mm, r);
> -			max_nr_accesses = max(r->nr_accesses, max_nr_accesses);
> +			if (ctx->counter_type == DAMOS_NUMBER_WRITES)
> +				max_nr = max(r->nr_writes, max_nr);
> +			else
> +				max_nr = max(r->nr_accesses, max_nr);
>  		}
>  		mmput(mm);
>  	}
>  
> -	return max_nr_accesses;
> +	return max_nr;
>  }
>  
>  /*
> @@ -597,6 +658,7 @@ static int damos_madvise(struct damon_target *target, struct damon_region *r,
>  
>  	ret = do_madvise(mm, PAGE_ALIGN(r->ar.start),
>  			PAGE_ALIGN(r->ar.end - r->ar.start), behavior);
> +
>  	mmput(mm);
>  out:
>  	return ret;
> @@ -624,6 +686,12 @@ int damon_va_apply_scheme(struct damon_ctx *ctx, struct damon_target *t,
>  	case DAMOS_NOHUGEPAGE:
>  		madv_action = MADV_NOHUGEPAGE;
>  		break;
> +	case DAMOS_MERGEABLE:
> +		madv_action = MADV_MERGEABLE;
> +		break;
> +	case DAMOS_UNMERGEABLE:
> +		madv_action = MADV_UNMERGEABLE;
> +		break;
>  	case DAMOS_STAT:
>  		return 0;
>  	default:
> -- 
> 2.25.1

Re: [PATCH] mm/damon: Add option to monitor only writes

[Pedro Gomes]

On Thu, Feb 3, 2022 at 10:39 AM SeongJae Park <sj@kernel.org> wrote:

> I think this would be better to be implemented as another monitoring primitive
> based on the virtual address space monitoring primitives, e.g., vaddr-writes?
> Then the implementation would be simpler and changes to other files will be
> minimized.  For the user space interface, we could use a prefix to target_ids
> debugfs file.  For example,
>
>     # echo "vaddr-writes $(pidof workload)" > $debugfs/damon/target_ids

I will do that.

> > This patch also adds the actions mergeable and unmergeable to damos schemes.
> > These actions are used by KSM as explained in [1].
>
> Please do that in a separate patch, and also update the document
> (Documentation/admin-guide/mm/damon/usage.rst).  And, what's the expected usage
> of the action and benefits?

The idea is to use this action to all areas that are not written too frequently,
this way KSM can save more memory without too much overhead.
But I have to test it better and collect some data to see if it really
makes sense,
perhaps it is better to leave this patch for later.
I would like to know your opinion on this, do you think it makes sense?


-- 
Atenciosamente,
Pedro Demarchi Gomes.

Re: [PATCH] mm/damon: Add option to monitor only writes

[SeongJae Park]

On Fri, 4 Feb 2022 12:11:44 -0300 Pedro Gomes <pedrodemargomes@gmail.com> wrote:

> On Thu, Feb 3, 2022 at 10:39 AM SeongJae Park <sj@kernel.org> wrote:
> 
> > I think this would be better to be implemented as another monitoring primitive
> > based on the virtual address space monitoring primitives, e.g., vaddr-writes?
> > Then the implementation would be simpler and changes to other files will be
> > minimized.  For the user space interface, we could use a prefix to target_ids
> > debugfs file.  For example,
> >
> >     # echo "vaddr-writes $(pidof workload)" > $debugfs/damon/target_ids
> 
> I will do that.

Thanks!

> 
> > > This patch also adds the actions mergeable and unmergeable to damos schemes.
> > > These actions are used by KSM as explained in [1].
> >
> > Please do that in a separate patch, and also update the document
> > (Documentation/admin-guide/mm/damon/usage.rst).  And, what's the expected usage
> > of the action and benefits?
> 
> The idea is to use this action to all areas that are not written too frequently,
> this way KSM can save more memory without too much overhead.
> But I have to test it better and collect some data to see if it really
> makes sense,
> perhaps it is better to leave this patch for later.
> I would like to know your opinion on this, do you think it makes sense?

Yes, that idea makes sense to me :)


Thanks,
SJ

> 
> 
> -- 
> Atenciosamente,
> Pedro Demarchi Gomes.

Re: [PATCH] mm/damon: Add option to monitor only writes

[kernel test robot]

Hi Pedro,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hnaz-mm/master]

url:    https://github.com/0day-ci/linux/commits/Pedro-Demarchi-Gomes/mm-damon-Add-option-to-monitor-only-writes/20220203-211407
base:   https://github.com/hnaz/linux-mm master
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220204/202202040109.d4u6Jwvm-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0958bb7e2514b62f174b8e5ccfdcff07ce2a2b6b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pedro-Demarchi-Gomes/mm-damon-Add-option-to-monitor-only-writes/20220203-211407
        git checkout 0958bb7e2514b62f174b8e5ccfdcff07ce2a2b6b
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash mm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h: In function 'damon_test_merge_two':
   mm/damon/core-test.h:154:33: error: passing argument 1 of 'damon_merge_two_regions' from incompatible pointer type [-Werror=incompatible-pointer-types]
     154 |         damon_merge_two_regions(t, r, r2);
         |                                 ^
         |                                 |
         |                                 struct damon_target *
   mm/damon/core.c:761:55: note: expected 'struct damon_ctx *' but argument is of type 'struct damon_target *'
     761 | static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
         |                                     ~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h:154:36: error: passing argument 2 of 'damon_merge_two_regions' from incompatible pointer type [-Werror=incompatible-pointer-types]
     154 |         damon_merge_two_regions(t, r, r2);
         |                                    ^
         |                                    |
         |                                    struct damon_region *
   mm/damon/core.c:761:79: note: expected 'struct damon_target *' but argument is of type 'struct damon_region *'
     761 | static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
         |                                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h:154:9: error: too few arguments to function 'damon_merge_two_regions'
     154 |         damon_merge_two_regions(t, r, r2);
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   mm/damon/core.c:761:13: note: declared here
     761 | static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
         |             ^~~~~~~~~~~~~~~~~~~~~~~
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h: In function 'damon_test_merge_regions_of':
   mm/damon/core-test.h:201:32: error: passing argument 1 of 'damon_merge_regions_of' from incompatible pointer type [-Werror=incompatible-pointer-types]
     201 |         damon_merge_regions_of(t, 9, 9999);
         |                                ^
         |                                |
         |                                struct damon_target *
   mm/damon/core.c:787:54: note: expected 'struct damon_ctx *' but argument is of type 'struct damon_target *'
     787 | static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
         |                                    ~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
>> mm/damon/core-test.h:201:35: warning: passing argument 2 of 'damon_merge_regions_of' makes pointer from integer without a cast [-Wint-conversion]
     201 |         damon_merge_regions_of(t, 9, 9999);
         |                                   ^
         |                                   |
         |                                   int
   mm/damon/core.c:787:78: note: expected 'struct damon_target *' but argument is of type 'int'
     787 | static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
         |                                                         ~~~~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h:201:9: error: too few arguments to function 'damon_merge_regions_of'
     201 |         damon_merge_regions_of(t, 9, 9999);
         |         ^~~~~~~~~~~~~~~~~~~~~~
   mm/damon/core.c:787:13: note: declared here
     787 | static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
         |             ^~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/trace_events.h:21,
                    from include/trace/define_trace.h:102,
                    from include/trace/events/damon.h:43,
                    from mm/damon/core.c:19:
   At top level:
   arch/arc/include/asm/perf_event.h:126:27: warning: 'arc_pmu_cache_map' defined but not used [-Wunused-const-variable=]
     126 | static const unsigned int arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
         |                           ^~~~~~~~~~~~~~~~~
   arch/arc/include/asm/perf_event.h:91:27: warning: 'arc_pmu_ev_hw_map' defined but not used [-Wunused-const-variable=]
      91 | static const char * const arc_pmu_ev_hw_map[] = {
         |                           ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/damon_merge_regions_of +201 mm/damon/core-test.h

17ccae8bb5c9289 SeongJae Park 2021-09-07  181  
17ccae8bb5c9289 SeongJae Park 2021-09-07  182  static void damon_test_merge_regions_of(struct kunit *test)
17ccae8bb5c9289 SeongJae Park 2021-09-07  183  {
17ccae8bb5c9289 SeongJae Park 2021-09-07  184  	struct damon_target *t;
17ccae8bb5c9289 SeongJae Park 2021-09-07  185  	struct damon_region *r;
17ccae8bb5c9289 SeongJae Park 2021-09-07  186  	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184};
17ccae8bb5c9289 SeongJae Park 2021-09-07  187  	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230};
17ccae8bb5c9289 SeongJae Park 2021-09-07  188  	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2};
17ccae8bb5c9289 SeongJae Park 2021-09-07  189  
17ccae8bb5c9289 SeongJae Park 2021-09-07  190  	unsigned long saddrs[] = {0, 114, 130, 156, 170};
17ccae8bb5c9289 SeongJae Park 2021-09-07  191  	unsigned long eaddrs[] = {112, 130, 156, 170, 230};
17ccae8bb5c9289 SeongJae Park 2021-09-07  192  	int i;
17ccae8bb5c9289 SeongJae Park 2021-09-07  193  
17ccae8bb5c9289 SeongJae Park 2021-09-07  194  	t = damon_new_target(42);
17ccae8bb5c9289 SeongJae Park 2021-09-07  195  	for (i = 0; i < ARRAY_SIZE(sa); i++) {
17ccae8bb5c9289 SeongJae Park 2021-09-07  196  		r = damon_new_region(sa[i], ea[i]);
17ccae8bb5c9289 SeongJae Park 2021-09-07  197  		r->nr_accesses = nrs[i];
17ccae8bb5c9289 SeongJae Park 2021-09-07  198  		damon_add_region(r, t);
17ccae8bb5c9289 SeongJae Park 2021-09-07  199  	}
17ccae8bb5c9289 SeongJae Park 2021-09-07  200  
17ccae8bb5c9289 SeongJae Park 2021-09-07 @201  	damon_merge_regions_of(t, 9, 9999);
17ccae8bb5c9289 SeongJae Park 2021-09-07  202  	/* 0-112, 114-130, 130-156, 156-170 */
17ccae8bb5c9289 SeongJae Park 2021-09-07  203  	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
17ccae8bb5c9289 SeongJae Park 2021-09-07  204  	for (i = 0; i < 5; i++) {
17ccae8bb5c9289 SeongJae Park 2021-09-07  205  		r = __nth_region_of(t, i);
17ccae8bb5c9289 SeongJae Park 2021-09-07  206  		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
17ccae8bb5c9289 SeongJae Park 2021-09-07  207  		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
17ccae8bb5c9289 SeongJae Park 2021-09-07  208  	}
17ccae8bb5c9289 SeongJae Park 2021-09-07  209  	damon_free_target(t);
17ccae8bb5c9289 SeongJae Park 2021-09-07  210  }
17ccae8bb5c9289 SeongJae Park 2021-09-07  211  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] mm/damon: Add option to monitor only writes

[kernel test robot]

Hi Pedro,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hnaz-mm/master]

url:    https://github.com/0day-ci/linux/commits/Pedro-Demarchi-Gomes/mm-damon-Add-option-to-monitor-only-writes/20220203-211407
base:   https://github.com/hnaz/linux-mm master
config: hexagon-randconfig-r041-20220130 (https://download.01.org/0day-ci/archive/20220204/202202040414.4fF2j3mp-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a73e4ce6a59b01f0e37037761c1e6889d539d233)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0958bb7e2514b62f174b8e5ccfdcff07ce2a2b6b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pedro-Demarchi-Gomes/mm-damon-Add-option-to-monitor-only-writes/20220203-211407
        git checkout 0958bb7e2514b62f174b8e5ccfdcff07ce2a2b6b
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> mm/damon/prmtv-common.c:87:7: error: implicit declaration of function 'pmd_dirty' [-Werror,-Wimplicit-function-declaration]
                   if (pmd_dirty(old))
                       ^
   mm/damon/prmtv-common.c:87:7: note: did you mean 'pte_dirty'?
   arch/hexagon/include/asm/pgtable.h:301:19: note: 'pte_dirty' declared here
   static inline int pte_dirty(pte_t pte)
                     ^
>> mm/damon/prmtv-common.c:88:10: error: implicit declaration of function 'pmd_mkdirty' [-Werror,-Wimplicit-function-declaration]
                           pmd = pmd_mkdirty(pmd);
                                 ^
>> mm/damon/prmtv-common.c:88:8: error: assigning to 'pmd_t' from incompatible type 'int'
                           pmd = pmd_mkdirty(pmd);
                               ^ ~~~~~~~~~~~~~~~~
>> mm/damon/prmtv-common.c:89:7: error: implicit declaration of function 'pmd_young' [-Werror,-Wimplicit-function-declaration]
                   if (pmd_young(old))
                       ^
   mm/damon/prmtv-common.c:89:7: note: did you mean 'pte_young'?
   arch/hexagon/include/asm/pgtable.h:295:19: note: 'pte_young' declared here
   static inline int pte_young(pte_t pte)
                     ^
>> mm/damon/prmtv-common.c:90:10: error: implicit declaration of function 'pmd_mkyoung' [-Werror,-Wimplicit-function-declaration]
                           pmd = pmd_mkyoung(pmd);
                                 ^
   mm/damon/prmtv-common.c:90:8: error: assigning to 'pmd_t' from incompatible type 'int'
                           pmd = pmd_mkyoung(pmd);
                               ^ ~~~~~~~~~~~~~~~~
>> mm/damon/prmtv-common.c:92:9: error: implicit declaration of function 'pmd_wrprotect' [-Werror,-Wimplicit-function-declaration]
                   pmd = pmd_wrprotect(pmd);
                         ^
   mm/damon/prmtv-common.c:92:9: note: did you mean 'pte_wrprotect'?
   arch/hexagon/include/asm/pgtable.h:315:21: note: 'pte_wrprotect' declared here
   static inline pte_t pte_wrprotect(pte_t pte)
                       ^
   mm/damon/prmtv-common.c:92:7: error: assigning to 'pmd_t' from incompatible type 'int'
                   pmd = pmd_wrprotect(pmd);
                       ^ ~~~~~~~~~~~~~~~~~~
>> mm/damon/prmtv-common.c:95:3: error: implicit declaration of function 'set_pmd_at' [-Werror,-Wimplicit-function-declaration]
                   set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
                   ^
   mm/damon/prmtv-common.c:98:3: error: implicit declaration of function 'set_pmd_at' [-Werror,-Wimplicit-function-declaration]
                   set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
                   ^
   10 errors generated.


vim +/pmd_dirty +87 mm/damon/prmtv-common.c

    78	
    79	static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
    80			unsigned long addr, pmd_t *pmdp)
    81	{
    82		pmd_t old, pmd = *pmdp;
    83	
    84		if (pmd_present(pmd)) {
    85			/* See comment in change_huge_pmd() */
    86			old = pmdp_invalidate(vma, addr, pmdp);
  > 87			if (pmd_dirty(old))
  > 88				pmd = pmd_mkdirty(pmd);
  > 89			if (pmd_young(old))
  > 90				pmd = pmd_mkyoung(pmd);
    91	
  > 92			pmd = pmd_wrprotect(pmd);
    93			pmd = pmd_clear_soft_dirty(pmd);
    94	
  > 95			set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
    96		} else if (is_migration_entry(pmd_to_swp_entry(pmd))) {
    97			pmd = pmd_swp_clear_soft_dirty(pmd);
    98			set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
    99		}
   100	}
   101	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] mm/damon: Add option to monitor only writes

[kernel test robot]

Hi Pedro,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hnaz-mm/master]

url:    https://github.com/0day-ci/linux/commits/Pedro-Demarchi-Gomes/mm-damon-Add-option-to-monitor-only-writes/20220203-211407
base:   https://github.com/hnaz/linux-mm master
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220204/202202040626.isVzU9Xl-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0958bb7e2514b62f174b8e5ccfdcff07ce2a2b6b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pedro-Demarchi-Gomes/mm-damon-Add-option-to-monitor-only-writes/20220203-211407
        git checkout 0958bb7e2514b62f174b8e5ccfdcff07ce2a2b6b
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h: In function 'damon_test_merge_two':
>> mm/damon/core-test.h:154:33: error: passing argument 1 of 'damon_merge_two_regions' from incompatible pointer type [-Werror=incompatible-pointer-types]
     154 |         damon_merge_two_regions(t, r, r2);
         |                                 ^
         |                                 |
         |                                 struct damon_target *
   mm/damon/core.c:761:55: note: expected 'struct damon_ctx *' but argument is of type 'struct damon_target *'
     761 | static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
         |                                     ~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h:154:36: error: passing argument 2 of 'damon_merge_two_regions' from incompatible pointer type [-Werror=incompatible-pointer-types]
     154 |         damon_merge_two_regions(t, r, r2);
         |                                    ^
         |                                    |
         |                                    struct damon_region *
   mm/damon/core.c:761:79: note: expected 'struct damon_target *' but argument is of type 'struct damon_region *'
     761 | static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
         |                                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
>> mm/damon/core-test.h:154:9: error: too few arguments to function 'damon_merge_two_regions'
     154 |         damon_merge_two_regions(t, r, r2);
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   mm/damon/core.c:761:13: note: declared here
     761 | static void damon_merge_two_regions(struct damon_ctx *c, struct damon_target *t,
         |             ^~~~~~~~~~~~~~~~~~~~~~~
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h: In function 'damon_test_merge_regions_of':
>> mm/damon/core-test.h:201:32: error: passing argument 1 of 'damon_merge_regions_of' from incompatible pointer type [-Werror=incompatible-pointer-types]
     201 |         damon_merge_regions_of(t, 9, 9999);
         |                                ^
         |                                |
         |                                struct damon_target *
   mm/damon/core.c:787:54: note: expected 'struct damon_ctx *' but argument is of type 'struct damon_target *'
     787 | static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
         |                                    ~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
   mm/damon/core-test.h:201:35: warning: passing argument 2 of 'damon_merge_regions_of' makes pointer from integer without a cast [-Wint-conversion]
     201 |         damon_merge_regions_of(t, 9, 9999);
         |                                   ^
         |                                   |
         |                                   int
   mm/damon/core.c:787:78: note: expected 'struct damon_target *' but argument is of type 'int'
     787 | static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
         |                                                         ~~~~~~~~~~~~~~~~~~~~~^
   In file included from mm/damon/core.c:1123:
>> mm/damon/core-test.h:201:9: error: too few arguments to function 'damon_merge_regions_of'
     201 |         damon_merge_regions_of(t, 9, 9999);
         |         ^~~~~~~~~~~~~~~~~~~~~~
   mm/damon/core.c:787:13: note: declared here
     787 | static void damon_merge_regions_of(struct damon_ctx *c, struct damon_target *t, unsigned int thres,
         |             ^~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/trace_events.h:21,
                    from include/trace/define_trace.h:102,
                    from include/trace/events/damon.h:43,
                    from mm/damon/core.c:19:
   At top level:
   arch/arc/include/asm/perf_event.h:126:27: warning: 'arc_pmu_cache_map' defined but not used [-Wunused-const-variable=]
     126 | static const unsigned int arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
         |                           ^~~~~~~~~~~~~~~~~
   arch/arc/include/asm/perf_event.h:91:27: warning: 'arc_pmu_ev_hw_map' defined but not used [-Wunused-const-variable=]
      91 | static const char * const arc_pmu_ev_hw_map[] = {
         |                           ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/damon_merge_two_regions +154 mm/damon/core-test.h

17ccae8bb5c928 SeongJae Park 2021-09-07  139  
17ccae8bb5c928 SeongJae Park 2021-09-07  140  static void damon_test_merge_two(struct kunit *test)
17ccae8bb5c928 SeongJae Park 2021-09-07  141  {
17ccae8bb5c928 SeongJae Park 2021-09-07  142  	struct damon_target *t;
17ccae8bb5c928 SeongJae Park 2021-09-07  143  	struct damon_region *r, *r2, *r3;
17ccae8bb5c928 SeongJae Park 2021-09-07  144  	int i;
17ccae8bb5c928 SeongJae Park 2021-09-07  145  
17ccae8bb5c928 SeongJae Park 2021-09-07  146  	t = damon_new_target(42);
17ccae8bb5c928 SeongJae Park 2021-09-07  147  	r = damon_new_region(0, 100);
17ccae8bb5c928 SeongJae Park 2021-09-07  148  	r->nr_accesses = 10;
17ccae8bb5c928 SeongJae Park 2021-09-07  149  	damon_add_region(r, t);
17ccae8bb5c928 SeongJae Park 2021-09-07  150  	r2 = damon_new_region(100, 300);
17ccae8bb5c928 SeongJae Park 2021-09-07  151  	r2->nr_accesses = 20;
17ccae8bb5c928 SeongJae Park 2021-09-07  152  	damon_add_region(r2, t);
17ccae8bb5c928 SeongJae Park 2021-09-07  153  
17ccae8bb5c928 SeongJae Park 2021-09-07 @154  	damon_merge_two_regions(t, r, r2);
17ccae8bb5c928 SeongJae Park 2021-09-07  155  	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
17ccae8bb5c928 SeongJae Park 2021-09-07  156  	KUNIT_EXPECT_EQ(test, r->ar.end, 300ul);
17ccae8bb5c928 SeongJae Park 2021-09-07  157  	KUNIT_EXPECT_EQ(test, r->nr_accesses, 16u);
17ccae8bb5c928 SeongJae Park 2021-09-07  158  
17ccae8bb5c928 SeongJae Park 2021-09-07  159  	i = 0;
17ccae8bb5c928 SeongJae Park 2021-09-07  160  	damon_for_each_region(r3, t) {
17ccae8bb5c928 SeongJae Park 2021-09-07  161  		KUNIT_EXPECT_PTR_EQ(test, r, r3);
17ccae8bb5c928 SeongJae Park 2021-09-07  162  		i++;
17ccae8bb5c928 SeongJae Park 2021-09-07  163  	}
17ccae8bb5c928 SeongJae Park 2021-09-07  164  	KUNIT_EXPECT_EQ(test, i, 1);
17ccae8bb5c928 SeongJae Park 2021-09-07  165  
17ccae8bb5c928 SeongJae Park 2021-09-07  166  	damon_free_target(t);
17ccae8bb5c928 SeongJae Park 2021-09-07  167  }
17ccae8bb5c928 SeongJae Park 2021-09-07  168  
17ccae8bb5c928 SeongJae Park 2021-09-07  169  static struct damon_region *__nth_region_of(struct damon_target *t, int idx)
17ccae8bb5c928 SeongJae Park 2021-09-07  170  {
17ccae8bb5c928 SeongJae Park 2021-09-07  171  	struct damon_region *r;
17ccae8bb5c928 SeongJae Park 2021-09-07  172  	unsigned int i = 0;
17ccae8bb5c928 SeongJae Park 2021-09-07  173  
17ccae8bb5c928 SeongJae Park 2021-09-07  174  	damon_for_each_region(r, t) {
17ccae8bb5c928 SeongJae Park 2021-09-07  175  		if (i++ == idx)
17ccae8bb5c928 SeongJae Park 2021-09-07  176  			return r;
17ccae8bb5c928 SeongJae Park 2021-09-07  177  	}
17ccae8bb5c928 SeongJae Park 2021-09-07  178  
17ccae8bb5c928 SeongJae Park 2021-09-07  179  	return NULL;
17ccae8bb5c928 SeongJae Park 2021-09-07  180  }
17ccae8bb5c928 SeongJae Park 2021-09-07  181  
17ccae8bb5c928 SeongJae Park 2021-09-07  182  static void damon_test_merge_regions_of(struct kunit *test)
17ccae8bb5c928 SeongJae Park 2021-09-07  183  {
17ccae8bb5c928 SeongJae Park 2021-09-07  184  	struct damon_target *t;
17ccae8bb5c928 SeongJae Park 2021-09-07  185  	struct damon_region *r;
17ccae8bb5c928 SeongJae Park 2021-09-07  186  	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184};
17ccae8bb5c928 SeongJae Park 2021-09-07  187  	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230};
17ccae8bb5c928 SeongJae Park 2021-09-07  188  	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2};
17ccae8bb5c928 SeongJae Park 2021-09-07  189  
17ccae8bb5c928 SeongJae Park 2021-09-07  190  	unsigned long saddrs[] = {0, 114, 130, 156, 170};
17ccae8bb5c928 SeongJae Park 2021-09-07  191  	unsigned long eaddrs[] = {112, 130, 156, 170, 230};
17ccae8bb5c928 SeongJae Park 2021-09-07  192  	int i;
17ccae8bb5c928 SeongJae Park 2021-09-07  193  
17ccae8bb5c928 SeongJae Park 2021-09-07  194  	t = damon_new_target(42);
17ccae8bb5c928 SeongJae Park 2021-09-07  195  	for (i = 0; i < ARRAY_SIZE(sa); i++) {
17ccae8bb5c928 SeongJae Park 2021-09-07  196  		r = damon_new_region(sa[i], ea[i]);
17ccae8bb5c928 SeongJae Park 2021-09-07  197  		r->nr_accesses = nrs[i];
17ccae8bb5c928 SeongJae Park 2021-09-07  198  		damon_add_region(r, t);
17ccae8bb5c928 SeongJae Park 2021-09-07  199  	}
17ccae8bb5c928 SeongJae Park 2021-09-07  200  
17ccae8bb5c928 SeongJae Park 2021-09-07 @201  	damon_merge_regions_of(t, 9, 9999);
17ccae8bb5c928 SeongJae Park 2021-09-07  202  	/* 0-112, 114-130, 130-156, 156-170 */
17ccae8bb5c928 SeongJae Park 2021-09-07  203  	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
17ccae8bb5c928 SeongJae Park 2021-09-07  204  	for (i = 0; i < 5; i++) {
17ccae8bb5c928 SeongJae Park 2021-09-07  205  		r = __nth_region_of(t, i);
17ccae8bb5c928 SeongJae Park 2021-09-07  206  		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
17ccae8bb5c928 SeongJae Park 2021-09-07  207  		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
17ccae8bb5c928 SeongJae Park 2021-09-07  208  	}
17ccae8bb5c928 SeongJae Park 2021-09-07  209  	damon_free_target(t);
17ccae8bb5c928 SeongJae Park 2021-09-07  210  }
17ccae8bb5c928 SeongJae Park 2021-09-07  211  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] mm/damon: Add option to monitor only writes

[David Hildenbrand]

On 03.02.22 14:12, Pedro Demarchi Gomes wrote:
> When "writes" is written to /sys/kernel/debug/damon/counter_type damon will monitor only writes.
> This patch also adds the actions mergeable and unmergeable to damos schemes. These actions are used by KSM as explained in [1].

[...]

>  
> +static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +{
> +	struct page *page;
> +
> +	if (!pte_write(pte))
> +		return false;
> +	if (!is_cow_mapping(vma->vm_flags))
> +		return false;
> +	if (likely(!test_bit(MMF_HAS_PINNED, &vma->vm_mm->flags)))
> +		return false;
> +	page = vm_normal_page(vma, addr, pte);
> +	if (!page)
> +		return false;
> +	return page_maybe_dma_pinned(page);
> +}
> +
> +static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> +		unsigned long addr, pmd_t *pmdp)
> +{
> +	pmd_t old, pmd = *pmdp;
> +
> +	if (pmd_present(pmd)) {
> +		/* See comment in change_huge_pmd() */
> +		old = pmdp_invalidate(vma, addr, pmdp);
> +		if (pmd_dirty(old))
> +			pmd = pmd_mkdirty(pmd);
> +		if (pmd_young(old))
> +			pmd = pmd_mkyoung(pmd);
> +
> +		pmd = pmd_wrprotect(pmd);
> +		pmd = pmd_clear_soft_dirty(pmd);
> +
> +		set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
> +	} else if (is_migration_entry(pmd_to_swp_entry(pmd))) {
> +		pmd = pmd_swp_clear_soft_dirty(pmd);
> +		set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
> +	}
> +}
> +
> +static inline void clear_soft_dirty(struct vm_area_struct *vma,
> +		unsigned long addr, pte_t *pte)
> +{
> +	/*
> +	 * The soft-dirty tracker uses #PF-s to catch writes
> +	 * to pages, so write-protect the pte as well. See the
> +	 * Documentation/admin-guide/mm/soft-dirty.rst for full description
> +	 * of how soft-dirty works.
> +	 */
> +	pte_t ptent = *pte;
> +
> +	if (pte_present(ptent)) {
> +		pte_t old_pte;
> +
> +		if (pte_is_pinned(vma, addr, ptent))
> +			return;
> +		old_pte = ptep_modify_prot_start(vma, addr, pte);
> +		ptent = pte_wrprotect(old_pte);
> +		ptent = pte_clear_soft_dirty(ptent);
> +		ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
> +	} else if (is_swap_pte(ptent)) {
> +		ptent = pte_swp_clear_soft_dirty(ptent);
> +		set_pte_at(vma->vm_mm, addr, pte, ptent);
> +	}
> +}

Just like clearrefs, this can race against GUP-fast to detect pinned
pages. And just like clearrefs, we're not handling PMDs properly. And
just like anything that write-protects random anon pages right now, this
does not consider O_DIRECT as is.

Fortunately, there are not too many users of clearreefs/softdirty
tracking out there (my search a while ago returned no open source
users). My assumption is that your feature might see more widespread use.

Adding more random write protection until we fixed the COW issues [1]
really makes my stomach hurt on a Monday morning.

Please, let's defer any more features that rely on write-protecting
random anon pages until we have ways in place to not corrupt random user
space.

That is:
1) Teaching the COW logic to not copy pages that are pinned -- I'm
working on that.
2) Converting O_DIRECT to use FOLL_PIN instead of FOLL_GET. John is
working on that.

So I'm not against this change. I'm against this change at this point in
time.

[1]
https://lore.kernel.org/all/3ae33b08-d9ef-f846-56fb-645e3b9b4c66@redhat.com/

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/damon: Add option to monitor only writes

[Pedro Gomes]

On Mon, Feb 7, 2022 at 7:32 AM David Hildenbrand <david@redhat.com> wrote:
>
> Just like clearrefs, this can race against GUP-fast to detect pinned
> pages. And just like clearrefs, we're not handling PMDs properly. And
> just like anything that write-protects random anon pages right now, this
> does not consider O_DIRECT as is.
>
> Fortunately, there are not too many users of clearreefs/softdirty
> tracking out there (my search a while ago returned no open source
> users). My assumption is that your feature might see more widespread use.
>
> Adding more random write protection until we fixed the COW issues [1]
> really makes my stomach hurt on a Monday morning.

I was not aware of these issues.

> Please, let's defer any more features that rely on write-protecting
> random anon pages until we have ways in place to not corrupt random user
> space.
>
> That is:
> 1) Teaching the COW logic to not copy pages that are pinned -- I'm
> working on that.
> 2) Converting O_DIRECT to use FOLL_PIN instead of FOLL_GET. John is
> working on that.
>
> So I'm not against this change. I'm against this change at this point in
> time.

I agree. I will wait until the COW problems are solved to send this patch.


-- 
Atenciosamente,
Pedro Demarchi Gomes.

Re: [PATCH] mm/damon: Add option to monitor only writes

[David Hildenbrand]

On 08.02.22 03:05, Pedro Gomes wrote:
> On Mon, Feb 7, 2022 at 7:32 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> Just like clearrefs, this can race against GUP-fast to detect pinned
>> pages. And just like clearrefs, we're not handling PMDs properly. And
>> just like anything that write-protects random anon pages right now, this
>> does not consider O_DIRECT as is.
>>
>> Fortunately, there are not too many users of clearreefs/softdirty
>> tracking out there (my search a while ago returned no open source
>> users). My assumption is that your feature might see more widespread use.
>>
>> Adding more random write protection until we fixed the COW issues [1]
>> really makes my stomach hurt on a Monday morning.
> 
> I was not aware of these issues.
> 
>> Please, let's defer any more features that rely on write-protecting
>> random anon pages until we have ways in place to not corrupt random user
>> space.
>>
>> That is:
>> 1) Teaching the COW logic to not copy pages that are pinned -- I'm
>> working on that.
>> 2) Converting O_DIRECT to use FOLL_PIN instead of FOLL_GET. John is
>> working on that.
>>
>> So I'm not against this change. I'm against this change at this point in
>> time.
> 
> I agree. I will wait until the COW problems are solved to send this patch.
> 
> 

I'll put you on CC once I have something ready to at least handle pinned
pages (FOLL_PIN) as expected, to not get them accidentally COWed.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/damon: Add option to monitor only writes

[Pedro Gomes]

On Tue, Feb 8, 2022 at 6:01 AM David Hildenbrand <david@redhat.com> wrote:
>
> I'll put you on CC once I have something ready to at least handle pinned
> pages (FOLL_PIN) as expected, to not get them accidentally COWed.
>

OK, thanks!


-- 
Atenciosamente,
Pedro Demarchi Gomes.