Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 6 May 2022 17:59:20 +0900
From:   Naoya Horiguchi <naoya.horiguchi@linux.dev>
To:     zhenwei pi <pizhenwei@bytedance.com>
Cc:     akpm@linux-foundation.org, naoya.horiguchi@nec.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Wu Fengguang <fengguang.wu@intel.com>
Subject: Re: [PATCH 3/4] mm/memofy-failure.c: optimize hwpoison_filter
Message-ID: <20220506085920.GC1356094@u2004>
References: <20220429142206.294714-1-pizhenwei@bytedance.com>
 <20220429142206.294714-4-pizhenwei@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220429142206.294714-4-pizhenwei@bytedance.com>
Precedence: bulk

On Fri, Apr 29, 2022 at 10:22:05PM +0800, zhenwei pi wrote:
> In the memory failure procedure, hwpoison_filter has higher priority,
> if memory_filter() filters the error event, there is no need to do
> the further work.

Could you clarify what problem you are trying to solve (what does
"optimize" mean in this context or what is the benefit)?

Now hwpoison_filter() can be called both with *and* without taking page refcount.
It's mainly called *with* taking page refcount in order to make sure that the
actual handling process is executed only for pages that meet a given condition.
IOW, it's important to prevent pages which do not meet the condition from going
ahead to further steps (false-positive is not permitted).  So this type of
callsite should not be omittable.

As for the other case, hwpoison_filter() is also called in hwpoison_inject()
*without* taking page refcount.  This actually has a different nuance and
intended to speculatively filter the injection events before setting
PageHWPoison flag to reduce the noise due to setting PG_hwpoison temporary.
The point is that it's not intended here to filter precisely and this callsite
is omittable.

So in my understanding, we need keep hwpoison_filter() after taking page
refcount as we do now.  Maybe optionally and additionally calling
hwpoison_filter() at the beginning of memory_failure() might be possible,
but I'm not sure yet how helpful...

Thanks,
Naoya Horiguchi

> 
> Cc: Wu Fengguang <fengguang.wu@intel.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  mm/memory-failure.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index ece05858568f..a6a27c8b800f 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1800,6 +1800,11 @@ int memory_failure(unsigned long pfn, int flags)
>  		goto unlock_mutex;
>  	}
>  
> +	if (hwpoison_filter(p)) {
> +		res = -EOPNOTSUPP;
> +		goto unlock_mutex;
> +	}
> +
>  try_again:
>  	res = try_memory_failure_hugetlb(pfn, flags, &hugetlb);
>  	if (hugetlb)
> @@ -1937,15 +1942,6 @@ int memory_failure(unsigned long pfn, int flags)
>  	 */
>  	page_flags = p->flags;
>  
> -	if (hwpoison_filter(p)) {
> -		if (TestClearPageHWPoison(p))
> -			num_poisoned_pages_dec();
> -		unlock_page(p);
> -		put_page(p);
> -		res = -EOPNOTSUPP;
> -		goto unlock_mutex;
> -	}
> -
>  	/*
>  	 * __munlock_pagevec may clear a writeback page's LRU flag without
>  	 * page_lock. We need wait writeback completion for this page or it
> -- 
> 2.20.1
>