Re: [PATCH v3 2/3] mm,hwpoison: return -EHWPOISON when page already

From: Borislav Petkov <bp@alien8.de>
To: Naoya Horiguchi <nao.horiguchi@gmail.com>
Cc: linux-mm@kvack.org, Tony Luck <tony.luck@intel.com>,
	Aili Yao <yaoaili@kingsoft.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oscar Salvador <osalvador@suse.de>,
	David Hildenbrand <david@redhat.com>,
	Andy Lutomirski <luto@kernel.org>,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	Jue Wang <juew@google.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] mm,hwpoison: return -EHWPOISON when page already
Date: Thu, 22 Apr 2021 19:02:04 +0200	[thread overview]
Message-ID: <20210422170204.GD7021@zn.tnic> (raw)
In-Reply-To: <20210421005728.1994268-3-nao.horiguchi@gmail.com>

On Wed, Apr 21, 2021 at 09:57:27AM +0900, Naoya Horiguchi wrote:
> From: Aili Yao <yaoaili@kingsoft.com>

> Subject: Re: [PATCH v3 2/3] mm,hwpoison: return -EHWPOISON when page already

		... Return -EHWPOISON to denote that the page has already been poisoned"

> When the page is already poisoned, another memory_failure() call in the
> same page now returns 0, meaning OK. For nested memory mce handling, this
> behavior may lead to one mce looping,

s/mce/MCE/g

> Example:

For example:

> 1. When LCME is enabled, and there are two processes A && B running on
> different core X && Y separately, which will access one same page, then

				  which access the same page...

s/&&/and/g

> the page corrupted when process A access it, a MCE will be rasied to
> core X and the error process is just underway.

... and you lost me here. I don't understand what that is trying to say.
Is that trying to say that when process A encounters the error, the MCE
will be raised on CPU X?

> 2. Then B access the page and trigger another MCE to core Y, it will also
> do error process, it will see TestSetPageHWPoison be true, and 0 is
> returned.

That sentence needs massaging.

> 3. The kill_me_maybe will check the return:
> 
>     1244 static void kill_me_maybe(struct callback_head *cb)
>     1245 {
>     ...
>     1254         if (!memory_failure(p->mce_addr >> PAGE_SHIFT, flags) &&
>     1255             !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) {
>     1256                 set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page);
>     1257                 sync_core();
>     1258                 return;
>     1259         }
>     ...
>     1267 }

No need for the line numbers.

> 4. The error process for B will end, and may nothing happened if
> kill-early is not set, The process B will re-excute instruction and get
> into mce again and then loop happens. And also the set_mce_nospec()
> here is not proper, may refer to commit fd0e786d9d09 ("x86/mm,
> mm/hwpoison: Don't unconditionally unmap kernel 1:1 pages").

That needs massaging too.

> For other cases which care the return value of memory_failure() should
> check why they want to process a memory error which have already been
> processed. This behavior seems reasonable.

This whole commit message needs sanitizing.

Also, looking at the next patch, you can merge this one into the next
because the next one is acting on -EHWPOISON so it all belongs together
in a single patch.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette