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 90705C636D7 for ; Tue, 21 Feb 2023 22:28:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 18B726B0071; Tue, 21 Feb 2023 17:28:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 115916B0072; Tue, 21 Feb 2023 17:28:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EF75C6B0073; Tue, 21 Feb 2023 17:28:38 -0500 (EST) 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 DB47B6B0071 for ; Tue, 21 Feb 2023 17:28:38 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id A9A4012080E for ; Tue, 21 Feb 2023 22:28:38 +0000 (UTC) X-FDA: 80492739516.09.8492780 Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by imf28.hostedemail.com (Postfix) with ESMTP id D8E9AC0013 for ; Tue, 21 Feb 2023 22:28:36 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=mspeJSNV; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.173 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1677018516; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=acGnR7zCh6ECcT7oOcZq1a4dkWUWWWZOL9ziUjEaicY=; b=kXEaOzvK6CBIr8mJ9HNMRZLFY9k3o57+jmIqVGkyLk4WnBTuSPmvU0mgH6pzoER0ZfNqdR 2nCx3GVVZfkJFcAIp7zrOitO6ii2vESxtwArfYFe3lL48q0lEUWg6/ud77AZySRRqRw+7a Q2Tz4Lx8gQEcfQ12Ol/xsoIZMjJfdmM= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=mspeJSNV; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.173 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1677018516; a=rsa-sha256; cv=none; b=sLO2URnoYw1oSm31XIFVR8Upca6ah8ZV1F8pQFfp6qE/06bttn8JZSKwYbMUWPMTdpTLcv QpWJb4wVhSc41z6jZlLDoa939dOjAjVrEWOwy5UDBDjstwNbStHlsEYaY4JyygSY2X8adb bBuFaPAJvVzrTsoGCcw5kltnLnEeDmc= Received: by mail-pf1-f173.google.com with SMTP id n20so3410136pfu.12 for ; Tue, 21 Feb 2023 14:28:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=acGnR7zCh6ECcT7oOcZq1a4dkWUWWWZOL9ziUjEaicY=; b=mspeJSNVRBqIDXkMkTX1TDYD6a1Eo1IJp9bzYbQULUXd/gqBtnfSE8Rj+/URLcStAN Hrz8XWNDV+puJljscApSmEJk6hOiKs82+OVRjRkSR3zEbbF4IyQWYlmN/HLnGc16DjQV +wZL/USrDjeMup/N6aDm9dYRHI1OL+KBQRbheet/EmjwQqwmYNxOEQs7+WkhxnL0COmI Yg6L/VVe+ISq6heE6JqAsrcugGTLKoJfj9OSsP8EiuITNPegh6/ZLHBhmML3H3MEKIHu vFlBcMbofCxxpvBDNdcJosSezw4xeBVblKK7Xvn1PWIP32KcilIB5rQJtEXPYt95/PGT 8AeA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=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=acGnR7zCh6ECcT7oOcZq1a4dkWUWWWZOL9ziUjEaicY=; b=bEKyAWyKHCGlpxImRngRCUKfEpz4ey9p60ztYLq/QuI8BZbQSupvGTZDVkmVj5UO34 inD3jq493TOcYTDuh+2hNOEGUREf9jrsULjiPxen1ZzaBU3OT+g6cx2SWxOEScDrNhlp D1ggy0iAR6R9AyJ4l04+5Teqck/GddITX3WO4AotFrhMBEGfQr60FZA7amNozTZvvPQC EvMF05mqRJZYrl9IH3hx1zgk+8BGQAby+rploLPdxilJBa+nenljSgpUm/PIBYNDmy6p hT7LmDKMhUgyTnlW75ygD7EnbYB86gF4hy0kj95LVOQxJkdbCxrFLsSchXN11U0R5mmY 24mg== X-Gm-Message-State: AO0yUKVUwJRVJp2cCyeRjIJ1AbZOw9w+ZfsTdxdhHiDetIQLA9+s/9Az HEU7o/YC15/XWdGRg10u8YPRpIldChzOFRheOmI= X-Google-Smtp-Source: AK7set9Ohhg+8g9jUftr5yftEqHtU3m5cIwYcY9Lwgpz0yF0FzEY0VSzyo5BJ/HxuU7Unca1/oI0PLRMvsZt5jKNcwE= X-Received: by 2002:a63:7e55:0:b0:4fb:97bd:ac25 with SMTP id o21-20020a637e55000000b004fb97bdac25mr789368pgn.6.1677018515624; Tue, 21 Feb 2023 14:28:35 -0800 (PST) MIME-Version: 1.0 References: <20230217085439.2826375-1-stevensd@google.com> <20230217085439.2826375-2-stevensd@google.com> In-Reply-To: From: Yang Shi Date: Tue, 21 Feb 2023 14:28:24 -0800 Message-ID: Subject: Re: [PATCH v4 1/3] mm/khugepaged: refactor collapse_file control flow To: Peter Xu Cc: David Stevens , linux-mm@kvack.org, Matthew Wilcox , Andrew Morton , "Kirill A . Shutemov" , David Hildenbrand , Hugh Dickins , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: D8E9AC0013 X-Rspam-User: X-Stat-Signature: qobs5jex3io7bpmjashmmjnuc3q8y5pt X-HE-Tag: 1677018516-74596 X-HE-Meta: U2FsdGVkX1/04I6vYM7Pfk8Clll9CxLsjuVbacnO7NGqeOgbqBw1VxMsmcM9pfl7lrRIsWMopYFy4oglC5LQwHNwa1hm+XmxHHaK8NZFwNXfU1sTTHVbB6i+kRu16GHIzqCNg34z4pdEFnRoL9O8q0eT5gtSao/n/+Q76Na220VPmw557VVe1ma/UF/0Dc5Yo4HJhEtDH72ggTC7DmMpgtbXoLTibH0i2PX3F41YjujmBks8UHpgpP1e/nI6gmA/3JU7ui6xgP7yFMQoXLORxNf8KyjzmUEoqYCSFWaiCGIrskZhfu8MyDv1ZndHZThJMDtg/mzqXPrR3N/oTh5Y4NJ2jgCFbvfF1UpAmrwEri8Hf/Pd7LYsw7Qnof1nq74neoMkQh683GYEcwYUOOgpg7pwg1IA0RK1sEtuZGOsmiu/79wZqB/3tWK8ttrkL/tADM/yCsIhCgfzlPBzIyB8ulYZQpY8NfoFiZ2fGF5GBg1NPmYExIyHKaM0ztfsv5nijo3o8YsWbdIh4/o3eOI7itmUvmvVMU+101HxKv8Kc7abhFqnLadTpbo8BFbOE4rlC+HuOLTaejyOPv7ombsIBAwjZ+5NilLjtED+UkfkPrYxZTb40preTs5OOobfpLyn6ojSmukAcwvM5Ppx3769EouT1JtoUsZfel29zdayNXGrM91aRSMT6GaZXw/c8mOqAr1WN+NneRYckAwLEJfDTfDhmkou1WBGcr69J/+CDJ0FP8uy+JPeXtslPHgr+eUE9yUV+T0nwnUs4rM6SF0sW2eyXle7XLDUbYJouam3T+Ph7RRx6jT/O7sB2bDRQKhOP3HWFiqij9HVLGsRxScD05Hcicwpd6SDV4+BqXhJouMQUh/F9WuCCsw+NEf3U8U7TIYSLVZFpWFw0EyKJVbdLGfpCZ+SxFobEP4lDnJ+XmFJgeCRyCYJbv6EWiAEZ0s2nBkYgok1g8daA/qsPri j2iy3CfO Qc57EJxAWondNDOF5y10iT4G4VC6YyGEVbPWgIz6RzY++ef13tBEiKkei9dsVu7X5d5+3oCaVi7zKVYpokdP/+taGzTWWRRQcgIBSVod4MY72R8ptL8pUe67VMy0ZJmVo95Kk/ZUnFk/5b7ZD0ix6U6Uvjj9D4Qc/tYFBivTAgMUngEjhMp9I+6gnwluNdFhveD8GhjIHBfQ3hT5+Cd/3uxc5p4/R6CDj7jke262IQzau7T4O2BLvsot1qZuBWcnOlOZ9106WNCIeNGmmooeDab9YDQZByaucDOg8HS03lRYjReYVaU7zxVrAAJkAstBEwd5KKNFdD0nCA4FQGS5aODADJJB50Jxi0m/+uDcqWv0SNulFayi+unNms5gL9xhr/t/zudl5iUqdlPDvERxvwyeq3/SsUD7NwzyESbyNTXX+cSuydaMfcwlv7JSWyNf1MCnz4oP2KkzyFrhi1Kx7bs4XNP84szu8SavGrgfuAgmC7SqHJpOzMk72SwMthSrggJnR7JTHuqfHg05atRniZehpZd5tFOCKxLt5vqzeupO4kMgbZmgkcjdVVw== 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, Feb 21, 2023 at 1:54 PM Peter Xu wrote: > > On Fri, Feb 17, 2023 at 05:54:37PM +0900, David Stevens wrote: > > From: David Stevens > > > > Add a rollback label to deal with failure, instead of continuously > > checking for RESULT_SUCCESS, to make it easier to add more failure > > cases. The refactoring also allows the collapse_file tracepoint to > > include hpage on success (instead of NULL). > > > > Signed-off-by: David Stevens > > --- > > mm/khugepaged.c | 223 ++++++++++++++++++++++++------------------------ > > 1 file changed, 110 insertions(+), 113 deletions(-) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index 8dbc39896811..6a3d6d2e25e0 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -1885,6 +1885,12 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, > > if (result != SCAN_SUCCEED) > > goto out; > > > > + __SetPageLocked(hpage); > > + if (is_shmem) > > + __SetPageSwapBacked(hpage); > > + hpage->index = start; > > + hpage->mapping = mapping; > > + > > /* > > * Ensure we have slots for all the pages in the range. This is > > * almost certainly a no-op because most of the pages must be present > > @@ -1897,16 +1903,10 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, > > xas_unlock_irq(&xas); > > if (!xas_nomem(&xas, GFP_KERNEL)) { > > result = SCAN_FAIL; > > - goto out; > > + goto rollback; > > } > > } while (1); > > > > - __SetPageLocked(hpage); > > - if (is_shmem) > > - __SetPageSwapBacked(hpage); > > - hpage->index = start; > > - hpage->mapping = mapping; > > - > > /* > > * At this point the hpage is locked and not up-to-date. > > * It's safe to insert it into the page cache, because nobody would > > @@ -2123,131 +2123,128 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, > > */ > > try_to_unmap_flush(); > > > > - if (result == SCAN_SUCCEED) { > > - /* > > - * Replacing old pages with new one has succeeded, now we > > - * attempt to copy the contents. > > - */ > > - index = start; > > - list_for_each_entry(page, &pagelist, lru) { > > - while (index < page->index) { > > - clear_highpage(hpage + (index % HPAGE_PMD_NR)); > > - index++; > > - } > > - if (copy_mc_page(hpage + (page->index % HPAGE_PMD_NR), > > - page) > 0) { > > - result = SCAN_COPY_MC; > > - break; > > - } > > - index++; > > - } > > - while (result == SCAN_SUCCEED && index < end) { > > + if (result != SCAN_SUCCEED) > > + goto rollback; > > + > > + /* > > + * Replacing old pages with new one has succeeded, now we > > + * attempt to copy the contents. > > + */ > > + index = start; > > + list_for_each_entry(page, &pagelist, lru) { > > + while (index < page->index) { > > clear_highpage(hpage + (index % HPAGE_PMD_NR)); > > index++; > > } > > + if (copy_mc_page(hpage + (page->index % HPAGE_PMD_NR), > > + page) > 0) { > > + result = SCAN_COPY_MC; > > + goto rollback; > > + } > > + index++; > > + } > > + while (index < end) { > > + clear_highpage(hpage + (index % HPAGE_PMD_NR)); > > + index++; > > } > > > > - if (result == SCAN_SUCCEED) { > > - /* > > - * Copying old pages to huge one has succeeded, now we > > - * need to free the old pages. > > - */ > > - list_for_each_entry_safe(page, tmp, &pagelist, lru) { > > - list_del(&page->lru); > > - page->mapping = NULL; > > - page_ref_unfreeze(page, 1); > > - ClearPageActive(page); > > - ClearPageUnevictable(page); > > - unlock_page(page); > > - put_page(page); > > - } > > + /* > > + * Copying old pages to huge one has succeeded, now we > > + * need to free the old pages. > > + */ > > + list_for_each_entry_safe(page, tmp, &pagelist, lru) { > > + list_del(&page->lru); > > + page->mapping = NULL; > > + page_ref_unfreeze(page, 1); > > + ClearPageActive(page); > > + ClearPageUnevictable(page); > > + unlock_page(page); > > + put_page(page); > > + } > > > > - xas_lock_irq(&xas); > > - if (is_shmem) > > - __mod_lruvec_page_state(hpage, NR_SHMEM_THPS, nr); > > - else > > - __mod_lruvec_page_state(hpage, NR_FILE_THPS, nr); > > + xas_lock_irq(&xas); > > + if (is_shmem) > > + __mod_lruvec_page_state(hpage, NR_SHMEM_THPS, nr); > > + else > > + __mod_lruvec_page_state(hpage, NR_FILE_THPS, nr); > > + > > + if (nr_none) { > > + __mod_lruvec_page_state(hpage, NR_FILE_PAGES, nr_none); > > + /* nr_none is always 0 for non-shmem. */ > > + __mod_lruvec_page_state(hpage, NR_SHMEM, nr_none); > > + } > > + /* Join all the small entries into a single multi-index entry. */ > > + xas_set_order(&xas, start, HPAGE_PMD_ORDER); > > + xas_store(&xas, hpage); > > + xas_unlock_irq(&xas); > > > > - if (nr_none) { > > - __mod_lruvec_page_state(hpage, NR_FILE_PAGES, nr_none); > > - /* nr_none is always 0 for non-shmem. */ > > - __mod_lruvec_page_state(hpage, NR_SHMEM, nr_none); > > - } > > - /* Join all the small entries into a single multi-index entry. */ > > - xas_set_order(&xas, start, HPAGE_PMD_ORDER); > > - xas_store(&xas, hpage); > > - xas_unlock_irq(&xas); > > + folio = page_folio(hpage); > > + folio_mark_uptodate(folio); > > + folio_ref_add(folio, HPAGE_PMD_NR - 1); > > > > - folio = page_folio(hpage); > > - folio_mark_uptodate(folio); > > - folio_ref_add(folio, HPAGE_PMD_NR - 1); > > + if (is_shmem) > > + folio_mark_dirty(folio); > > + folio_add_lru(folio); > > > > - if (is_shmem) > > - folio_mark_dirty(folio); > > - folio_add_lru(folio); > > + /* > > + * Remove pte page tables, so we can re-fault the page as huge. > > + */ > > + result = retract_page_tables(mapping, start, mm, addr, hpage, > > + cc); > > + unlock_page(hpage); > > + goto out; > > + > > +rollback: > > + /* Something went wrong: roll back page cache changes */ > > + xas_lock_irq(&xas); > > + if (nr_none) { > > + mapping->nrpages -= nr_none; > > + shmem_uncharge(mapping->host, nr_none); > > + } > > > > - /* > > - * Remove pte page tables, so we can re-fault the page as huge. > > - */ > > - result = retract_page_tables(mapping, start, mm, addr, hpage, > > - cc); > > - unlock_page(hpage); > > - hpage = NULL; > > - } else { > > - /* Something went wrong: roll back page cache changes */ > > - xas_lock_irq(&xas); > > - if (nr_none) { > > - mapping->nrpages -= nr_none; > > - shmem_uncharge(mapping->host, nr_none); > > + xas_set(&xas, start); > > + xas_for_each(&xas, page, end - 1) { > > + page = list_first_entry_or_null(&pagelist, > > + struct page, lru); > > + if (!page || xas.xa_index < page->index) { > > + if (!nr_none) > > + break; > > + nr_none--; > > + /* Put holes back where they were */ > > + xas_store(&xas, NULL); > > + continue; > > } > > > > - xas_set(&xas, start); > > - xas_for_each(&xas, page, end - 1) { > > - page = list_first_entry_or_null(&pagelist, > > - struct page, lru); > > - if (!page || xas.xa_index < page->index) { > > - if (!nr_none) > > - break; > > - nr_none--; > > - /* Put holes back where they were */ > > - xas_store(&xas, NULL); > > - continue; > > - } > > + VM_BUG_ON_PAGE(page->index != xas.xa_index, page); > > > > - VM_BUG_ON_PAGE(page->index != xas.xa_index, page); > > + /* Unfreeze the page. */ > > + list_del(&page->lru); > > + page_ref_unfreeze(page, 2); > > + xas_store(&xas, page); > > + xas_pause(&xas); > > + xas_unlock_irq(&xas); > > + unlock_page(page); > > + putback_lru_page(page); > > + xas_lock_irq(&xas); > > + } > > + VM_BUG_ON(nr_none); > > + /* > > + * Undo the updates of filemap_nr_thps_inc for non-SHMEM file only. > > + * This undo is not needed unless failure is due to SCAN_COPY_MC. > > + */ > > + if (!is_shmem && result == SCAN_COPY_MC) > > + filemap_nr_thps_dec(mapping); > > > > - /* Unfreeze the page. */ > > - list_del(&page->lru); > > - page_ref_unfreeze(page, 2); > > - xas_store(&xas, page); > > - xas_pause(&xas); > > - xas_unlock_irq(&xas); > > - unlock_page(page); > > - putback_lru_page(page); > > - xas_lock_irq(&xas); > > - } > > - VM_BUG_ON(nr_none); > > - /* > > - * Undo the updates of filemap_nr_thps_inc for non-SHMEM file only. > > - * This undo is not needed unless failure is due to SCAN_COPY_MC. > > - */ > > - if (!is_shmem && result == SCAN_COPY_MC) > > - filemap_nr_thps_dec(mapping); > > + xas_unlock_irq(&xas); > > > > - xas_unlock_irq(&xas); > > + hpage->mapping = NULL; > > > > - hpage->mapping = NULL; > > - } > > + unlock_page(hpage); > > + mem_cgroup_uncharge(page_folio(hpage)); > > + put_page(hpage); > > > > - if (hpage) > > - unlock_page(hpage); > > out: > > VM_BUG_ON(!list_empty(&pagelist)); > > - if (hpage) { > > - mem_cgroup_uncharge(page_folio(hpage)); > > - put_page(hpage); > > - } > > Moving this chunk seems wrong, as it can leak the huge page if > alloc_charge_hpage() failed on mem charging, iiuc. Yeah, good catch. > > And I found that keeping it seems wrong either, because if mem charge > failed we'll uncharge even without charging it before. But that's nothing > about this patch alone. We should be able to just simply check the return value. For example: if (result == SCAN_CGROUP_CHARGE_FAIL) put_page(hpage); > > Posted a patch for this: > > https://lore.kernel.org/all/20230221214344.609226-1-peterx@redhat.com/ > > I _think_ this patch will make sense after rebasing to that fix if that's > correct, but please review it and double check. It is ok too. > > Thanks, > > > - > > trace_mm_khugepaged_collapse_file(mm, hpage, index, is_shmem, addr, file, nr, result); > > return result; > > } > > -- > > 2.39.2.637.g21b0678d19-goog > > > > -- > Peter Xu >