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 5B547C636D6 for ; Fri, 17 Feb 2023 23:45:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AB9ED6B0071; Fri, 17 Feb 2023 18:45:13 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A6A2F6B0072; Fri, 17 Feb 2023 18:45:13 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 90AC46B0073; Fri, 17 Feb 2023 18:45:13 -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 829B96B0071 for ; Fri, 17 Feb 2023 18:45:13 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 5FD8E120332 for ; Fri, 17 Feb 2023 23:45:13 +0000 (UTC) X-FDA: 80478417306.24.49F2203 Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by imf30.hostedemail.com (Postfix) with ESMTP id 9BAC08000F for ; Fri, 17 Feb 2023 23:45:11 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=BPQlZ4B7; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf30.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.41 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1676677511; 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=ZxS+9RgiWjgQdh5YVChlSHlv0qg+BOH35aN+0obnUU8=; b=4cSllK5ax0U152pG4qut+KKT+oxK/gyIYuZXtyEYOmSgvypZXqlqmSwuGKv5dA4LYz0v8M YgKvX9FzLu/X1r7JkNLuCSKYmQMGR6Bpu6VaDBivaqWiRAwbQ0Ma5UpXxD9qJLgwODziBi IzksBFqfvf/8npK+eAcHN62hdN6CprU= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=BPQlZ4B7; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf30.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.41 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1676677511; a=rsa-sha256; cv=none; b=RQCfA5hFWcPFq8MIYMd79qTxpy9/ncQSKWjP4BfTRB+Cwp2Jh62M6d2FJkp/umKdhGMNst 9nRu1iFTnu40oGvZS5xT2BJPAzNwevI7wUYAZ3PnJ3UH2skyavV6Gu4uDCHgvTJ8t5tt6F +rsqbXsSon1fKxijbsBPXmeNz1zW3fc= Received: by mail-pj1-f41.google.com with SMTP id ev7-20020a17090aeac700b002341621377cso2793978pjb.2 for ; Fri, 17 Feb 2023 15:45:11 -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=ZxS+9RgiWjgQdh5YVChlSHlv0qg+BOH35aN+0obnUU8=; b=BPQlZ4B7gjKpSaCiNyr9zpk5Hau1atVDsU0aO0va0wBByttZ+bdxdrjycdKAqZAnY0 rwLf3TGMa9Ngz2zNGBlxvqMimTHKcOLIsMphW7slpM0kDF552zILOPvh2NUeDLrq/His J9mSPiFee7xhjp5ob0DG5dRyVR5gJli55MWvgkcNraCh5DF4PItFmbuowsOTaJGIO4Ju tcdtFcVhPuDDaU+ufOZHm50bwvUWthGUHgHYwgcn3tpBSzo1UyMMomBcoYRUplSUygGR p2s+D1kzoYZQciEgY8PZk/ogBsjgFtx2TmPe7Z0u6aDjkT74jmyWLZvOyhnE4nRBwguL 8oRA== 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=ZxS+9RgiWjgQdh5YVChlSHlv0qg+BOH35aN+0obnUU8=; b=GPor8jY/alPc+2EA4+N/YX53jBBgft5rB0SVD4vqcNEFkwjLNGnYopedFW/BxUwP5Q 5KJqplvFxMF/5sRMmsKkNeXzWCcaFEFEnLTaJiiDawecpAEFXttZtLlFfAzAEmiQeQSW ZXLhn3tSItOE0vX7s4zrqY0h0ooI6FLzkuL5q7lX3LkE9u9XnKTcJ2UIpB85nhDN+2bt RyaX85xsPPyWildG18ROWCzOmHeN5Il0V5LEjy1/H/KH6d/gZKNySyDjXF8E8CMAXNZx c+8Cu4Lv7fTUvRMyMCoRu47HYAyTmaYdPlariXOSjYZgp5GFKuqXcTLx4+8Hoora8xA0 5H0A== X-Gm-Message-State: AO0yUKW9sifok0mkjfzipRqXEVCyjERV0yh5GZH4WNTciljqCmrqIg64 V+Q1uEngo7APve58IKdd3Vx+qsz3YO9eepSCkTs= X-Google-Smtp-Source: AK7set/GBttqOHZqBwe/yN2PAJZxyDT6bC6cEOqxqLDDwsoJIa7t75iHD6c/UPHy+HZ2+vfH+XH1gHaDbnLpJAocMYM= X-Received: by 2002:a17:90b:1e47:b0:230:b973:a726 with SMTP id pi7-20020a17090b1e4700b00230b973a726mr1861002pjb.23.1676677510260; Fri, 17 Feb 2023 15:45:10 -0800 (PST) MIME-Version: 1.0 References: <20230217085439.2826375-1-stevensd@google.com> <20230217085439.2826375-2-stevensd@google.com> In-Reply-To: <20230217085439.2826375-2-stevensd@google.com> From: Yang Shi Date: Fri, 17 Feb 2023 15:44:58 -0800 Message-ID: Subject: Re: [PATCH v4 1/3] mm/khugepaged: refactor collapse_file control flow To: David Stevens Cc: linux-mm@kvack.org, Peter Xu , 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-Queue-Id: 9BAC08000F X-Rspamd-Server: rspam09 X-Rspam-User: X-Stat-Signature: 4sp98do9ownpnq9znaja87684qcd8iit X-HE-Tag: 1676677511-433980 X-HE-Meta: U2FsdGVkX19x4PP+ytE7MxPVWG2KFTouKfZ+cIrVbT6TyF7QwQE5HqA6HUi48UWWhbauT2J5m/nrSWdfNIhgV03k1QFHGWuTaVIrXfCAWNhhFIzK8OiMegFd0WebLik2j14ODhsIhID6WcAlvxSrC8UupoNqCjiKKgsBswmzGNqLrA/iEeqoVvP2sN6BV6go4HY1bJADOV+ep5dc7omMIsBNNEBPncxM6sC31Kk02mpxvQWN4LW+mvdF+DyHJgnPpbbrsPaz1fTZYikyL3s6Z1TsQdD1aq0XqM7sK8IBoN30nSuPVXkWz84r2cgirx/SGYg1rQhZxwo6mwtgbNL990UOPUYfj34Zakq4PJP+PZ/4CyeGCNnDEBxuUusFhnLou0I8U1+C3MT5wtrHcHo2KFa5E8ftYCHRCyXe0u3KHdK9Z6kmecMjxu8MTSW0pgJoIY+R04JyjbnJH5j+x9mx+ucN43V3W2o5SqMxVC4eWjg+KsvnztdwE1oV48lPkzMQ7Tpb3U9OR3w+dBGdMatVyFbw7GO0ERnTGUahaLrJJ2TOi9MgAY1Zf3Z8UVC77kTFOoN7uTMu6jn/O39xE5lebyy+xlL1tM+Pxa/rDFxUxnI5RbYkys6AwN9irCtRGL1MrWUg6FmicCX3rYBpLkIuk5u7MTYibQYVyUQhnHEL64ERrB9JKi5Oa6/YGsTZtNzz2zagX7+XFvEqXSwhlp+XUoqxq1uOrBD6DjW0Ko0leNBYQmOkKt29O1d9Z3s+KlbWe/oLMq5dypJG/g1kVg46yxZW/dtAsT5fZTYNrok7MJZu+DRbgqLnV1+XEEF2aiNP7l6j7i4gcVIOV99+CztAQk/od1j6WGPbqeDiblH9riE0qT3AQvUgVHZxkJPYyBdw/0jWp+y8TcbCh8ITVohxA6ecEMIQKCNLV7WrSqGnpPNvEG4TslD0OkID55kN8Rz6nqkPyzxg4eDiHVPJ09U Xu3U1ywt KnxoRy4xkwKR/fbab8ORFuqGEQ5IeRNjThwVvnQ+WfjZiIMo6w7ydmL1CJh0/BCSRhZCq8xJVSHApB3Z/t31Hcc7S0E2K01z3aBvEuo6FCU+paBpITd76//hucVjWmzPabY7UHb0ov3h5gestqXGbDlFyvedWhXcCn+HkEguv5xwJuVeQJr7LJ49OQ2JEZsC70CgVftnw5lXB7jGuePEPPlwqlDCD7icZJlBx0NWPU5XpgIUyoZzf7+EvUDxV1qGQqH784siiybjeUPVar8cwOpqC1i8+1wyjQmx3F9uAjmYTTIm7f8X6oVZmbv5Tb1aCHd7bdg9MSbZZny10DDAYS8NiNaN1AWGh6RSgSBLPnN+2hG61MD+uSI+A2BVw5oytsqXStBLbxr3/b4wcZq6rnUSgBM9LQp47cNNr2zO4Y5bRs5Q= 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 12:55 AM 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 The refactor looks good to me. Reviewed-by: Yang Shi > --- > 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); > - } > - > trace_mm_khugepaged_collapse_file(mm, hpage, index, is_shmem, addr, file, nr, result); > return result; > } > -- > 2.39.2.637.g21b0678d19-goog >