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 12283C433EF for ; Tue, 28 Jun 2022 06:26:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A3E5B8E0003; Tue, 28 Jun 2022 02:26:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A153E8E0001; Tue, 28 Jun 2022 02:26:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8B6058E0003; Tue, 28 Jun 2022 02:26:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 7F4FD8E0001 for ; Tue, 28 Jun 2022 02:26:56 -0400 (EDT) Received: from smtpin31.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 4EEA03418C for ; Tue, 28 Jun 2022 06:26:56 +0000 (UTC) X-FDA: 79626661632.31.73EAE12 Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) by imf04.hostedemail.com (Postfix) with ESMTP id 1566340027 for ; Tue, 28 Jun 2022 06:26:53 +0000 (UTC) Received: by mail-pj1-f43.google.com with SMTP id g20-20020a17090a579400b001ed52939d72so6188759pji.4 for ; Mon, 27 Jun 2022 23:26:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=TcBAgP9gjnc09wVHOyQFWt+pLNq9eqMwqHAb/bpUegc=; b=VicilnIIIKtLNGB5HYDHJMg3MfrSGE7V2Tdr8bYYUZOjG8I7ngm9jbEmlmom07s9OL 12Ch8pPumjK/x8jrj7jlkJne7gWMNsdHAaOpVGtr5fqswyNge8WdkQNwTOr3Ba+bA/rZ G8HwfhIrbvREdOfnqaUtWKQseBYMrU09GT+bTOCH2qAhWkKx3TW90/iG/072ouPEMLXE 3fx5j26pisFefNGobjy34kRTj33ADIvLeGvMv6B6PmsPAqGou4DdEcl7RivzWcVxN2mF p2Aryvbb26vHy3c8/Mh/KaQoXly6h6/Qr3nu7AON1YI19B7jnQudE9+WfeeEUCAwUdG0 ottw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=TcBAgP9gjnc09wVHOyQFWt+pLNq9eqMwqHAb/bpUegc=; b=tLuyPWmX7HsVeHTDxsZsxkqfX4s05okYm7xD4OuDeYL8J/Yn2R6UGrl2uyxbp31/em 3NVrQvDxT79BGad6XrnBXZfqB1lEqy/fUUoQrZMQe+ojPLLLi7+1+XC3iEPYroG3CaBf UCED92Fnb3VZWOX3GE8Jj7YhAHuUDofVhftyrsKtPw0MVMXGczIuePCuxXa7cL0ocejx FHNQSBrOVKC0fOrqFA0GQxuysjypdFsnBDhFkTgMSOyczLmOjMAohTlnY9uoDPCrtFml cbYsdlnGHd9/H7r1bS/CNApfd2ZnCsZBQhAC9vVernH9zwzgpCF2KG7B1P4+Dc1SXr/0 vIHw== X-Gm-Message-State: AJIora91idWTVjZNxA7BmgwQ4WBz4maq4MVvKmdKFG8sTZE9JVKz5H2V rP7+BLbFXe8rPWW1PqjS+cuRlA== X-Google-Smtp-Source: AGRyM1uzTbTqvrEmzc2dqdLDQJlzV4L5BZs6spUfy4LlzlszTOI4SrY/5RAniCfFkk21csBVgsO+TA== X-Received: by 2002:a17:90b:1e44:b0:1ec:cc44:be77 with SMTP id pi4-20020a17090b1e4400b001eccc44be77mr26052055pjb.34.1656397612671; Mon, 27 Jun 2022 23:26:52 -0700 (PDT) Received: from localhost ([139.177.225.245]) by smtp.gmail.com with ESMTPSA id y5-20020a1709029b8500b00163f6428657sm8332163plp.6.2022.06.27.23.26.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Jun 2022 23:26:51 -0700 (PDT) Date: Tue, 28 Jun 2022 14:26:47 +0800 From: Muchun Song To: HORIGUCHI =?utf-8?B?TkFPWUEo5aCA5Y+j44CA55u05LmfKQ==?= Cc: Naoya Horiguchi , "linux-mm@kvack.org" , Andrew Morton , David Hildenbrand , Mike Kravetz , Miaohe Lin , Liu Shixin , Yang Shi , Oscar Salvador , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 4/9] mm, hwpoison, hugetlb: support saving mechanism of raw error pages Message-ID: References: <20220623235153.2623702-1-naoya.horiguchi@linux.dev> <20220623235153.2623702-5-naoya.horiguchi@linux.dev> <20220628024121.GF2159330@hori.linux.bs1.fc.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220628024121.GF2159330@hori.linux.bs1.fc.nec.co.jp> ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=VicilnII; dmarc=pass (policy=none) header.from=bytedance.com; spf=pass (imf04.hostedemail.com: domain of songmuchun@bytedance.com designates 209.85.216.43 as permitted sender) smtp.mailfrom=songmuchun@bytedance.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1656397615; 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=TcBAgP9gjnc09wVHOyQFWt+pLNq9eqMwqHAb/bpUegc=; b=bxz3EHA9LNU5fWBd6ZTjXC9nLJZonotTwB/QIfVJZRScDe4DRLz5cJ6l7hjDuumfRKx2Ul BSTA8pHv7vLeW6Ext/+kKNhQCyv4w+Uw4S4B/K866nObSn+6QjQyzYQWdw14lBRtkOeQwF 8VTt51xCFkK57pIIF9RCBM6URBmg/C8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1656397615; a=rsa-sha256; cv=none; b=KA3/pbceuOPMVFI2JNJC9FsDZdyxDOfs4nc9Xl5Fl3LsMwnbja9q2bhr+nxLWVJaNWm8P/ DSognTJbE6yKNz4DHbRp7+K4yO0nORcbVn5VOheZub6QYNwuyKT5vQCRhOWX5PvU2edE9c h9YWyMKa3Ptkxux0FGqmIinlvuuJhmA= X-Stat-Signature: ufjt3xhdu1fmgu5kyzygeomawpscpxpa X-Rspamd-Queue-Id: 1566340027 X-Rspam-User: Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=VicilnII; dmarc=pass (policy=none) header.from=bytedance.com; spf=pass (imf04.hostedemail.com: domain of songmuchun@bytedance.com designates 209.85.216.43 as permitted sender) smtp.mailfrom=songmuchun@bytedance.com X-Rspamd-Server: rspam12 X-HE-Tag: 1656397613-494016 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 Tue, Jun 28, 2022 at 02:41:22AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote: > On Mon, Jun 27, 2022 at 05:26:01PM +0800, Muchun Song wrote: > > On Fri, Jun 24, 2022 at 08:51:48AM +0900, Naoya Horiguchi wrote: > > > From: Naoya Horiguchi > > > > > > When handling memory error on a hugetlb page, the error handler tries to > > > dissolve and turn it into 4kB pages. If it's successfully dissolved, > > > PageHWPoison flag is moved to the raw error page, so that's all right. > > > However, dissolve sometimes fails, then the error page is left as > > > hwpoisoned hugepage. It's useful if we can retry to dissolve it to save > > > healthy pages, but that's not possible now because the information about > > > where the raw error pages is lost. > > > > > > Use the private field of a few tail pages to keep that information. The > > > code path of shrinking hugepage pool uses this info to try delayed dissolve. > > > In order to remember multiple errors in a hugepage, a singly-linked list > > > originated from SUBPAGE_INDEX_HWPOISON-th tail page is constructed. Only > > > simple operations (adding an entry or clearing all) are required and the > > > list is assumed not to be very long, so this simple data structure should > > > be enough. > > > > > > If we failed to save raw error info, the hwpoison hugepage has errors on > > > unknown subpage, then this new saving mechanism does not work any more, > > > so disable saving new raw error info and freeing hwpoison hugepages. > > > > > > Signed-off-by: Naoya Horiguchi > > > > Thanks for your work on this. I have several quesions below. > > > ... > > > > @@ -1499,6 +1499,97 @@ static int try_to_split_thp_page(struct page *page, const char *msg) > > > } > > > > > > #ifdef CONFIG_HUGETLB_PAGE > > > +/* > > > + * Struct raw_hwp_page represents information about "raw error page", > > > + * constructing singly linked list originated from ->private field of > > > + * SUBPAGE_INDEX_HWPOISON-th tail page. > > > + */ > > > +struct raw_hwp_page { > > > + struct llist_node node; > > > + struct page *page; > > > +}; > > > + > > > +static inline struct llist_head *raw_hwp_list_head(struct page *hpage) > > > +{ > > > + return (struct llist_head *)&page_private(hpage + SUBPAGE_INDEX_HWPOISON); > > > +} > > > + > > > +static inline int raw_hwp_unreliable(struct page *hpage) > > > +{ > > > + return page_private(hpage + SUBPAGE_INDEX_HWPOISON_UNRELIABLE); > > > +} > > > + > > > +static inline void set_raw_hwp_unreliable(struct page *hpage) > > > +{ > > > + set_page_private(hpage + SUBPAGE_INDEX_HWPOISON_UNRELIABLE, 1); > > > +} > > > > Why not use HPAGEFLAG(HwpoisonUnreliable, hwpoison_unreliable) directly? > > > > OK, that sounds better, I'll do it. > > > > + > > > +/* > > > + * about race consideration > > > + */ > > > +static inline int hugetlb_set_page_hwpoison(struct page *hpage, > > > + struct page *page) > > > +{ > > > + struct llist_head *head; > > > + struct raw_hwp_page *raw_hwp; > > > + struct llist_node *t, *tnode; > > > + int ret; > > > + > > > + /* > > > + * Once the hwpoison hugepage has lost reliable raw error info, > > > + * there is little mean to keep additional error info precisely, > > > + * so skip to add additional raw error info. > > > + */ > > > + if (raw_hwp_unreliable(hpage)) > > > + return -EHWPOISON; > > > + head = raw_hwp_list_head(hpage); > > > + llist_for_each_safe(tnode, t, head->first) { > > > + struct raw_hwp_page *p = container_of(tnode, struct raw_hwp_page, node); > > > + > > > + if (p->page == page) > > > + return -EHWPOISON; > > > + } > > > + > > > + ret = TestSetPageHWPoison(hpage) ? -EHWPOISON : 0; > > > + /* the first error event will be counted in action_result(). */ > > > + if (ret) > > > + num_poisoned_pages_inc(); > > > + > > > + raw_hwp = kmalloc(sizeof(struct raw_hwp_page), GFP_KERNEL); > > > > This function can be called in atomic context, GFP_ATOMIC should be used > > here. > > OK, I'll use GFP_ATOMIC. > > > > > > + if (raw_hwp) { > > > + raw_hwp->page = page; > > > + llist_add(&raw_hwp->node, head); > > > > The maximum amount of items in the list is one, right? > > The maximum is the number of subpages in the hugepage (512 for 2MB hugepage, > 262144 for 1GB hugepage). > You are right. I have missed something yesterday. > > > > > + } else { > > > + /* > > > + * Failed to save raw error info. We no longer trace all > > > + * hwpoisoned subpages, and we need refuse to free/dissolve > > > + * this hwpoisoned hugepage. > > > + */ > > > + set_raw_hwp_unreliable(hpage); > > > + return ret; > > > + } > > > + return ret; > > > +} > > > + > > > +inline int hugetlb_clear_page_hwpoison(struct page *hpage) > > > +{ > > > + struct llist_head *head; > > > + struct llist_node *t, *tnode; > > > + > > > + if (raw_hwp_unreliable(hpage)) > > > + return -EBUSY; > > > > IIUC, we use head page's PageHWPoison to synchronize hugetlb_clear_page_hwpoison() > > and hugetlb_set_page_hwpoison(), right? If so, who can set hwp_unreliable here? > > Sorry if I might miss your point, but raw_hwp_unreliable is set when > allocating raw_hwp_page failed. hugetlb_set_page_hwpoison() can be called Sorry. I have missed this. Thanks for your clarification. > multiple times on a hugepage and if one of the calls fails, the hwpoisoned > hugepage becomes unreliable. > > BTW, as you pointed out above, if we switch to passing GFP_ATOMIC to kmalloc(), > the kmalloc() never fails, so we no longer have to implement this unreliable No. kmalloc() with GFP_ATOMIC can fail unless I miss something important. > flag, so things get simpler. > > > > > > + ClearPageHWPoison(hpage); > > > + head = raw_hwp_list_head(hpage); > > > + llist_for_each_safe(tnode, t, head->first) { > > > > Is it possible that a new item is added hugetlb_set_page_hwpoison() and we do not > > traverse it (we have cleared page's PageHWPoison)? Then we ignored a real hwpoison > > page, right? > > Maybe you are mentioning the race like below. Yes, that's possible. > Sorry, ignore my previous comments, I'm thinking something wrong. > CPU 0 CPU 1 > > free_huge_page > lock hugetlb_lock > ClearHPageMigratable remove_hugetlb_page() // the page is non-HugeTLB now > unlock hugetlb_lock > get_huge_page_for_hwpoison > lock hugetlb_lock > __get_huge_page_for_hwpoison // cannot reach here since it is not a HugeTLB page now. // So this race is impossible. Then we fallback to normal // page handling. Seems there is a new issue here. // // memory_failure() // try_memory_failure_hugetlb() // if (hugetlb) // goto unlock_mutex; // if (TestSetPageHWPoison(p)) { // // This non-HugeTLB page's vmemmap is still optimized. Setting COMPOUND_PAGE_DTOR after hugetlb_vmemmap_restore() might fix this issue, but we will encounter this race as you mentioned below. Thanks. > hugetlb_set_page_hwpoison > allocate raw_hwp_page > TestSetPageHWPoison > update_and_free_page > __update_and_free_page > if (PageHWPoison) > hugetlb_clear_page_hwpoison > TestClearPageHWPoison > // remove all list items > llist_add > unlock hugetlb_lock > > > The end result seems not critical (leaking raced raw_hwp_page?), but > we need fix. > > > > > > + struct raw_hwp_page *p = container_of(tnode, struct raw_hwp_page, node); > > > + > > > + SetPageHWPoison(p->page); > > > + kfree(p); > > > + } > > > + llist_del_all(head); > > > > If the above issue exists, moving ClearPageHWPoison(hpage) to here could > > fix it. We should clear PageHWPoison carefully since the head page is > > also can be poisoned. > > The reason why I put ClearPageHWPoison(hpage) before llist_for_each_safe() > was that raw error page can be the head page. But this can be solved > with some additional code to remember whether raw_hwp_page list has an item > associated with the head page. > > Or another approach in my mind now is to call hugetlb_clear_page_hwpoison() > with taking mf_mutex. > > > > > Thanks. > > Thank you for valuable feedbacks. > > - Naoya Horiguchi > > > > > > + return 0; > > > +} > > > + > > > /* > > > * Called from hugetlb code with hugetlb_lock held. > > > * > > > @@ -1533,7 +1624,7 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags) > > > goto out; > > > } > > > > > > - if (TestSetPageHWPoison(head)) { > > > + if (hugetlb_set_page_hwpoison(head, page)) { > > > ret = -EHWPOISON; > > > goto out; > > > } > > > @@ -1585,7 +1676,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb > > > lock_page(head); > > > > > > if (hwpoison_filter(p)) { > > > - ClearPageHWPoison(head); > > > + hugetlb_clear_page_hwpoison(head); > > > res = -EOPNOTSUPP; > > > goto out; > > > } > > > -- > > > 2.25.1 > > > > > >