From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03A36C7EE2E for ; Fri, 26 May 2023 00:28:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0948F900002; Thu, 25 May 2023 20:28:32 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 01DDD6B0075; Thu, 25 May 2023 20:28:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DB299900002; Thu, 25 May 2023 20:28:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C50F06B0074 for ; Thu, 25 May 2023 20:28:31 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 95866AE844 for ; Fri, 26 May 2023 00:28:31 +0000 (UTC) X-FDA: 80830520022.07.3AA9650 Received: from mail-yb1-f177.google.com (mail-yb1-f177.google.com [209.85.219.177]) by imf19.hostedemail.com (Postfix) with ESMTP id E1E4D1A0018 for ; Fri, 26 May 2023 00:28:28 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=0FlBV9gQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of jiaqiyan@google.com designates 209.85.219.177 as permitted sender) smtp.mailfrom=jiaqiyan@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1685060909; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=R4raorCV/k4j/KfC5TZ+3SxDxTpR/9GVfhD/np1Sjp4=; b=drUGcmqcdgqjozmOF1iZc0bIYkkr0HpMkPZNzV2aDKIq9MKEua5aR1meEIcXfH5rgbE3jl QyBetHt6lQowqbNBBJA8SR0dG7gSs4P0qEpetwK1HfwwDIrek3v+p0wwxvcpxxiChOLhjC TSX6wZs0xRXtbjwYYcVJntKxM01+xCA= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=0FlBV9gQ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of jiaqiyan@google.com designates 209.85.219.177 as permitted sender) smtp.mailfrom=jiaqiyan@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685060909; a=rsa-sha256; cv=none; b=JhalMJpsCf/umEKoO3UIJ4JkJDUE3fJKWTZ+AQOPkq6WMdQqqOiFAySb9nsr88UhpfQadP bQyGUAgnKR7OY5pQLwRWlG8TQGamT45JcjkoFbuXOG5J8OwTgbu8N9u1CZ6s7K7SMYpPN1 mU7a2GboGJlTrcjP0rOr+4/nuxYveng= Received: by mail-yb1-f177.google.com with SMTP id 3f1490d57ef6-ba6d024a196so228936276.2 for ; Thu, 25 May 2023 17:28:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1685060909; x=1687652909; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=R4raorCV/k4j/KfC5TZ+3SxDxTpR/9GVfhD/np1Sjp4=; b=0FlBV9gQskTLgn2L9BWaSCo74b0mrx8Ly8JPn03FKbQnKjr9WxcMsaSomCdwdcCs8f wghYvFyazaqb+F/4D2hqOrPBYiY/8Y6bSDMCjXyEWF+XbZuLWI7RqocqUKtP0Nmc/xEX LHQFkWDA0Fmzlv+h6r8CVDgWF/ApSZ2Hh71CqJsEHLCAcgBQ6GvSO3ZZm+1ZPe2Sb9Mm sC5bEu9FR3UT2H3zpYvXCioti/xNSan/D4eHRQA/iTg7h6b5x2fzfFUrYC6ZLcxOrOOo AkjC9tEUnyV+/RYmKoJfPiJe69abU13eTu0odKsLZHLydUZOltLsZ0nGphpk+bHkIEN1 BoyA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685060909; x=1687652909; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=R4raorCV/k4j/KfC5TZ+3SxDxTpR/9GVfhD/np1Sjp4=; b=CDIYFdw1UvLkgnLPkEk9enYIoyArcFH/WDzOCtMv2jVTZNYgI62xFlxOjK5thwiRZc o+FY1tp3Q5y9cm/PJC7941mRXoickrJu/WGwVMvbmtz55foo1mIemKXKc3JKJhcSDudg RvdhkisktOdTsVbIqvNDP5ypfDGOrgAGFwRhnzwY15k9oweKu9EpzJmnkHWtooVGpUEC S0rR2YrMGkh5AsGQTvJt73h8apB19YM1Mz/+PkE1zlGxmpq8SSw9Qo4Qm0LhbdlZdBC9 h3wxBePe472m9L+26dSwG/5qw8cSxyexbSsXNuSLYW3horjYgaDiEr0YOT5ZSmgMXH6q 4sWQ== X-Gm-Message-State: AC+VfDzZjwxnGrdw9ng4i4VVak+LDNbMEA2NLi2D1isDBLWU4wWsNbZp +EyjSpxz3JszdpI62MXyw0b5QB0x8hSuBr3rUbxhiA== X-Google-Smtp-Source: ACHHUZ4nG7Jkyk/tfSml6A5FggrLgGyyAESoFiw42YuFresYFAy6C1e84vcUbe6doQ4VEDrkEiI4FestEybPeBeJ+6g= X-Received: by 2002:a05:6902:1006:b0:bab:cd91:3639 with SMTP id w6-20020a056902100600b00babcd913639mr6269857ybt.55.1685060908646; Thu, 25 May 2023 17:28:28 -0700 (PDT) MIME-Version: 1.0 References: <20230517160948.811355-1-jiaqiyan@google.com> <20230517160948.811355-2-jiaqiyan@google.com> <20230517235314.GB10757@monkey> <20230519224214.GB3581@monkey> <20230522044557.GA845371@hori.linux.bs1.fc.nec.co.jp> <20230523024305.GA920098@hori.linux.bs1.fc.nec.co.jp> In-Reply-To: <20230523024305.GA920098@hori.linux.bs1.fc.nec.co.jp> From: Jiaqi Yan Date: Thu, 25 May 2023 17:28:17 -0700 Message-ID: Subject: Re: [PATCH v1 1/3] mm/hwpoison: find subpage in hugetlb HWPOISON list To: =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= Cc: Mike Kravetz , "songmuchun@bytedance.com" , "shy828301@gmail.com" , "linmiaohe@huawei.com" , "akpm@linux-foundation.org" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "duenwen@google.com" , "axelrasmussen@google.com" , "jthoughton@google.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: ky6rx1z6wbipcipufcxxye1tnptyh91n X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: E1E4D1A0018 X-HE-Tag: 1685060908-33489 X-HE-Meta: U2FsdGVkX19SD0eA2nVcsvKj+3Rv1CtllMwIyLllD8CZK9i+q36zJcpa/HB2FsgaATqT5u4uHAXqaqnvQZ3ncEOubwsc2YfCjkBNE4qHbf1ghaj/gJwDhrXk4Etfg/eK8WB2h3hV6jc0z9CLqhbIxDAu2xAFRE0KMiuCc2pDheAFl7HgxSvlJlniWvKPnd/Te0YEYQ1ByR97aHEe3LmtwlTEbQRSVXRFHouJK9E3usaNQ501o+1z5rXRwoQ3Yfkeqvg3fMBIFS65odBptdwCzVddwf9e6PcthtUeREdQUmCokbaGePjPzmB8I91N8/MW1XaaqwiGn7DKVYGe3tmaryrXbW8aht/bxRx6fRQCcHpEWBkjwmdXmYcqBzyZ6c2/5u1t+xu+SOdOg1r0i1NFs+Nz5wcLUYr6Ef3xUiouhJpgDk9c7wm/WtrpRdjxfh/FJasstiMbUybmL1TMq2nb4j/2rU6V8bvzO/U3HMgs9eLZB2vqrdtF8eb6zikOy3Q5qg+Gk3TLn6fI+DvDElm7+QXEn/Y72Vpi1rDtyygt24utUtsWNADUJ2CIdSqIQmuwJhsbmpRXDCjhP9GHyhQv2Dr0Eh942BYQw8MQJPKX5TDmvKhPWfrTyJZUaaErDc1MwOH5BcDZRxMuZzoUZDHzr4NCvr8BhZa83jlhQjl9dDqJmtPYj1OTWkHd4/dLkdh+qe06TtpSUfz7SEG1085cGOpLTfNk5i8tBv1iY2dE+1DyaSs78Tv+5xqyfPK9kJJFHEkBfeIhHJH+iuyX+2FTMElKoYwVltNwVS+rtQTg3Tl7afDInulOc8hCY0RxC69Yr6OEcXH9PdbcTOvq6Xe5ix4srz/nmjng5W5I5inCL7cqewQ3OXqOHsCndA1h6tvVxGV1htGsQMlTa5E/2w1mhhn1YB0cqzL7h0XdwaX9eDxyh7CCh1kQp9I+nZWTjuVzjz2AbRF0NTmOPw7cEPy rFp4/yyy /S+YUSdH9SJKqowvFQ9kKwJp+MT5Zz68Fvu5P4cHqKqQIDYK+WjT+bWCE5MSKWqTJprIJ4u5oTLqhTodEHXjxCWamHS/g3f7NDkCct+s1WCyZmt4lbErmwpAg0OyZ5KHoIbKy5EUZeKBCiKy7XGch2CeoFDZsGpCzdtViKSC7LMEMAjhNGeBkOfkqXs+h1T7J2fW1xcaESbv7NZ7U3K73kV1u0rh9i1btTPMbDjcdIESgICJXp5Ky5xrichrsVXoMlEK/XxoIm8+TUOPGawMgxp3wKnQwm4oI97OgAIDXvxPFHP08Lk5jIC1k47DMvsTb2ITlle6r+6YtWyFAATLcY+Epng== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, May 22, 2023 at 7:43=E2=80=AFPM HORIGUCHI NAOYA(=E5=A0=80=E5=8F=A3= =E3=80=80=E7=9B=B4=E4=B9=9F) wrote: > > On Mon, May 22, 2023 at 11:22:49AM -0700, Jiaqi Yan wrote: > > On Sun, May 21, 2023 at 9:50=E2=80=AFPM HORIGUCHI NAOYA(=E5=A0=80=E5=8F= =A3=E3=80=80=E7=9B=B4=E4=B9=9F) > > wrote: > > > > > > On Fri, May 19, 2023 at 03:42:14PM -0700, Mike Kravetz wrote: > > > > On 05/19/23 13:54, Jiaqi Yan wrote: > > > > > On Wed, May 17, 2023 at 4:53=E2=80=AFPM Mike Kravetz wrote: > > > > > > > > > > > > On 05/17/23 16:09, Jiaqi Yan wrote: > > > > > > > Adds the functionality to search a subpage's corresponding ra= w_hwp_page > > > > > > > in hugetlb page's HWPOISON list. This functionality can also = tell if a > > > > > > > subpage is a raw HWPOISON page. > > > > > > > > > > > > > > Exports this functionality to be immediately used in the read= operation > > > > > > > for hugetlbfs. > > > > > > > > > > > > > > Signed-off-by: Jiaqi Yan > > > > > > > --- > > > > > > > include/linux/mm.h | 23 +++++++++++++++++++++++ > > > > > > > mm/memory-failure.c | 26 ++++++++++++++++---------- > > > > > > > 2 files changed, 39 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > > > > > index 27ce77080c79..f191a4119719 100644 > > > > > > > --- a/include/linux/mm.h > > > > > > > +++ b/include/linux/mm.h > > > > > > > > > > > > Any reason why you decided to add the following to linux/mm.h i= nstead of > > > > > > linux/hugetlb.h? Since it is hugetlb specific I would have tho= ught > > > > > > hugetlb.h was more appropriate. > > > > > > > > > > > > > @@ -3683,6 +3683,29 @@ enum mf_action_page_type { > > > > > > > */ > > > > > > > extern const struct attribute_group memory_failure_attr_grou= p; > > > > > > > > > > > > > > +#ifdef CONFIG_HUGETLB_PAGE > > > > > > > +/* > > > > > > > + * Struct raw_hwp_page represents information about "raw err= or page", > > > > > > > + * constructing singly linked list from ->_hugetlb_hwpoison = field of folio. > > > > > > > + */ > > > > > > > +struct raw_hwp_page { > > > > > > > + struct llist_node node; > > > > > > > + struct page *page; > > > > > > > +}; > > > > > > > + > > > > > > > +static inline struct llist_head *raw_hwp_list_head(struct fo= lio *folio) > > > > > > > +{ > > > > > > > + return (struct llist_head *)&folio->_hugetlb_hwpoison; > > > > > > > +} > > > > > > > + > > > > > > > +/* > > > > > > > + * Given @subpage, a raw page in a hugepage, find its locati= on in @folio's > > > > > > > + * _hugetlb_hwpoison list. Return NULL if @subpage is not in= the list. > > > > > > > + */ > > > > > > > +struct raw_hwp_page *find_raw_hwp_page(struct folio *folio, > > > > > > > + struct page *subpage); > > > > > > > +#endif > > > > > > > + > > > > > > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_H= UGETLBFS) > > > > > > > extern void clear_huge_page(struct page *page, > > > > > > > unsigned long addr_hint, > > > > > > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > > > > > > > index 5b663eca1f29..c49e6c2d1f07 100644 > > > > > > > --- a/mm/memory-failure.c > > > > > > > +++ b/mm/memory-failure.c > > > > > > > @@ -1818,18 +1818,24 @@ EXPORT_SYMBOL_GPL(mf_dax_kill_procs); > > > > > > > #endif /* CONFIG_FS_DAX */ > > > > > > > > > > > > > > #ifdef CONFIG_HUGETLB_PAGE > > > > > > > -/* > > > > > > > - * Struct raw_hwp_page represents information about "raw err= or page", > > > > > > > - * constructing singly linked list from ->_hugetlb_hwpoison = field of folio. > > > > > > > - */ > > > > > > > -struct raw_hwp_page { > > > > > > > - struct llist_node node; > > > > > > > - struct page *page; > > > > > > > -}; > > > > > > > > > > > > > > -static inline struct llist_head *raw_hwp_list_head(struct fo= lio *folio) > > > > > > > +struct raw_hwp_page *find_raw_hwp_page(struct folio *folio, > > > > > > > + struct page *subpage) > > > > > > > { > > > > > > > - return (struct llist_head *)&folio->_hugetlb_hwpoison; > > > > > > > + struct llist_node *t, *tnode; > > > > > > > + struct llist_head *raw_hwp_head =3D raw_hwp_list_head(f= olio); > > > > > > > + struct raw_hwp_page *hwp_page =3D NULL; > > > > > > > + struct raw_hwp_page *p; > > > > > > > + > > > > > > > + llist_for_each_safe(tnode, t, raw_hwp_head->first) { > > > > > > > > > > > > IIUC, in rare error cases a hugetlb page can be poisoned WITHOU= T a > > > > > > raw_hwp_list. This is indicated by the hugetlb page specific f= lag > > > > > > RawHwpUnreliable or folio_test_hugetlb_raw_hwp_unreliable(). > > > > > > > > > > > > Looks like this routine does not consider that case. Seems lik= e it should > > > > > > always return the passed subpage if folio_test_hugetlb_raw_hwp_= unreliable() > > > > > > is true? > > > > > > > > > > Thanks for catching this. I wonder should this routine consider > > > > > RawHwpUnreliable or should the caller do. > > > > > > > > > > find_raw_hwp_page now returns raw_hwp_page* in the llist entry to > > > > > caller (valid one at the moment), but once RawHwpUnreliable is se= t, > > > > > all the raw_hwp_page in the llist will be kfree(), and the return= ed > > > > > value becomes dangling pointer to caller (if the caller holds tha= t > > > > > caller long enough). Maybe returning a bool would be safer to the > > > > > caller? If the routine returns bool, then checking RawHwpUnreliab= le > > > > > can definitely be within the routine. > > > > > > > > I think the check for RawHwpUnreliable should be within this routin= e. > > > > Looking closer at the code, I do not see any way to synchronize thi= s. > > > > It looks like manipulation in the memory-failure code would be > > > > synchronized via the mf_mutex. However, I do not see how traversal= and > > > > freeing of the raw_hwp_list called from __update_and_free_hugetlb_= folio > > > > is synchronized against memory-failure code modifying the list. > > > > > > > > Naoya, can you provide some thoughts? > > > > > > Thanks for elaborating the issue. I think that making find_raw_hwp_p= age() and > > > folio_clear_hugetlb_hwpoison() do their works within mf_mutex can be = one solution. > > > try_memory_failure_hugetlb(), one of the callers of folio_clear_huget= lb_hwpoison(), > > > already calls it within mf_mutex, so some wrapper might be needed to = implement > > > calling path from __update_and_free_hugetlb_folio() to take mf_mutex. > > > > > > It might be a concern that mf_mutex is a big lock to cover overall hw= poison > > > subsystem, but I think that the impact is not so big if the changed c= ode paths > > > take mf_mutex only after checking hwpoisoned hugepage. Maybe using f= olio_lock > > > to synchronize accesses to the raw_hwp_list could be possible, but cu= rrently > > > __get_huge_page_for_hwpoison() calls folio_set_hugetlb_hwpoison() wit= hout > > > folio_lock, so this approach needs update on locking rule and it soun= ds more > > > error-prone to me. > > > > Thanks Naoya, since memory_failure is the main user of raw_hwp_list, I > > agree mf_mutex could help to sync its two del_all operations (one from > > try_memory_failure_hugetlb and one from > > __update_and_free_hugetlb_folio). > > > > I still want to ask a perhaps stupid question, somewhat related to how > > to implement find_raw_hwp_page() correctly. It seems > > llist_for_each_safe should only be used to traverse after list entries > > already *deleted* via llist_del_all. But the llist_for_each_safe calls > > in memory_failure today is used *directly* on the raw_hwp_list. This > > is quite different from other users of llist_for_each_safe (for > > example, kernel/bpf/memalloc.c). > > Oh, I don't noticed that when writing the original code. (I just chose > llist_for_each_safe because I just wanted struct llist_node as a singly > linked list.) And maybe because you can avoid doing INIT_LIST_HEAD (which seems doable in folio_set_hugetlb_hwpoison if hugepage is hwpoison-ed for the first time)? > > > Why is it correct? I guess mostly > > because they are sync-ed under mf_mutex (except the missing coverage > > on __update_and_free_hugetlb_folio)? > > Yes, and there seems no good reason to use the macro llist_for_each_safe > here. I think it's OK to switch to simpler one like list_for_each which > should be supposed to be called directly. To do this, struct raw_hwp_pag= e > need to have @node (typed struct list_head instead of struct llist_node). I will start to work on a separate patch to switch to list_head, and make sure operations from __update_and_free_hugetlb_folio and memory_failure are serialized (hopefully without intro new locks and just use mf_mutex). > > Thanks, > Naoya Horiguchi