From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>,
Michal Hocko <mhocko@suse.com>, "hch@lst.de" <hch@lst.de>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"jack@suse.cz" <jack@suse.cz>,
"ross.zwisler@linux.intel.com" <ross.zwisler@linux.intel.com>
Subject: Re: [PATCH v5 05/11] mm, madvise_inject_error: Let memory_failure() optionally take a page reference
Date: Fri, 13 Jul 2018 06:31:25 +0000 [thread overview]
Message-ID: <20180713063125.GA10034@hori1.linux.bs1.fc.nec.co.jp> (raw)
In-Reply-To: <153074044986.27838.16910122305490506387.stgit@dwillia2-desk3.amr.corp.intel.com>
Hello Dan,
On Wed, Jul 04, 2018 at 02:40:49PM -0700, Dan Williams wrote:
> The madvise_inject_error() routine uses get_user_pages() to lookup the
> pfn and other information for injected error, but it does not release
> that pin. The assumption is that failed pages should be taken out of
> circulation.
>
> However, for dax mappings it is not possible to take pages out of
> circulation since they are 1:1 physically mapped as filesystem blocks,
> or device-dax capacity. They also typically represent persistent memory
> which has an error clearing capability.
>
> In preparation for adding a special handler for dax mappings, shift the
> responsibility of taking the page reference to memory_failure(). I.e.
> drop the page reference and do not specify MF_COUNT_INCREASED to
> memory_failure().
>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> mm/madvise.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 4d3c922ea1a1..b731933dddae 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -631,11 +631,13 @@ static int madvise_inject_error(int behavior,
>
>
> for (; start < end; start += PAGE_SIZE << order) {
> + unsigned long pfn;
> int ret;
>
> ret = get_user_pages_fast(start, 1, 0, &page);
> if (ret != 1)
> return ret;
> + pfn = page_to_pfn(page);
>
> /*
> * When soft offlining hugepages, after migrating the page
> @@ -651,17 +653,27 @@ static int madvise_inject_error(int behavior,
>
> if (behavior == MADV_SOFT_OFFLINE) {
> pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
> - page_to_pfn(page), start);
> + pfn, start);
>
> ret = soft_offline_page(page, MF_COUNT_INCREASED);
> if (ret)
> return ret;
> continue;
> }
> +
> pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
> - page_to_pfn(page), start);
> + pfn, start);
> +
> + ret = memory_failure(pfn, 0);
> +
> + /*
> + * Drop the page reference taken by get_user_pages_fast(). In
> + * the absence of MF_COUNT_INCREASED the memory_failure()
> + * routine is responsible for pinning the page to prevent it
> + * from being released back to the page allocator.
> + */
> + put_page(page);
>
> - ret = memory_failure(page_to_pfn(page), MF_COUNT_INCREASED);
MF_COUNT_INCREASED means that the page refcount for memory error handling
is taken by the caller so you don't have to take one inside memory_failure().
So this code don't keep with the definition, then another refcount can be
taken in memory_failure() in normal LRU page's case for example.
As a result the error message "Memory failure: %#lx: %s still referenced by
%d users\n" will be dumped in page_action().
So if you want to put put_page() in madvise_inject_error(), I think that
put_page(page);
ret = memory_failure(pfn, 0);
can be acceptable because the purpose of get_user_pages_fast() here is
just getting pfn, and the refcount itself is not so important.
IOW, memory_failure() is called only with pfn which never changes depending
on the page's status.
In production system memory_failure() is called via machine check code
without taking any pagecount, so I don't think the this injection interface
is properly mocking the real thing. So I'm feeling that this flag will be
wiped out at some point.
Thanks,
Naoya Horiguchi
next prev parent reply other threads:[~2018-07-13 6:31 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-04 21:40 [PATCH v5 00/11] mm: Teach memory_failure() about ZONE_DEVICE pages Dan Williams
2018-07-04 21:40 ` [PATCH v5 01/11] device-dax: Convert to vmf_insert_mixed and vm_fault_t Dan Williams
2018-07-04 21:40 ` [PATCH v5 02/11] device-dax: Enable page_mapping() Dan Williams
2018-07-04 21:40 ` [PATCH v5 03/11] device-dax: Set page->index Dan Williams
2018-07-04 21:40 ` [PATCH v5 04/11] filesystem-dax: " Dan Williams
2018-07-04 21:40 ` [PATCH v5 05/11] mm, madvise_inject_error: Let memory_failure() optionally take a page reference Dan Williams
2018-07-13 6:31 ` Naoya Horiguchi [this message]
2018-07-14 0:34 ` Dan Williams
2018-07-04 21:40 ` [PATCH v5 06/11] mm, memory_failure: Collect mapping size in collect_procs() Dan Williams
2018-07-13 6:49 ` Naoya Horiguchi
2018-07-04 21:41 ` [PATCH v5 07/11] filesystem-dax: Introduce dax_lock_mapping_entry() Dan Williams
2018-07-05 1:07 ` kbuild test robot
2018-07-05 3:31 ` kbuild test robot
2018-07-05 3:33 ` [PATCH v6] " Dan Williams
2018-09-24 15:57 ` [PATCH v5 07/11] " Barret Rhoden
2018-09-27 11:13 ` Jan Kara
2018-07-04 21:41 ` [PATCH v5 08/11] mm, memory_failure: Teach memory_failure() about dev_pagemap pages Dan Williams
2018-07-13 8:52 ` Naoya Horiguchi
2018-07-14 0:28 ` Dan Williams
2018-07-17 6:36 ` Naoya Horiguchi
2018-07-04 21:41 ` [PATCH v5 09/11] x86/mm/pat: Prepare {reserve, free}_memtype() for "decoy" addresses Dan Williams
2018-07-04 21:41 ` [PATCH v5 10/11] x86/memory_failure: Introduce {set, clear}_mce_nospec() Dan Williams
2018-07-04 21:41 ` [PATCH v5 11/11] libnvdimm, pmem: Restore page attributes when clearing errors Dan Williams
2018-07-13 4:44 ` [PATCH v5 00/11] mm: Teach memory_failure() about ZONE_DEVICE pages Dan Williams
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=20180713063125.GA10034@hori1.linux.bs1.fc.nec.co.jp \
--to=n-horiguchi@ah.jp.nec.com \
--cc=dan.j.williams@intel.com \
--cc=hch@lst.de \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nvdimm@lists.01.org \
--cc=mhocko@suse.com \
--cc=ross.zwisler@linux.intel.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).