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 BED15C6FD1C for ; Thu, 23 Mar 2023 19:52:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 50AA06B0071; Thu, 23 Mar 2023 15:52:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4BB5A6B0072; Thu, 23 Mar 2023 15:52:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 381EF6B0074; Thu, 23 Mar 2023 15:52:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 26AFC6B0071 for ; Thu, 23 Mar 2023 15:52:08 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id EC401A017A for ; Thu, 23 Mar 2023 19:52:07 +0000 (UTC) X-FDA: 80601209094.10.5D9EBA9 Received: from mail-oi1-f176.google.com (mail-oi1-f176.google.com [209.85.167.176]) by imf01.hostedemail.com (Postfix) with ESMTP id 29E2140003 for ; Thu, 23 Mar 2023 19:52:05 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=qMdmcfnJ; spf=pass (imf01.hostedemail.com: domain of hughd@google.com designates 209.85.167.176 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679601126; a=rsa-sha256; cv=none; b=6ulkEZA7l8Uq/zXjK1uh1wyat9WUyuyTLNwONOhd43iRFpEUI/Tx4AUgHHZ0qMBSVC5fFm 9y9Of8KNGiTedrW3MIaYs+4bijLdHmYM8ypZpuYuvYGdSMM4u+RH8BKG9x6y3O3Ny2vmT7 yk7zOQW38+0p4QNwnenn7cHXnr0qvh8= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=qMdmcfnJ; spf=pass (imf01.hostedemail.com: domain of hughd@google.com designates 209.85.167.176 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679601126; 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=dA0uThRsW2cr4sByCucjOZMV1mZ7yw0K4Dowgh98688=; b=XwXFXVp7fBOKoIfQO09IcQ49hPbH/jEV844XSzQxPx2P6/Wm2aJ9WCSOpAZio3Q/5+F1bv Z6YnweRxhRXpeG2RZOgO/Np23pheH8E4jJyMPJxMGpgnGU58QPIQnaFNmTf/7NRaZAZFYm eADJZYNtILOORDtB45fkIGL8hpa5CjA= Received: by mail-oi1-f176.google.com with SMTP id be16so17126072oib.0 for ; Thu, 23 Mar 2023 12:52:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1679601125; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=dA0uThRsW2cr4sByCucjOZMV1mZ7yw0K4Dowgh98688=; b=qMdmcfnJmLHjjyQrcKEkqOG70E46fZJmY2r5fQ9Gk5cUjLT8NqytvC2G6SbP8ipSxb FSd/JklkIRDBaJ53nGvrkfE3/D5vI3cb8dK/2nkZUok+ncObfA05l1X6cUqLPhiwEIPj ikznRHU+vnF5ILKs9+5LANvvD7O8jlSL9673Z4Bpka5d/P41/RB3OmGHAR/8LMZKnI1T SqSSsQngbKGzgMs7NuWVXYmx1tPx222uSw2t69vNDN+ZJ5O83T5D50tjKGoihDVUCtmE kEnWGzSHZETuJfMCaT7uAo65HDYLuCmHXy9Wj6TKrNCsCvL6vs57VIJB0O2F5trEpTh5 SzHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679601125; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dA0uThRsW2cr4sByCucjOZMV1mZ7yw0K4Dowgh98688=; b=vKxJeFJ0+nAucpXkWnaZX/aazprVHWuWCQR43c0owr/tzVYLs3Rs0bFxvxv4v07OAx hAJBt278cBtNUEbZkO/hGy3xxy1KfEUqymvGkds4h6p4Xmf6FvMNz3tUjvR3oshzwN0O Cg82AusAv0aG4fMm2GH7f01GKI1mo+4RTO45NdCUoqPt5IqgB/desWSGwmRUUE6mFIAQ duwviaiWAiFOUU4/IVBFDCsg/eoxuuq1R0Rhu3/7+M/MNaLkr0Cvwg5nWyUFncIB83qc ZAeG3LcRMgerNSRq5JAZoXPNmGd18DQId0FAtZPOGhOpi88jKXL1sdLZtGektHcuS1PK xiWQ== X-Gm-Message-State: AO0yUKVz1PKzZ4+yJ4bo9IehjnSTD9jKDlJ2zt4MgtmkiAXNJ0jwoXa9 TutSKKDIkqNxTBkLCEeVX27B9w== X-Google-Smtp-Source: AK7set+1+f5IkYHLSVzSFkUCGB9noFTXJKEYTDhV1PLhevmSpvBjRowcQHTZBxIbn40GfTtRy2dEJw== X-Received: by 2002:a54:4785:0:b0:360:cb13:e78a with SMTP id o5-20020a544785000000b00360cb13e78amr3597800oic.58.1679601125102; Thu, 23 Mar 2023 12:52:05 -0700 (PDT) Received: from ripple.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id w40-20020a4a97ab000000b00524f381f681sm7621315ooi.27.2023.03.23.12.52.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Mar 2023 12:52:04 -0700 (PDT) Date: Thu, 23 Mar 2023 12:51:52 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@ripple.attlocal.net To: David Stevens cc: linux-mm@kvack.org, Andrew Morton , Peter Xu , Matthew Wilcox , "Kirill A . Shutemov" , Yang Shi , David Hildenbrand , Hugh Dickins , Jiaqi Yan , linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 1/3] mm/khugepaged: refactor collapse_file control flow In-Reply-To: <20230307052036.1520708-2-stevensd@google.com> Message-ID: <46f29fba-d35-c6a-74d0-38dc6aa15f15@google.com> References: <20230307052036.1520708-1-stevensd@google.com> <20230307052036.1520708-2-stevensd@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Rspam-User: X-Rspamd-Queue-Id: 29E2140003 X-Rspamd-Server: rspam01 X-Stat-Signature: 69zdoxxgb3njo3x3shchqtexj1yxh91w X-HE-Tag: 1679601125-793288 X-HE-Meta: U2FsdGVkX1+8d3YgmjDGRp4mgdGRcY30P3CWVO52KhV6suo53T0+rlWImQggrV7mfWP7djwbqngTD9EiU5SA0yicR3nVEt9YE7iV4Wc807xkHsuwmCdWP/Z72YzxSLXY6mBnfiG252uW2a+oXtu16RefFZEgWZpY1HBzTuGL9eci6a+xOOb15bpU2yKa9offAqLXJLZKUMqyzVogX4ptXjZonvYjzeBqvJ9XhOqkSrhQm+O8Sbd1tNvufkthHNPAFPiEnWQta8RIrohN+T02/TtLdXsMnKtoz2SlQUu674S0jNbrJMsXduECBj7qNf/0u2Ktjek0xBeA6ZyhRX6tJsnaJOaDX/gjoQ5MoeFhaLePyFQerdQCNG0J6205oBPbTAoKXvUM7p7k9HnimRxmOt9si0WtLkHiLfMDQXtg9uLj5ACxfLHEyNiXng1LGuLwq0WgNrDHzhVBfV34qshglARb+Hc1/NrpCg2KtXAI+GDUxyvhLylxqLfj9Bn3Ks8YGfVZ1wMZVnoOHx0Kf2Il8CabH++TCjlVOQ6sjCOlJozEWpluxYxLEJWAIhbEIH6QgRDD/KUOHgs1wSmunU7OodeysyT0wOOuw1vh9WBdryItbE5gBUmWfu5+UA2M5rpUtxUlolX2upCRTfkvW6114uCcumTvvfI40bZoD3UduV5YTZJEG+JDEgVG+vmaw4OlMM8LSa6F/OlmKjDfm6KaK3Xc0p6rZhxmSZXd9D0eY0uEnkzuQmu5FqYoA79nNZG7xbznAHnhGVPIju+P16m59ay/vA5Ior55WzKL0tBdyDV+RrjcMUbRpU/qNaQkBFFeD0O4ngjN3SdyCQLgPLo3Yn7jz+h3kldETf25694lmF0aSkaoOEYE+2wu2GqVdbfF9fsTZ/TZ/K0tqVWdCR8wLTnvq+Z5gX0AlLNy5aqMc+oNWVWszbVRpXU9s52gM0EaZDippjYBCnO0WqZ+Ki3 YfC0ByUL xs0T1bZG+xWpwcdGuIlcWDsboTk24VjERnQC1BNwI2flH/Vp5LhmLc5BuSInVOu4CbDKnPRaPgB85ycQtNjFm2BM0Zw9IVzqORGwqT2p2ZFCPYXkgNlhapE/6pDQ+OIXjkgoEWZmxZ44VcGmgdjBNpoe6XUozJsHBL+WK7m36yNbJfMwTYKRGB4IRv+mdrKLO44guonhpcWLmfwQN8g+LNRG4Beo5viYnA8REscIUtaY+O7f87h25Wht74JCF2p0tcrCvDrAI7fvx1PgLRXYOs5/PHqOf8wR97SwGKzHeCfloCBZb/V0I0MPPnN3TOGZloWBeGt8cREY8geghvXJNJ0gsNmfs1fgZSjZzUyx4IFD7Py5VUaanK96p9QfkHC8m0zp/WZqa4NYh7JkgZYbHEgrZOUhMYTCeIY81JzOA4gI1UuTbN9wXyuRjclKg9iQ12jj8YyTTX8yMPiwpAfVjV6xUMJ9rZNAkgYNx2UxLC3O6ClLfHTYy6AYxg+fwPXdZc/NtM+OKEJw5f3SsU/cUHEyNAHiMcggnFQIgj/j2OAWWaSaD0XSoq/jZeg== 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, 7 Mar 2023, 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 > Acked-by: Peter Xu > Reviewed-by: Yang Shi This one looks like a nice de-indentation to me, even if it's not followed by 2/3 and 3/3. Acked-by: Hugh Dickins > --- > mm/khugepaged.c | 219 ++++++++++++++++++++++++------------------------ > 1 file changed, 108 insertions(+), 111 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 3ea2aa55c2c5..b954e3c685e7 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1907,6 +1907,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 > @@ -1919,16 +1925,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 > @@ -2145,129 +2145,126 @@ 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_highpage(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_highpage(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); > - 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); > + /* > + * 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); > + } > + > + 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); > + put_page(hpage); > > - if (hpage) > - unlock_page(hpage); > out: > VM_BUG_ON(!list_empty(&pagelist)); > - if (hpage) > - put_page(hpage); > - > trace_mm_khugepaged_collapse_file(mm, hpage, index, is_shmem, addr, file, nr, result); > return result; > } > -- > 2.40.0.rc0.216.gc4246ad0f0-goog > >