Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   SeongJae Park <sj@kernel.org>
Cc:     sj@kernel.org, akpm@linux-foundation.org, damon@lists.linux.dev,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Kaixu Xia <kaixuxia@tencent.com>
Subject: Re: [PATCH v2 2/2] mm/damon/vaddr: remove comparison between mm and last_mm when checking region accesses
Date:   Sat, 27 Aug 2022 21:25:00 +0000
Message-Id: <20220827212500.50479-1-sj@kernel.org>
In-Reply-To: <1661590971-20893-3-git-send-email-kaixuxia@tencent.com>
References: 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
To:     unlisted-recipients:; (no To-header on input)
Precedence: bulk

Hi Kaixu,

> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The damon regions that belong to the same damon target have the same
> 'struct mm_struct *mm', so it's unnecessary to compare the mm and last_mm
> objects among the damon regions in one damon target when checking accesses.
> But the check is necessary when the target changed in
> '__damon_va_check_accesses()', so we can simplify the whole operation by
> using the bool 'same_target' to indicate whether the target changed.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

> ---
>  mm/damon/vaddr.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index e481f81c3efb..5f66c826ee3a 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -533,15 +533,14 @@ 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)
> +				struct damon_region *r, bool same_target)
>  {
> -	static struct mm_struct *last_mm;
>  	static unsigned long last_addr;
>  	static unsigned long last_page_sz = PAGE_SIZE;
>  	static bool last_accessed;
>  
>  	/* If the region is in the last checked page, reuse the result */
> -	if (mm == last_mm && (ALIGN_DOWN(last_addr, last_page_sz) ==
> +	if (same_target && (ALIGN_DOWN(last_addr, last_page_sz) ==
>  				ALIGN_DOWN(r->sampling_addr, last_page_sz))) {
>  		if (last_accessed)
>  			r->nr_accesses++;
> @@ -552,7 +551,6 @@ static void __damon_va_check_access(struct mm_struct *mm,
>  	if (last_accessed)
>  		r->nr_accesses++;
>  
> -	last_mm = mm;
>  	last_addr = r->sampling_addr;
>  }
>  
> @@ -562,14 +560,17 @@ static unsigned int damon_va_check_accesses(struct damon_ctx *ctx)
>  	struct mm_struct *mm;
>  	struct damon_region *r;
>  	unsigned int max_nr_accesses = 0;
> +	bool same_target;
>  
>  	damon_for_each_target(t, ctx) {
>  		mm = damon_get_mm(t);
>  		if (!mm)
>  			continue;
> +		same_target = false;
>  		damon_for_each_region(r, t) {
> -			__damon_va_check_access(mm, r);
> +			__damon_va_check_access(mm, r, same_target);
>  			max_nr_accesses = max(r->nr_accesses, max_nr_accesses);
> +			same_target = true;
>  		}
>  		mmput(mm);
>  	}
> -- 
> 2.27.0