From:   SeongJae Park <sj@kernel.org>
To:     Baolin Wang <baolin.wang@linux.alibaba.com>
Cc:     sj@kernel.org, akpm@linux-foundation.org, dan.carpenter@oracle.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/damon: Remove redundant page validation
Date:   Thu,  6 Jan 2022 11:40:47 +0000
Message-Id: <20220106114047.6595-1-sj@kernel.org>
In-Reply-To: <cd28052e89166d4ab90e360cce3421748076a0fc.1641465458.git.baolin.wang@linux.alibaba.com>
Precedence: bulk

Hi Baolin,


Thank you always for your great contribution!  As always, trivial comments
below.

On Thu, 6 Jan 2022 18:40:11 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> It will never get a NULL page by pte_page(), thus remove the redundant
> page validation to fix below Smatch static checker warning.

I guess the issue has reported in the open place[1], right?  I think it would
be better to add the link in this commit message, so that future readers can
read and learn from your discussion in the mail thread.

[1] https://lore.kernel.org/linux-mm/20220106091200.GA14564@kili/

> 
> mm/damon/vaddr.c:405 damon_hugetlb_mkold()
> warn: 'page' can't be NULL.

Sorry for continuously obsessing about this trivial and personal taste, but...
Could you please indent quoting blocks like above (hopefully with 4 spaces)?
That makes the message much easier to read for me, and I'd like to keep the
commit messages for DAMON consistent and easy to read, at least for me.

Other than that,

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


Thanks,
SJ

> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/damon/vaddr.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 89b6468d..8a52e00 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -402,9 +402,6 @@ static void damon_hugetlb_mkold(pte_t *pte, struct mm_struct *mm,
>  	pte_t entry = huge_ptep_get(pte);
>  	struct page *page = pte_page(entry);
>  
> -	if (!page)
> -		return;
> -
>  	get_page(page);
>  
>  	if (pte_young(entry)) {
> @@ -564,9 +561,6 @@ static int damon_young_hugetlb_entry(pte_t *pte, unsigned long hmask,
>  		goto out;
>  
>  	page = pte_page(entry);
> -	if (!page)
> -		goto out;
> -
>  	get_page(page);
>  
>  	if (pte_young(entry) || !page_is_idle(page) ||
> -- 
> 1.8.3.1