Re: [PATCH v6 2/4] mm/hwpoison: move definitions of num_poisoned_pages_* to memory-failure.c

From: Naoya Horiguchi <naoya.horiguchi@linux.dev>
To: Oscar Salvador <osalvador@suse.de>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Miaohe Lin <linmiaohe@huawei.com>,
	David Hildenbrand <david@redhat.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Yang Shi <shy828301@gmail.com>,
	Muchun Song <songmuchun@bytedance.com>,
	Jane Chu <jane.chu@oracle.com>,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 2/4] mm/hwpoison: move definitions of num_poisoned_pages_* to memory-failure.c
Date: Fri, 14 Oct 2022 15:38:26 +0900	[thread overview]
Message-ID: <20221014063826.GA1711843@u2004> (raw)
In-Reply-To: <Y0ghWfP4n5XNWDg/@localhost.localdomain>

On Thu, Oct 13, 2022 at 04:31:53PM +0200, Oscar Salvador wrote:
> On Fri, Oct 07, 2022 at 10:07:04AM +0900, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > 
> > These interfaces will be used by drivers/base/memory.c by later patch, so as a
> > preparatory work move them to more common header file visible to the file.
> > 
> > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> > ---
> > ChangeLog v3 -> v6:
> > - remove static in definition of num_poisoned_pages_inc() to fix build error.
> > 
> > ChangeLog v2 -> v3:
> > - added declaration of num_poisoned_pages_inc() in #ifdef CONFIG_MEMORY_FAILURE
> > ---
> >  arch/parisc/kernel/pdt.c |  3 +--
> >  include/linux/mm.h       |  5 +++++
> >  include/linux/swapops.h  | 24 ++----------------------
> >  mm/memory-failure.c      | 10 ++++++++++
> >  4 files changed, 18 insertions(+), 24 deletions(-)
> > 
> > diff --git a/arch/parisc/kernel/pdt.c b/arch/parisc/kernel/pdt.c
> > index e391b175f5ec..fdc880e2575a 100644
> > --- a/arch/parisc/kernel/pdt.c
> > +++ b/arch/parisc/kernel/pdt.c
> > @@ -18,8 +18,7 @@
> >  #include <linux/kthread.h>
> >  #include <linux/initrd.h>
> >  #include <linux/pgtable.h>
> > -#include <linux/swap.h>
> > -#include <linux/swapops.h>
> > +#include <linux/mm.h>
> 
> I am probably missing something.
> num_poisoned_pages_* functions are in swapops.h, but why are you removing swap.h as well?

This file included swap.h and swapops.h together to use num_poisoned_pages_inc()
by commit 0e5a7ff6e36a ("parisc: Report bad pages as HardwareCorrupted"),
so I thought these may be updated together.

> 
> Also, reading the changelog it sounded like both functions would be in mm.h,
> but actually only the _inc part is.

> > ChangeLog v2 -> v3:
> > - added declaration of num_poisoned_pages_inc() in #ifdef CONFIG_MEMORY_FAILURE

Yeah, important part of this log is "in #ifdef CONFIG_MEMORY_FAILURE", but
this might not be clear from my writing.  Sorry about that, I'll care about
making change log clearer from now.  This change log will not included when
merged to mainline, so this hopefully will not confuse anyone.

> 
> The rest looks good to me.

Thank you.

- Naoya Horiguchi