linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Michal Hocko <mhocko@kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	"xishi.qiuxishi@alibaba-inc.com" <xishi.qiuxishi@alibaba-inc.com>,
	"Laurent Dufour" <ldufour@linux.vnet.ibm.com>
Subject: Re: [RFC][PATCH v1 04/11] mm: madvise: call soft_offline_page() without MF_COUNT_INCREASED
Date: Tue, 13 Nov 2018 00:18:55 +0000	[thread overview]
Message-ID: <20181113001855.GC5945@hori1.linux.bs1.fc.nec.co.jp> (raw)
In-Reply-To: <21e5b9ca-ad72-b0d5-3397-4b65831b236b@arm.com>

On Fri, Nov 09, 2018 at 04:16:55PM +0530, Anshuman Khandual wrote:
> 
> 
> On 11/09/2018 12:17 PM, Naoya Horiguchi wrote:
> > Currently madvise_inject_error() pins the target page when calling
> > memory error handler, but it's not good because the refcount is just
> > an artifact of error injector and mock nothing about hw error itself.
> > IOW, pinning the error page is part of error handler's task, so
> > let's stop doing it.
> 
> Did not get that. Could you please kindly explain how an incremented
> ref count through get_user_pages_fast() was a mocking the HW error
> previously ? Though I might be missing the some context here.

I meant in "mock nothing about hw error itself" that in the code path
for actual HW error (from MCE handler code) the error page is not pinned
outside (but inside) memory_failure().
So it makes more sense to me to do similarly also in error injection code,
and another good thing is that that makes code more simple (A later patch
eliminates MF_COUNT_INCREASED.)

> 
> > 
> > Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > ---
> >  mm/madvise.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git v4.19-mmotm-2018-10-30-16-08/mm/madvise.c v4.19-mmotm-2018-10-30-16-08_patched/mm/madvise.c
> > index 6cb1ca9..9fa0225 100644
> > --- v4.19-mmotm-2018-10-30-16-08/mm/madvise.c
> > +++ v4.19-mmotm-2018-10-30-16-08_patched/mm/madvise.c
> > @@ -637,6 +637,16 @@ static int madvise_inject_error(int behavior,
> >  		ret = get_user_pages_fast(start, 1, 0, &page);
> >  		if (ret != 1)
> >  			return ret;
> > +		/*
> > +		 * The get_user_pages_fast() is just to get the pfn of the
> > +		 * given address, and the refcount has nothing to do with
> > +		 * what we try to test, so it should be released immediately.
> > +		 * This is racy but it's intended because the real hardware
> > +		 * errors could happen at any moment and memory error handlers
> > +		 * must properly handle the race.
> > +		 */
> > +		put_page(page);
> > +
> >  		pfn = page_to_pfn(page);
> >  
> >  		/*
> > @@ -646,16 +656,11 @@ static int madvise_inject_error(int behavior,
> >  		 */
> >  		order = compound_order(compound_head(page));
> >  
> > -		if (PageHWPoison(page)) {
> > -			put_page(page);
> > -			continue;
> > -		}
> > -
> >  		if (behavior == MADV_SOFT_OFFLINE) {
> >  			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
> >  					pfn, start);
> >  
> > -			ret = soft_offline_page(page, MF_COUNT_INCREASED);
> > +			ret = soft_offline_page(page, 0);
> 
> Probably something defined as a new "ignored" in the memory faults flag
> enumeration instead of passing '0' directly.

MF_* flags are defined as bitmap, not separate values. And according to
other caller like do_memory_failure(), multiple bits in flags can be set together.

    static int do_memory_failure(struct mce *m)
    {
            int flags = MF_ACTION_REQUIRED;
            ....
            if (!(m->mcgstatus & MCG_STATUS_RIPV))
                    flags |= MF_MUST_KILL;
            ret = memory_failure(m->addr >> PAGE_SHIFT, flags);

So I think that simply adding new MF_* value doesn't work, and "flags == 0"
seems to me to show "no flag set" in the clearest way.
Or if you have any code suggestion, that's great.

Thanks,
Naoya Horiguchi

  reply	other threads:[~2018-11-13  0:21 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-09  6:47 [PATCH RFC v1 00/11] hwpoison improvement part 1 Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 01/11] mm: hwpoison: cleanup unused PageHuge() check Naoya Horiguchi
2018-11-09  9:52   ` Anshuman Khandual
2018-11-09  6:47 ` [RFC][PATCH v1 02/11] mm: soft-offline: add missing error check of set_hwpoison_free_buddy_page() Naoya Horiguchi
2018-11-09 10:20   ` Anshuman Khandual
2018-11-13  0:16     ` Naoya Horiguchi
2018-11-14  8:53       ` Anshuman Khandual
2018-11-09  6:47 ` [RFC][PATCH v1 03/11] mm: move definition of num_poisoned_pages_inc/dec to include/linux/mm.h Naoya Horiguchi
2018-11-09 10:28   ` Anshuman Khandual
2018-11-13  0:17     ` Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 04/11] mm: madvise: call soft_offline_page() without MF_COUNT_INCREASED Naoya Horiguchi
2018-11-09 10:46   ` Anshuman Khandual
2018-11-13  0:18     ` Naoya Horiguchi [this message]
2018-11-09  6:47 ` [RFC][PATCH v1 05/11] mm: hwpoison-inject: don't pin for hwpoison_filter() Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 06/11] mm: hwpoison: remove MF_COUNT_INCREASED Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 07/11] mm: remove flag argument from soft offline functions Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 08/11] mm: soft-offline: isolate error pages from buddy freelist Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 09/11] mm: hwpoison: apply buddy page handling code to hard-offline Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 10/11] mm: clear PageHWPoison in memory hotremove Naoya Horiguchi
2018-11-13  1:32   ` Naoya Horiguchi
2018-11-09  6:47 ` [RFC][PATCH v1 11/11] mm: hwpoison: introduce clear_hwpoison_free_buddy_page() Naoya Horiguchi
2018-11-09 11:33   ` Anshuman Khandual
2018-11-13  0:19     ` Naoya Horiguchi
2018-11-14  8:23       ` Anshuman Khandual

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181113001855.GC5945@hori1.linux.bs1.fc.nec.co.jp \
    --to=n-horiguchi@ah.jp.nec.com \
    --cc=akpm@linux-foundation.org \
    --cc=anshuman.khandual@arm.com \
    --cc=ldufour@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=mike.kravetz@oracle.com \
    --cc=xishi.qiuxishi@alibaba-inc.com \
    --subject='Re: [RFC][PATCH v1 04/11] mm: madvise: call soft_offline_page() without MF_COUNT_INCREASED' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).