Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   =?iso-2022-jp?B?SE9SSUdVQ0hJIE5BT1lBKBskQktZOH0hIUQ+TGkbKEIp?= 
        <naoya.horiguchi@nec.com>
To:     Aili Yao <yaoaili@kingsoft.com>
CC:     Oscar Salvador <osalvador@suse.de>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        "david@redhat.com" <david@redhat.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "bp@alien8.de" <bp@alien8.de>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "hpa@zytor.com" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>,
        "inux-edac@vger.kernel.org" <inux-edac@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "yangfeng1@kingsoft.com" <yangfeng1@kingsoft.com>
Subject: Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned
Thread-Topic: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned
Thread-Index: AQHXCn0F5fAOAyGRl0mX4JfXYioGdapnG5GAgAEgN4CAAIHeAA==
Date:   Thu, 25 Feb 2021 11:28:18 +0000
Message-ID: <20210225112818.GA10141@hori.linux.bs1.fc.nec.co.jp>
References: <20210224151619.67c29731@alex-virtual-machine>
 <20210224103105.GA16368@linux> <20210225114329.4e1a41c6@alex-virtual-machine>
In-Reply-To: <20210225114329.4e1a41c6@alex-virtual-machine>
Accept-Language: ja-JP, en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-2022-jp"
Content-ID: <2F1ADB323F838F48B411E9387AA9B558@jpnprd01.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 39f0d871-177e-40f2-4e6e-08d8d9807374
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Feb 2021 11:28:18.7829
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: e67df547-9d0d-4f4d-9161-51c6ed1f7d11
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: bz0UYVDiS482tExH3rZjF/8KUwkVAXzTxW1je/cAVWOAFRhVwi39OPJk9AaHj6YSycpcGYmwX9klaw67ajuzKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYAPR01MB3967
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 25, 2021 at 11:43:29AM +0800, Aili Yao wrote:
> On Wed, 24 Feb 2021 11:31:55 +0100 Oscar Salvador <osalvador@suse.de> wro=
te:
...
> =20
> > >=20
> > > 3.The kill_me_maybe will check the return:
> > >=20
> > > 1244 static void kill_me_maybe(struct callback_head *cb)
> > > 1245 {
> > >=20
> > > 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, =20
> >=20
> > So, IIUC, in case of a LMCE nested call, the second MCE will reach here=
.
> > set_mce_nospec() will either mark the underlying page as not mapped/cac=
hed.
> >
> This set_mce_nospec() is not proper when the recovery job is on the fly. =
In my test
> this function failed.

Hi Aili,

I agree that this set_mce_nospec() is not expected to be called for
"already hwpoisoned" page because in the reported case the error
page is already contained and no need to resort changing cache mode.

...

> > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > > index e9481632fcd1..06f006174b8c 100644
> > > --- a/mm/memory-failure.c
> > > +++ b/mm/memory-failure.c
> > > @@ -1224,7 +1224,7 @@ static int memory_failure_hugetlb(unsigned long=
 pfn, int flags)
> > >  	if (TestSetPageHWPoison(head)) {
> > >  		pr_err("Memory failure: %#lx: already hardware poisoned\n",
> > >  		       pfn);
> > > -		return 0;
> > > +		return -EBUSY; =20
> >=20
> > As David said, madvise_inject_error() will start returning -EBUSY now i=
n case
> > we madvise(MADV_HWPOISON) on an already hwpoisoned page.
> >=20
> > AFAICS, memory_failure() can return 0, -Eerrors, and MF_XXX.
> > Would it make sense to unify that? That way we could declare error code=
s that
> > make somse sense (like MF_ALREADY_HWPOISONED).

It seems to me that memory_failure() does not return MF_XXX.  But yes,
returning some positive value for the reported case could be a solution.

> >=20
>=20
> @David:
>=20
> I checked the code again, and find a few places will care the exact retur=
n value, like:
>=20
> 1: drivers/base/memory.c:483:      ret =3D memory_failure(pfn, 0);
> This is for hard page offline, I see the code in mcelog:
> static void offline_action(struct mempage *mp, u64 addr)
> {
> 	if (offline <=3D OFFLINE_ACCOUNT)
> 		return;
> 	Lprintf("Offlining page %llx\n", addr);
> 	if (memory_offline(addr) < 0) {
> 		Lprintf("Offlining page %llx failed: %s\n", addr, strerror(errno));
> 		mp->offlined =3D PAGE_OFFLINE_FAILED;
> 	} else
> 		mp->offlined =3D PAGE_OFFLINE;
> }
> I think return an negative value will be more proper? As the related kill=
ing function may not be performed, and we can't say
> it's a success operation?
>=20
> 2:mm/hwpoison-inject.c:51:        return memory_failure(pfn, 0);
> mm/madvise.c:910:               ret =3D memory_failure(pfn, MF_COUNT_INCR=
EASED);
>=20
> These two cases are mainly for error injections, I checked the test codes=
, mostly it only care if the value is 0 or < 0;
> I do the related test, normally it work well, but for stress test, someti=
mes in some case, I do meet some fail cases along with the -EBUSY return.
> I will dig more.
>=20
> Other place will only care if the return value is 0. or just ignore it.
>=20
> Hi naoya, what's your opnion for this possible issue, I need your inputs!

We could use some negative value (error code) to report the reported case,
then as you mentioned above, some callers need change to handle the
new case, and the same is true if you use some positive value.
My preference is -EHWPOISON, but other options are fine if justified well.

Thanks,
Naoya Horiguchi=