| Message ID | 20250418075226.695014-4-ye.liu@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: minor cleanups in rmap and memory-failure | expand |
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 >
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();