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 AB59BC7EE23 for ; Mon, 22 May 2023 18:23:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1A7C3900002; Mon, 22 May 2023 14:23:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 12DE46B0075; Mon, 22 May 2023 14:23:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F1028900002; Mon, 22 May 2023 14:23:04 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id DC0326B0074 for ; Mon, 22 May 2023 14:23:04 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id A790540312 for ; Mon, 22 May 2023 18:23:04 +0000 (UTC) X-FDA: 80818712688.01.F9905A9 Received: from mail-yb1-f182.google.com (mail-yb1-f182.google.com [209.85.219.182]) by imf19.hostedemail.com (Postfix) with ESMTP id C6B601A0014 for ; Mon, 22 May 2023 18:23:01 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b="v4Bf3Zt/"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of jiaqiyan@google.com designates 209.85.219.182 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=1684779781; 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=AXtB3oHldSeARfmO9Kb5DTV3MtiHTcwqap7Kh6oNPas=; b=lperhkVscGXO24+mR05agiKLC2U4NPA9OPESRM4l5JgxpMQOWuPcizCjXFwQFF68W1kB5H l/iUKQ3E+YvKyrPYkEZO/qlUksewBWP1bJeHAkSbIHk23hynt+zJmit/ccCQhXkbaQxUm9 Rb/rliNLoBsCMGe6MtewxkP623+rjgo= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b="v4Bf3Zt/"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of jiaqiyan@google.com designates 209.85.219.182 as permitted sender) smtp.mailfrom=jiaqiyan@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1684779781; a=rsa-sha256; cv=none; b=Ya1YZN5W3kSqgmjN5fZBKx+utI+XPYcDpCRbWRSwsbrCDWMYaTrZLN2kscin0i3j8Q9PJz CvtDeLud9wuocyS07kxvqCJcpe/2P+si9F/kX+smBvRTa83TKVD83FzA62hdxBiJ1600eA ACJmo/YWdQq52ISushjxAA3tKDyIc4M= Received: by mail-yb1-f182.google.com with SMTP id 3f1490d57ef6-ba94605bcd5so5535416276.2 for ; Mon, 22 May 2023 11:23:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1684779781; x=1687371781; 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=AXtB3oHldSeARfmO9Kb5DTV3MtiHTcwqap7Kh6oNPas=; b=v4Bf3Zt/0dA6dX7QYk5te/8rKJqO2bXZjU8emhfM4C85TyIk2GZYN07dJ4TR71KefF l8bpYeuW7eApLH8K56CAyNXV39HOcUSv3L6IzMToFDZPDYEt4+AHl0dGN8bdhxr44N2Q fcDKB3WNhRjMgtX5sWG3Ix+YmApsizsNdTi6truSAwa1otHCVFidCSp2a3dOkGRWZlfk RNO9LKG2KadH7zDsXz28Scc9T6wczUzCsCaVjaR299M5yy1+88QK6KQ6PA+T2eQL8stY qgfodg7odRxDDEzNIdw3/vQr+8Y9nuCtkxSnfohA8Wgt4nMcRgY3z78aedhTLL6js3XI CqBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684779781; x=1687371781; 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=AXtB3oHldSeARfmO9Kb5DTV3MtiHTcwqap7Kh6oNPas=; b=NSQU57yROKw+QVALtF7LTVpl+NXUVcNYhbvczK6cox5AwzYcmRUaBivDImv9co7Ly/ ed9VYnIGsP3FjgUaRiueG0vs57UHv3XhRFb9zvOlqq0lVk/bp8i6+1IchwT8rfTlXBv4 gdTYD4T6GuEo+c5mSnJBvv/4GHQaGWdSvaJntg2SZ7lpa1gALoMFFPyHL1m0unos0rLq +C27MC0U8ONRkLDhrpY5ssdLZ+XQxurVmKNIjjJf+1lPH1O06+Nxu0RfcCXH6+ycE2JM kjnBYhnK0HDi90N98RZaQw1PLc9fhhaqz73Za/bVcQlSS068s0CJ7DxI86pyDu88xBYK xX7w== X-Gm-Message-State: AC+VfDzOntRavE1Qy8FtAPAYn5/opqn3aaPsCeAmsuXanyO/DmzmdYmS aztVU9Igr7YirbvXOLqRck44WxOF5TFRfW+APi+s1g== X-Google-Smtp-Source: ACHHUZ7brytdvrneMs3kt9SEg7c2NiHeHYJ6QQ+HaR3rOxCwDtd/21JuOoaCi5SzA30JUBilcXsYn8ZH56CsnxWGmk0= X-Received: by 2002:a81:6ad5:0:b0:55a:380b:552d with SMTP id f204-20020a816ad5000000b0055a380b552dmr11643260ywc.29.1684779780603; Mon, 22 May 2023 11:23:00 -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> In-Reply-To: <20230522044557.GA845371@hori.linux.bs1.fc.nec.co.jp> From: Jiaqi Yan Date: Mon, 22 May 2023 11:22:49 -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-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: C6B601A0014 X-Stat-Signature: 3shdbwad9h1qcnsg5umgbkrfddnxare8 X-HE-Tag: 1684779781-467742 X-HE-Meta: U2FsdGVkX1/dhen9TeAqxPEmRqxMZKS2ci0/tRyYCaVZI6znCuc32IEvI8aBgdJ/cLO/smOLr7p5X5dIhszajwshHeRRI+6cUTYDEL3AGkTm9rJNMaxtSR7nuR1e8ZCZaIDkbdQFtklATMEJXQlToKHEeDhesRpW9ce93rjTjyGlnJCWpX1OWOsnSoxvrwoG6lR5IMXmxTa7eqmDGXS1ATsxXA6n/zSv+m3VIR+Z1i2LYZuUp8LL+CKqY2rCWVsuHzjYUeGhYEVu3RGVjyn+8VxJarADBNU4VsEg4psJiE7whVMtc1S6k75uSBoaC9rFcp5XEgaIbkQgdBMgbsvb6wMdhhY7yVwWHa349V3Lm86esQ1S4lM7mb6QevVR0cpOM0ICy4ljEVd8cmGYWpmNSFlqmLBzO3vrUIsn7WPYXW7fEdNuS0VYLJLPre64An16HIqobN7cxWG8bMaUVdpN8S0Zwl6Nx6ih6RORLsmyBF7mYJRX/NpO8LafBuKtt+dAdMjPvmkFK9kzmw+49HsWUTNatnFcjbA0rPxUqFqx+SJe3lP+QPrDrEpCNpJELPYHl+VkYD7OiCTP5i9M7bnPofjm1Sb6XixAy5T6EdBJHDYQP6A6taJudADQHDQGrXuoJjvpSeytKzn9WIHKtL1tehYXqtzoTxHWB4I3TCT4IE7ZWrA+o+yR0TZ7IBpBkmetb92SLuyM6bd+1a9scrmCxBreCweJ1oMoE5M4GgiAr+GkD654MTabhacZWWKLOmwf+oK6U2RwNcj9Ebw+4xW0TSn+1lUR0SNPpWGhIODpWRCJb3WRf0Gl8NBgZ+9LuIhQGrxKmduxX+myDQ0GdrSJuvwb7v7pSEZ5lYgF3/Ann0lusYGThOgY4N0iQXOQcXp9roRd/xh+LSN7k/mRGZ2XCje6Zhd/n5eZlLyQVUd8iFdYXDQmKLXdCPo/eQUkQuf2nmnQG19tqvWKZGL2pya 3o3OcZCH Dyj76qjzx/HF/S9jsBbGrE8AbjPCLmk2ImGjBr/Hrdm2i1w3+JK7VyPtfLO/KJuk2WncQI6M0q5Zp5s5yDT2maWe83yBfYcHX9Y6JgF0HyQpGr53cwUwuEInK7X04o38/7U20b58LAoYRAfZhyA44YtES9i56HvUgk1EbsU1ZcIcuAafs96C5gvGP+6uZYBTZPrZlWCPaGV5BXq9OLxt4kNFk0QCH7eJIZFRyVhWfoQ38oel8h8m/ByfsMbpWRRZFvJmSs+wLF1mqgSRT8T1v2gWsKHrPHE2cXsdSCr1IJgecXo8h8eP2IszSprIDlX9Bp+hQR13jH/y1gioFOgot4ne/3w== 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 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 raw_hw= p_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 ope= ration > > > > > 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 inste= ad of > > > > linux/hugetlb.h? Since it is hugetlb specific I would have thought > > > > hugetlb.h was more appropriate. > > > > > > > > > @@ -3683,6 +3683,29 @@ enum mf_action_page_type { > > > > > */ > > > > > extern const struct attribute_group memory_failure_attr_group; > > > > > > > > > > +#ifdef CONFIG_HUGETLB_PAGE > > > > > +/* > > > > > + * Struct raw_hwp_page represents information about "raw error p= age", > > > > > + * constructing singly linked list from ->_hugetlb_hwpoison fiel= d of folio. > > > > > + */ > > > > > +struct raw_hwp_page { > > > > > + struct llist_node node; > > > > > + struct page *page; > > > > > +}; > > > > > + > > > > > +static inline struct llist_head *raw_hwp_list_head(struct folio = *folio) > > > > > +{ > > > > > + return (struct llist_head *)&folio->_hugetlb_hwpoison; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Given @subpage, a raw page in a hugepage, find its location i= n @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_HUGET= LBFS) > > > > > 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 error p= age", > > > > > - * constructing singly linked list from ->_hugetlb_hwpoison fiel= d of folio. > > > > > - */ > > > > > -struct raw_hwp_page { > > > > > - struct llist_node node; > > > > > - struct page *page; > > > > > -}; > > > > > > > > > > -static inline struct llist_head *raw_hwp_list_head(struct folio = *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(folio= ); > > > > > + 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 WITHOUT a > > > > raw_hwp_list. This is indicated by the hugetlb page specific flag > > > > RawHwpUnreliable or folio_test_hugetlb_raw_hwp_unreliable(). > > > > > > > > Looks like this routine does not consider that case. Seems like it= should > > > > always return the passed subpage if folio_test_hugetlb_raw_hwp_unre= liable() > > > > 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 set, > > > all the raw_hwp_page in the llist will be kfree(), and the returned > > > value becomes dangling pointer to caller (if the caller holds that > > > caller long enough). Maybe returning a bool would be safer to the > > > caller? If the routine returns bool, then checking RawHwpUnreliable > > > can definitely be within the routine. > > > > I think the check for RawHwpUnreliable should be within this routine. > > Looking closer at the code, I do not see any way to synchronize this. > > 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_foli= o > > 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_page(= ) 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_hugetlb_h= wpoison(), > already calls it within mf_mutex, so some wrapper might be needed to impl= ement > 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 hwpois= on > subsystem, but I think that the impact is not so big if the changed code = paths > take mf_mutex only after checking hwpoisoned hugepage. Maybe using folio= _lock > to synchronize accesses to the raw_hwp_list could be possible, but curren= tly > __get_huge_page_for_hwpoison() calls folio_set_hugetlb_hwpoison() without > folio_lock, so this approach needs update on locking rule and it sounds m= ore > 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). 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)? > > Thanks, > Naoya Horiguchi > > > > > > > > > Another option is, this routine simply doesn one thing: find a > > > raw_hwp_page in raw_hwp_list for a subpage. But the caller needs to 1= ) > > > test RawHwpUnreliable before calls into the routine, and 2) test > > > RawHwpUnreliable before access returned raw_hwp_page*. I think 2nd > > > option will be error-prone and the 1st option is a better one. > > > > > > Maybe I am over-thinking. What do you think? > > > > I think racing code accessing the raw_hwp_list is very unlikely. > > However, it is possible and should be considered. > > -- > > Mike Kravetz > > > > > > > > > -- > > > > Mike Kravetz > > > > > > > > > + p =3D container_of(tnode, struct raw_hwp_page, node= ); > > > > > + if (subpage =3D=3D p->page) { > > > > > + hwp_page =3D p; > > > > > + break; > > > > > + } > > > > > + } > > > > > + > > > > > + return hwp_page; > > > > > } > > > > > > > > > > static unsigned long __folio_free_raw_hwp(struct folio *folio, b= ool move_flag) > > > > > -- > > > > > 2.40.1.606.ga4b1b128d6-goog > > > > >