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 0BB59C636D7 for ; Tue, 21 Feb 2023 21:54:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 835A26B0071; Tue, 21 Feb 2023 16:54:12 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7E6066B0072; Tue, 21 Feb 2023 16:54:12 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 686416B0073; Tue, 21 Feb 2023 16:54:12 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 548896B0071 for ; Tue, 21 Feb 2023 16:54:12 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 0E64940A27 for ; Tue, 21 Feb 2023 21:54:12 +0000 (UTC) X-FDA: 80492652744.26.EC293B9 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf28.hostedemail.com (Postfix) with ESMTP id 9E701C000A for ; Tue, 21 Feb 2023 21:54:09 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=TJiWeWkL; spf=pass (imf28.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1677016450; a=rsa-sha256; cv=none; b=1UNAeboY0baImvvIQL4ta64Dy2p477MN/16f8VbSF8/GOe7YzA9RGRNCNDbHS9m+3f7ReH J5j5eeaRuIu7ToVzMBjaPL8FdU3LIgqU6wnyCH8LCnEUiNdCqRedhkAMlGUUcLMTFbHPtR 50+qDF+nbuKdMXAAmz5+ubAGlV+5E3Y= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=TJiWeWkL; spf=pass (imf28.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1677016450; 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=Brdb9PCr7A23HeU1GuCqJ3BEfkvI1UmYfLnZWb/xXjo=; b=WOZNdEXKgRzsOLl2VBN4dmGXariPpAUZaaYAM2ZZTo4aF3cVXSbWKHr02FKm7Gvbn9O2K3 KV7wuMAA9mq28c/3oCQX2t60/akLDgYXDbppkGZkwJWEy+mqZ09n4JUX8nTMrt6TVdJnSU XpgHRhyhXBmoG/sKdoytMfd7R/dmXWU= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677016449; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Brdb9PCr7A23HeU1GuCqJ3BEfkvI1UmYfLnZWb/xXjo=; b=TJiWeWkLZpRtijX0MC3Q6Up2kb8+soRP0Pnk4vwpCC5Jpf9SYpWlUiKeWYpjNRZxwnhItc Yjrhbq6Yv6Yzd4uste+zLeAQUiIvwE28LMPr7eVqa2rYgaBK+wWO22rYcG2ZP84C3OcE5M hMja0sWDxdMz0FBJ4ReuHScYm0wzC7E= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-381-u6xtVkIBMNK8uGp69StZFw-1; Tue, 21 Feb 2023 16:54:07 -0500 X-MC-Unique: u6xtVkIBMNK8uGp69StZFw-1 Received: by mail-qv1-f69.google.com with SMTP id jh21-20020a0562141fd500b0053c23b938a0so2594073qvb.17 for ; Tue, 21 Feb 2023 13:54:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677016447; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Brdb9PCr7A23HeU1GuCqJ3BEfkvI1UmYfLnZWb/xXjo=; b=btutQv5lTzDC2kKSpILrTyYR+1K/6gJKblhQ0CUJYFT3rwh4Sg6yRcicQfBmsecJsh tAUBSGwIsW04daE+tDIg4o8piq7U603B65ufExvTJBMJ4G1mAAK7MkwTGYtfH9PVvrfF urpxmJ7isCjoEOyQY59n+RAAdgbrNerzt3SW75BB8pEIsV4SzsVad+/RB6NtX7aYvFlE aN6lsRBcVXaTv5Zt69SUtPDmYZGWqCoaEFN5v2U7U22UOAInC618/5vu7Kulj8JR6ajj X/qUwQZbOyx6FPL5e1rBULfUaCi6LnmjurhacsIfesJPgIl+KesRpULJUpNLbObG0Sw1 j3/g== X-Gm-Message-State: AO0yUKWdhkR5OPqALd4KUB61tI/FgH38Om35x5kCt/RVTT7kaCQm1TE+ RyoN7GzLfnFg7zZIUhnGSbapOpa0Zg3OuGuGGnTE5XdCuZTDSHZaFJXyyFoVlpJIAbceiZLjN5w 1sfbCBmpeXKA= X-Received: by 2002:ac8:5a0c:0:b0:3bf:a60d:43b9 with SMTP id n12-20020ac85a0c000000b003bfa60d43b9mr4250913qta.4.1677016447102; Tue, 21 Feb 2023 13:54:07 -0800 (PST) X-Google-Smtp-Source: AK7set/BI2njRIWRcNdTMIK1mpGmgjDnnY1Ek67y/6GmgbYqOGneFOtzdi5c7Fn+86kWeJ6FQr4ybg== X-Received: by 2002:ac8:5a0c:0:b0:3bf:a60d:43b9 with SMTP id n12-20020ac85a0c000000b003bfa60d43b9mr4250892qta.4.1677016446755; Tue, 21 Feb 2023 13:54:06 -0800 (PST) Received: from x1n (bras-base-aurron9127w-grc-56-70-30-145-63.dsl.bell.ca. [70.30.145.63]) by smtp.gmail.com with ESMTPSA id z13-20020ac8430d000000b003b9a50c8fa1sm1200878qtm.87.2023.02.21.13.54.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Feb 2023 13:54:05 -0800 (PST) Date: Tue, 21 Feb 2023 16:54:04 -0500 From: Peter Xu To: David Stevens Cc: linux-mm@kvack.org, Matthew Wilcox , Andrew Morton , "Kirill A . Shutemov" , Yang Shi , David Hildenbrand , Hugh Dickins , linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 1/3] mm/khugepaged: refactor collapse_file control flow Message-ID: References: <20230217085439.2826375-1-stevensd@google.com> <20230217085439.2826375-2-stevensd@google.com> MIME-Version: 1.0 In-Reply-To: <20230217085439.2826375-2-stevensd@google.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspam-User: X-Rspamd-Queue-Id: 9E701C000A X-Rspamd-Server: rspam01 X-Stat-Signature: 4bharys18g1zrdw3bx7n3ob7ifx3eh7w X-HE-Tag: 1677016449-267557 X-HE-Meta: U2FsdGVkX18PeffTiTlyVDSjDyiMRmA8V4goCP2wVOXZySANJddQ5rRrdlq14GCLUCByvdu6zPqWEbR8Aas2OvJbN2DNRMMbzp3E+Wn4qDHOtMNLPL5KPOx+TBhiv8CozUPnRat1ANnOtdI5BG86zSFemzEpVkWrhZiZhZdibNfWplA7MbwwOERlx/aubdpYCtf+60qCvQNlHuDjMJLYCvsVQ2jizvKBzoS4UFTZVQeyMMh62Iet60za4RvkXZS70qL4mfS6cHLK6qZbXMTUwNZN17Uk9R/xY5ktS+0+0prmQ8rFhbF7b3JtBY4Cml21/3E7M9qimCSu4O/YvugTpZw5TIhtYBdhButz6HRewSpMLxvLqmWJrNKif3ttgS64cIzpPqC3qjCvEBXOMKZGt13liobmRza+FEEYVLP+hToDzBP/+//6WnSAXOAqG+LKSsW9YbOBKuWWPb9Jj5v9kVyevUmEwWyTO9VorxYwH7j1nYA81niKE9Z1VR1bwN3aKQgNF9MsCFLJwRz4gAI+o396ekcCDJa9vVgqkRX9E1YA1XShnFv2MWynRnx77TPDw5U5vLQp3QrheUZQlpxKAHy91j1/MrjBrUSGt0xA75vGYNfDX3WrdS6EWVC6czpQenIuUMWcLd8oFf8WchKMTmlv/Lj2+iJ8L7adC9lYlt+BMax0bvoUzrejDubsZKzMVQ/S3Y6jM8BF2NX1HTrXDLbfNrW1QqU7P3QgxMQQfWwwP1ywGnNByKBfDQ3F4zV2zNi9eWYXnYvtYnAoqcVAPb9ajGbcCuqFqCiSsR4SKG7aiDj6Q7My96rt2pnPa9PfsQaHJoX3OFfwelvzKgfYppkRhwwqnEVvC15cQz3OS9ja5PJCiwQx+XbJkGm9gs01o4I+mKH7fuSIrWShylEmzZ/Es4TmQiqFg8Em/toesgFyKUB/h+QwHfnPtzWEPe/WdMsC9Lcr23Wi6IDwYKM tpcnkw1a 7SQDyYwjlWyXL6htn+ZUiMOXWOQXrOP6tx7qyuBMzWpeu2yn8qfddGoZHHKIqAGOXNDFTcMWblI/WY6spnlFPMJGnqHp8Rw/Edilcob1WXoAq1TLXcPu/BbCGyITREW/dr2V63cldmv0iWmZUJ+KW0oVp2eeVWi7MZs7UCCQ8qnJLCRAG0yux1t5W0g+8u0ps87sBtKk0etbW5vTljrK5a3dMvACJHLSyw4E+e08YY1Z90w0ffnVBhg8bVY/BFHuy4dK12xYn0UlqcJaYGhER4kmyegIsBWBps2Pl9HLQ47bKMYHGZnP0QL7Clqsly937P3o9MTST4U4LkwtcK1BXuFyKqh2iDeRHwjsxl8lIpcwm7mODG11TvdQjxAYMF3hKkALT96VDBp/77kdO9NpR7TUraki98b1KveAzcCXvUiq3SWCeJL6pgRq6pbdhe3TYkhC5yyu4CprUtDgLK8pglxqNjAG5XKxoyetvb1LSBWoAZHB68a5YRvo0aoi4Cf/h/92YvzjGxWcEgKCR2JApKIXpGSCIOvWeendr7CSXIvAweOWVWK1TSNSTwGYO34Hrb0dR 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 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. 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. 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. 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