Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: David Hildenbrand <david@redhat.com>
To: Oscar Salvador <osalvador@suse.de>, n-horiguchi@ah.jp.nec.com
Cc: mhocko@kernel.org, mike.kravetz@oracle.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 15/16] mm/hwpoison-inject: Rip off duplicated checks
Date: Mon, 21 Oct 2019 11:40:39 +0200
Message-ID: <1aa1f09a-1210-d0bc-86ac-9674828bff49@redhat.com> (raw)
In-Reply-To: <20191017142123.24245-16-osalvador@suse.de>

On 17.10.19 16:21, Oscar Salvador wrote:
> memory_failure() already performs the same checks, so leave it
> to the main routine.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>   mm/hwpoison-inject.c | 33 +++------------------------------
>   1 file changed, 3 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
> index 0c8cdb80fd7d..fdcca3df4283 100644
> --- a/mm/hwpoison-inject.c
> +++ b/mm/hwpoison-inject.c
> @@ -14,49 +14,22 @@ static struct dentry *hwpoison_dir;
>   static int hwpoison_inject(void *data, u64 val)
>   {
>   	unsigned long pfn = val;
> -	struct page *p;
> -	struct page *hpage;
> -	int err;
>   
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	if (!pfn_valid(pfn))
> -		return -ENXIO;
> -
> -	p = pfn_to_page(pfn);
> -	hpage = compound_head(p);
> -
> -	if (!hwpoison_filter_enable)
> -		goto inject;
> -
> -	shake_page(hpage, 0);
> -	/*
> -	 * This implies unable to support non-LRU pages.
> -	 */
> -	if (!PageLRU(hpage) && !PageHuge(p))
> -		return 0;
> -
> -	/*
> -	 * do a racy check to make sure PG_hwpoison will only be set for
> -	 * the targeted owner (or on a free page).
> -	 * memory_failure() will redo the check reliably inside page lock.
> -	 */
> -	err = hwpoison_filter(hpage);
> -	if (err)
> -		return 0;
> -
> -inject:
>   	pr_info("Injecting memory failure at pfn %#lx\n", pfn);
>   	return memory_failure(pfn, 0);
>   }
>   

I explored somewhere already why this code was added:


commit 31d3d3484f9bd263925ecaa341500ac2df3a5d9b
Author: Wu Fengguang <fengguang.wu@intel.com>
Date:   Wed Dec 16 12:19:59 2009 +0100

    HWPOISON: limit hwpoison injector to known page types
    
    __memory_failure()'s workflow is
    
            set PG_hwpoison
            //...
            unset PG_hwpoison if didn't pass hwpoison filter
    
    That could kill unrelated process if it happens to page fault on the
    page with the (temporary) PG_hwpoison. The race should be big enough to
    appear in stress tests.
    
    Fix it by grabbing the page and checking filter at inject time.  This
    also avoids the very noisy "Injecting memory failure..." messages.
    
    - we don't touch madvise() based injection, because the filters are
      generally not necessary for it.
    - if we want to apply the filters to h/w aided injection, we'd better to
      rearrange the logic in __memory_failure() instead of this patch.
    
    AK: fix documentation, use drain all, cleanups


You should justify why it is okay to do rip that code out now.
It's not just duplicate checks.

Was the documented race fixed?
Will we fix the race within memory_failure() later?
Don't we care?

Also, you should add that this fixes the access of uninitialized memmaps
now and makes the interface work correctly with devmem.

-- 

Thanks,

David / dhildenb



  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-17 14:21 [RFC PATCH v2 00/16] Hwpoison rework {hard,soft}-offline Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 01/16] mm,hwpoison: cleanup unused PageHuge() check Oscar Salvador
2019-10-18 11:48   ` Michal Hocko
2019-10-21  7:00     ` Naoya Horiguchi
2019-10-21 12:16       ` Michal Hocko
2019-11-12 12:22       ` Aneesh Kumar K.V
2019-11-13  6:02         ` Naoya Horiguchi
2019-10-17 14:21 ` [RFC PATCH v2 02/16] mm,madvise: call soft_offline_page() without MF_COUNT_INCREASED Oscar Salvador
2019-10-18 11:52   ` Michal Hocko
2019-10-21  7:02     ` Naoya Horiguchi
2019-10-21 12:20       ` Michal Hocko
2019-10-17 14:21 ` [RFC PATCH v2 03/16] mm,madvise: Refactor madvise_inject_error Oscar Salvador
2019-10-21  7:03   ` Naoya Horiguchi
2019-10-17 14:21 ` [RFC PATCH v2 04/16] mm,hwpoison-inject: don't pin for hwpoison_filter Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 05/16] mm,hwpoison: Un-export get_hwpoison_page and make it static Oscar Salvador
2019-10-21  7:03   ` Naoya Horiguchi
2019-10-17 14:21 ` [RFC PATCH v2 06/16] mm,hwpoison: Kill put_hwpoison_page Oscar Salvador
2019-10-21  7:04   ` Naoya Horiguchi
2019-10-17 14:21 ` [RFC PATCH v2 07/16] mm,hwpoison: remove MF_COUNT_INCREASED Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 08/16] mm,hwpoison: remove flag argument from soft offline functions Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 09/16] mm,hwpoison: Unify THP handling for hard and soft offline Oscar Salvador
2019-10-21  7:04   ` Naoya Horiguchi
2019-10-21  9:51     ` [PATCH 17/16] mm,hwpoison: introduce MF_MSG_UNSPLIT_THP Naoya Horiguchi
2019-10-22  8:00       ` Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 10/16] mm,hwpoison: Rework soft offline for free pages Oscar Salvador
2019-10-18 12:06   ` Michal Hocko
2019-10-21 12:58     ` Oscar Salvador
2019-10-21 15:41       ` Michal Hocko
2019-10-22  7:46         ` Oscar Salvador
2019-10-22  8:26           ` Michal Hocko
2019-10-22  8:35             ` Oscar Salvador
2019-10-22  9:22               ` Michal Hocko
2019-10-22  9:58                 ` Oscar Salvador
2019-10-22 10:24                   ` Michal Hocko
2019-10-22 10:33                     ` Oscar Salvador
2019-10-23  2:15                       ` Naoya Horiguchi
2019-10-23  2:01                   ` Naoya Horiguchi
2019-10-21  7:45   ` Naoya Horiguchi
2019-10-22  8:00     ` Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 11/16] mm,hwpoison: Rework soft offline for in-use pages Oscar Salvador
2019-10-18 12:39   ` Michal Hocko
2019-10-21 13:48     ` Oscar Salvador
2019-10-21 14:06       ` Michal Hocko
2019-10-22  7:56         ` Oscar Salvador
2019-10-22  8:30           ` Michal Hocko
2019-10-22  9:40             ` Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 12/16] mm,hwpoison: Refactor soft_offline_huge_page and __soft_offline_page Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 13/16] mm,hwpoison: Take pages off the buddy when hard-offlining Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 14/16] mm,hwpoison: Return 0 if the page is already poisoned in soft-offline Oscar Salvador
2019-10-21  9:20   ` Naoya Horiguchi
2019-10-17 14:21 ` [RFC PATCH v2 15/16] mm/hwpoison-inject: Rip off duplicated checks Oscar Salvador
2019-10-21  9:40   ` David Hildenbrand [this message]
2019-10-22  7:57     ` Oscar Salvador
2019-10-17 14:21 ` [RFC PATCH v2 16/16] mm, soft-offline: convert parameter to pfn Oscar Salvador
2019-10-18  8:15   ` David Hildenbrand
2020-06-11 16:43 ` [RFC PATCH v2 00/16] Hwpoison rework {hard,soft}-offline Dmitry Yakunin
2020-06-15  6:19   ` HORIGUCHI NAOYA(堀口 直也)

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=1aa1f09a-1210-d0bc-86ac-9674828bff49@redhat.com \
    --to=david@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=mike.kravetz@oracle.com \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=osalvador@suse.de \
    /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

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git