Re: [PATCH 3/3] mm/memory-failure: inline add_to_kill_anon_file() logic at call sites

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Apr 18, 2025 at 03:52:26PM +0800, Ye Liu wrote:
> From: Ye Liu <liuye@kylinos.cn>
>
> The add_to_kill_anon_file() helper function only checked for -EFAULT
> return values before calling __add_to_kill(). Removes the unnecessary
> wrapper and moves the addr == -EFAULT checks directly into
> collect_procs_anon() and collect_procs_file().
>
> This ensures that error handling is performed close to where the address
> is derived (via page_mapped_in_vma() or page_address_in_vma()), rather
> than being obscured inside a helper function.

Yeah, no.

You're duplicating logic here (the old classic - what if we change logic in one
call site but not the other etc.) and removing self-documentation of what's
going on here.

This is actually a net negative imo. Please drop this patch from the series +
send a v2 for the 2 good patches (with correct cc's ;)

Thanks!

>
> No functional changes are introduced.
>
> Signed-off-by: Ye Liu <liuye@kylinos.cn>
> ---
>  mm/memory-failure.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index b91a33fb6c69..ec0041c95b27 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -486,15 +486,6 @@ static void __add_to_kill(struct task_struct *tsk, const struct page *p,
>  	list_add_tail(&tk->nd, to_kill);
>  }
>
> -static void add_to_kill_anon_file(struct task_struct *tsk, const struct page *p,
> -		struct vm_area_struct *vma, struct list_head *to_kill,
> -		unsigned long addr)
> -{
> -	if (addr == -EFAULT)
> -		return;
> -	__add_to_kill(tsk, p, vma, to_kill, addr);
> -}
> -
>  #ifdef CONFIG_KSM
>  static bool task_in_to_kill_list(struct list_head *to_kill,
>  				 struct task_struct *tsk)
> @@ -634,7 +625,8 @@ static void collect_procs_anon(const struct folio *folio,
>  			if (vma->vm_mm != t->mm)
>  				continue;
>  			addr = page_mapped_in_vma(page, vma);
> -			add_to_kill_anon_file(t, page, vma, to_kill, addr);
> +			if (addr != -EFAULT)
> +				__add_to_kill(t, page, vma, to_kill, addr);
>  		}
>  	}
>  	rcu_read_unlock();
> @@ -674,7 +666,8 @@ static void collect_procs_file(const struct folio *folio,
>  			if (vma->vm_mm != t->mm)
>  				continue;
>  			addr = page_address_in_vma(folio, page, vma);
> -			add_to_kill_anon_file(t, page, vma, to_kill, addr);
> +			if (addr != -EFAULT)
> +				__add_to_kill(t, page, vma, to_kill, addr);
>  		}
>  	}
>  	rcu_read_unlock();
> --
> 2.25.1
>