Re: [PATCH v1 1/6] mm/hwpoison: mf_mutex for soft offline and unpoison

From: Miaohe Lin <linmiaohe@huawei.com>
To: "HORIGUCHI NAOYA(堀口 直也)" <naoya.horiguchi@nec.com>
Cc: Naoya Horiguchi <nao.horiguchi@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@redhat.com>,
	Oscar Salvador <osalvador@suse.de>,
	Michal Hocko <mhocko@suse.com>, Ding Hui <dinghui@sangfor.com.cn>,
	Tony Luck <tony.luck@intel.com>,
	Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>
Subject: Re: [PATCH v1 1/6] mm/hwpoison: mf_mutex for soft offline and unpoison
Date: Wed, 16 Jun 2021 11:14:31 +0800	[thread overview]
Message-ID: <b0fd4f21-0a51-81e1-9fd2-4ef900674f52@huawei.com> (raw)
In-Reply-To: <20210616004129.GB1924716@hori.linux.bs1.fc.nec.co.jp>

On 2021/6/16 8:41, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Tue, Jun 15, 2021 at 08:42:23PM +0800, Miaohe Lin wrote:
> ...
>>> @@ -1960,7 +1964,7 @@ int unpoison_memory(unsigned long pfn)
>>>  	if (!PageHuge(page) && PageTransHuge(page)) {
>>>  		unpoison_pr_info("Unpoison: Memory failure is now running on %#lx\n",
>>>  				 pfn, &unpoison_rs);
>>> -		return 0;
>>> +		goto unlock_mutex;
>>>  	}
>>>  
>>
>> Maybe it's more appropriate to start mutex_lock(&mf_mutex) here? I think these races start here.
> 
> Hi Miaohe,
> 
> Thank your for the review.
> > Consider that we put mutex_lock() here, and let's think about two concurrent
> calls of unpoison_memory(), then these events could be processed like below:
> 
>     CPU 0                             CPU 1
>     unpoison_memory
>     check PageHWPoison // true
>                                       unpoison_memory
>                                       check PageHWPoison  // true
>     mutex_lock
>     get_hwpoison_page
>     TestClearPageHWPoison
>     put_page
>     put_page // freed
>     mutex_unlock
> 
>     // the unpoisoned page can be used for allocation
> 
>                                       mutex_lock
>                                       get_hwpoison_page // succeeds
>                                       ... // unpoison the !PageHWPoison page !?
> 
> 
> So I thought that we had better do the prechecks in mf_mutex.  Maybe the 2nd
> unpoison_memory() just get and put the page refcount by 1 even in this race,
> so the impact is not so big, but I feel like avoiding "unpoison the
> !PageHWPoison page" situation.
> 
> Does it make sense for you?

Yes, I missed the races inside unpoison_memory() itself.
Many thanks for your detailed explanation!

> 
> Thanks,
> Naoya Horiguchi
> 