Re: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: identical data race in `mm/damon/paddr.c` causes access statistics corruption
--

--- Patch [3]: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization ---
commit d42bea4a157d94e45112b2190d4aba2a5eeffaa2
Author: SJ Park <[email protected]>

mm/damon/vaddr: drop last same folio access check optimization

This patch removes the "last same folio access check" optimization in
vaddr.c to fix a data race when multiple kdamonds are running.

Link: https://lore.kernel.org/[email protected]

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index d10b8042adb5b..d487b7a4a1042 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c

[ ... ]

> @@ -494,28 +487,17 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
>   * r	the region to be checked
>   */
>  static void __damon_va_check_access(struct mm_struct *mm,
> -				struct damon_region *r, bool same_target)
> +				struct damon_region *r)
>  {
> -	static unsigned long last_addr;
> -	static unsigned long last_folio_sz = PAGE_SIZE;
> -	static bool last_accessed;

[Severity: High]
This isn't a bug introduced by this patch, but does the same data race still
exist in mm/damon/paddr.c?

Looking at __damon_pa_check_access() in mm/damon/paddr.c, it seems to have the
exact same caching optimization using static variables:

	static phys_addr_t last_addr;
	static unsigned long last_folio_sz = PAGE_SIZE;
	static bool last_accessed;

Since these static variables aren't protected by any locks, could concurrent
kdamond threads using DAMON_OPS_PADDR clobber each other's state in the exact
same way?

For example, if Thread A processes a 2MB huge page and updates last_folio_sz
to 2MB, could Thread B evaluate its next sampling_addr using Thread A's 2MB
last_folio_sz?

This would cause ALIGN_DOWN(last_addr, 2MB) == ALIGN_DOWN(sampling_addr, 2MB)
to incorrectly evaluate to true for up to 512 consecutive 4KB pages, assigning
them all the last_accessed value without actually checking them. Should this
identical optimization be removed from paddr.c as well to completely resolve
the data race?

> +	bool accessed;
>  
>  	if (!mm) {
>  		damon_update_region_access_rate(r, false);
>  		return;
>  	}
>  
> -	/* If the region is in the last checked page, reuse the result */
> -	if (same_target && (ALIGN_DOWN(last_addr, last_folio_sz) ==
> -				ALIGN_DOWN(r->sampling_addr, last_folio_sz))) {
> -		damon_update_region_access_rate(r, last_accessed);
> -		return;
> -	}
> -
> -	last_accessed = damon_va_young(mm, r->sampling_addr, &last_folio_sz);
> -	damon_update_region_access_rate(r, last_accessed);
> -
> -	last_addr = r->sampling_addr;
> +	accessed = damon_va_young(mm, r->sampling_addr);
> +	damon_update_region_access_rate(r, accessed);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3