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 5348DC05027 for ; Wed, 8 Feb 2023 12:03:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4632F6B0071; Wed, 8 Feb 2023 07:03:03 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 413836B0072; Wed, 8 Feb 2023 07:03:03 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B4656B0073; Wed, 8 Feb 2023 07:03:03 -0500 (EST) 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 167F66B0071 for ; Wed, 8 Feb 2023 07:03:03 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id BB42E120E6F for ; Wed, 8 Feb 2023 12:03:02 +0000 (UTC) X-FDA: 80443988604.01.08145EF Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by imf20.hostedemail.com (Postfix) with ESMTP id DAC321C000F for ; Wed, 8 Feb 2023 12:02:57 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=PEYbU1Iv; spf=pass (imf20.hostedemail.com: domain of ying.huang@intel.com designates 192.55.52.88 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1675857779; 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=rgKIHIoGrKpHMYPPddMDqxLg5PneXOcyU+kF32pkVXU=; b=NTIWt0VTFNFCvfgX8R/mx3GiYt7sRB2QIXyBwK2lOvs5p6VV1OUMQYnM++SSjLOOsM9PrG gCBnBBWc3ikMFzW35v1hH8H/CnVPZ+CwFTb+RpA0pHe1BXLDalguAMOfcOwUcyMvYw9QCz p3Az4LWzWBkuvRVYZa7Sk/+I8aPjZqo= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=PEYbU1Iv; spf=pass (imf20.hostedemail.com: domain of ying.huang@intel.com designates 192.55.52.88 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1675857779; a=rsa-sha256; cv=none; b=oDNOSy1ZOyl4yixoflIRTy8xHpWILPL/MsDySxLLYS/yFrow9yRTmFsJejWTSx6OoiGNlJ I+xdPDxUlCmiZbYEW/SE0qr1csuXzUQENSA+qR6UcQV360MaCMODNBwbkQfLq1MT5ixGFd YcdiQevi4Ja6c5Zr5cOE0vuiAx6LwBM= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1675857777; x=1707393777; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=IFmlH0YoCQ7yEt8hGVGvzaLAv3SmHwRR0zL6VgMa4Zo=; b=PEYbU1IvSUyqLo9eJsqZzCFJhn9Fkoc36oBbyJBxa1xcxymQQ7iaF78h CXuex8om2TQ3Wlp/jx3r0kWc9MBY+pJebMW84Z2HRJRoHUTEvZOgUvMDM zoEy80NqENOLNf57wFgEwa945rhjWOJ4cIS/PqfphJ/seVPOGnpsH0a83 0Slx4AoDqiMGGfXMAQIfv9cKl+W8UMSRPkzV4w/BF1snjOOvkuE9Qr3iN Tx+NP0YeZjHv0Kk9NhlhUDr3w0q4Zskg9GPOFHb3Rs52S9TTAwplq61aU Y+vbAN0PNttboBnASjat182mfcsmoH8Q1ZDcJbZBv1aGVbGLUon7vUZkN A==; X-IronPort-AV: E=McAfee;i="6500,9779,10614"; a="357170485" X-IronPort-AV: E=Sophos;i="5.97,280,1669104000"; d="scan'208";a="357170485" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Feb 2023 04:02:55 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10614"; a="617194406" X-IronPort-AV: E=Sophos;i="5.97,280,1669104000"; d="scan'208";a="617194406" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Feb 2023 04:02:51 -0800 From: "Huang, Ying" To: Zi Yan Cc: Andrew Morton , , , Yang Shi , Baolin Wang , Oscar Salvador , "Matthew Wilcox" , Bharata B Rao , "Alistair Popple" , haoxin , Minchan Kim , Mike Kravetz , Hyeonggon Yoo <42.hyeyoo@gmail.com> Subject: Re: [PATCH -v4 7/9] migrate_pages: share more code between _unmap and _move References: <20230206063313.635011-1-ying.huang@intel.com> <20230206063313.635011-8-ying.huang@intel.com> Date: Wed, 08 Feb 2023 20:02:02 +0800 In-Reply-To: (Zi Yan's message of "Tue, 07 Feb 2023 09:50:56 -0500") Message-ID: <87zg9ofjr9.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: DAC321C000F X-Rspam-User: X-Stat-Signature: bx4d3opwdz78aacttfoup1xinw1g7mwo X-HE-Tag: 1675857777-887233 X-HE-Meta: U2FsdGVkX1+HbLrPatTVk5Te4Q7VPQU16K/8KLGa4/Yp++qguG4yIScmSOm4f/VVghMYZ2vD4JsQveD2hpOZNXxxL97Ub+7M8kgPGWrygX1Nl6yaDNXi0phUx6xBviCSdbc20sx9GhmXKF+NFtuKNmRprbJRxRbLI32vD+cLxZYxhaGkYvI9TTnfJRdO6h2tRYFOZY/umQaFOUzzRqWiy4oVtUkprMt1XltuQbkmfbzepDWhEa9cEd4MYTB0jrS2lc3iYCLWj9FeKE+TxztkfmaJIVQ5I9nK/aHBQ5vGVamNkyuQbVhOwLyBQ6ti1LdKI1zY5/sGYlvZeaJCXqgMxtra89F2FYjFTkRcGu1Z55gOPSnPhJs27xmj8X/XFUytAFSiXwa5eZQh0jT5esrrfa3JAfKonmpcXs9wodK/wHZ273IxY7xpQjHpR8oc57Qc/s9yF268UxmbVuf8h3IxXrdwjSjrQ+zXRklY2Zt5DpnKR3Hy7cvgTbF/WKOSIlv2dn6TjZuQjNlFlcUX3NXHMzq7BTSjMjnPwg1xc21ftmIJTq7AAI5blr1j0+wg3h8vAelmkf0j2ge7cmwFBP3wpo9VeI39SPoMEF5vrFQn789B9RiprJ8WMuQdyUzV3l9opeVYbRMjne9BE/E2OEMC4RfiQQMmaSFzFH2x7i8mJWdSzEe5+it2xMsMNYjmBdEETx1vByfX6ZUmJ0nNN4APNSw5F1ftTAwgauL9BGzvLZH+kNOmeW7PBBe7auT5afSEv+fdNCaZvLDZhgb2hb+Vno5h26MjIzR363UdDBOUKxJ5TqycDbLR+LgtBwaHVrxVsz4Oc9f+XlCDkA9u9KEuzqNpGaTk36dHv7L4nXLdhSTu7O0vnKarYeqZ/zsFKWmnTFYidmPkpE+Oy3FMkmpkl5LmU9DcUHZ/PZDoX42R/i6MgBJ+DpT+s8noiIBuz9tTtmy4FSnFiCPbTWh1NAT eXmZdst6 bJpo3KJqgsCkqp0I5pg2ZF4tHy4dteiaMhil7B7ldyLQgLWxraylzs9Js7fQVuHFUSV5Zp+haT9ADWQDPEg59kFI+EwNoeAEpSaXN1CiCawL4vqvIgbS/BMbrqkGaa2eaLZk+dpB66msgbWfMKyWq5ctTkxs+WT+cZ81OMgC37T8VB/jPIuWpjzWOedypD3daUDrz+uGbDe0VyNIugqU9/2wy9Lm5FJ7JutEemXv1pdg/v8S4IYWK8QFpZKOI6QBiCblvSZFK9U0WgYqgD08YblWsGkhfRnn+2o4t+CyFNhr/Rnd3Gn52fjJKl39eyQcWlfXKtXoqWiaMLFwNLX0SgE38fDugw59q5q9J//GYMOTb9QWYJhGmciR2ztawCqx0uhFsDPRdj89W3btmDarh6ZVAflkj1/icqBiFx4I+6rIMiZOKfEkc5Y729WLLSvl4GIamaRiH2BzBiDOcWJ5RGPARa+ct8CruQAiqm4K/ByhRef5SDUJW6YsTiPUsTTdg5GDcFRMWy1TRoWVX0dYreqbEp9AmUYI5DGfy 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: Zi Yan writes: > On 6 Feb 2023, at 1:33, Huang Ying wrote: > >> This is a code cleanup patch to reduce the duplicated code between the >> _unmap and _move stages of migrate_pages(). No functionality change >> is expected. >> >> Signed-off-by: "Huang, Ying" >> Cc: Zi Yan >> Cc: Yang Shi >> Cc: Baolin Wang >> Cc: Oscar Salvador >> Cc: Matthew Wilcox >> Cc: Bharata B Rao >> Cc: Alistair Popple >> Cc: haoxin >> Cc: Minchan Kim >> Cc: Mike Kravetz >> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com> >> --- >> mm/migrate.c | 203 ++++++++++++++++++++------------------------------- >> 1 file changed, 81 insertions(+), 122 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 23eb01cfae4c..9378fa2ad4a5 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1037,6 +1037,7 @@ static void __migrate_folio_extract(struct folio *dst, >> static void migrate_folio_undo_src(struct folio *src, >> int page_was_mapped, >> struct anon_vma *anon_vma, >> + bool locked, >> struct list_head *ret) >> { >> if (page_was_mapped) >> @@ -1044,16 +1045,20 @@ static void migrate_folio_undo_src(struct folio *src, >> /* Drop an anon_vma reference if we took one */ >> if (anon_vma) >> put_anon_vma(anon_vma); >> - folio_unlock(src); >> - list_move_tail(&src->lru, ret); >> + if (locked) >> + folio_unlock(src); > > Having a comment would be better. > /* A page that has not been migrated, move it to a list for later restoration */ Emm... the page state has been restored in the previous operations of the function. This is the last step and the page will be moved to "return" list, then the caller of migrate_pages() will call putback_movable_pages(). We have some comments for the function (migrate_folio_undo_src()) as follows, /* Restore the source folio to the original state upon failure */ >> + if (ret) >> + list_move_tail(&src->lru, ret); >> } >> >> /* Restore the destination folio to the original state upon failure */ >> static void migrate_folio_undo_dst(struct folio *dst, >> + bool locked, >> free_page_t put_new_page, >> unsigned long private) >> { >> - folio_unlock(dst); >> + if (locked) >> + folio_unlock(dst); >> if (put_new_page) >> put_new_page(&dst->page, private); >> else >> @@ -1078,13 +1083,42 @@ static void migrate_folio_done(struct folio *src, >> folio_put(src); >> } >> >> -static int __migrate_folio_unmap(struct folio *src, struct folio *dst, >> - int force, bool force_lock, enum migrate_mode mode) >> +/* Obtain the lock on page, remove all ptes. */ >> +static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page, >> + unsigned long private, struct folio *src, >> + struct folio **dstp, int force, bool force_lock, >> + enum migrate_mode mode, enum migrate_reason reason, >> + struct list_head *ret) >> { >> + struct folio *dst; >> int rc = -EAGAIN; >> + struct page *newpage = NULL; >> int page_was_mapped = 0; >> struct anon_vma *anon_vma = NULL; >> bool is_lru = !__PageMovable(&src->page); >> + bool locked = false; >> + bool dst_locked = false; >> + >> + if (!thp_migration_supported() && folio_test_transhuge(src)) >> + return -ENOSYS; >> + >> + if (folio_ref_count(src) == 1) { >> + /* Folio was freed from under us. So we are done. */ >> + folio_clear_active(src); >> + folio_clear_unevictable(src); >> + /* free_pages_prepare() will clear PG_isolated. */ >> + list_del(&src->lru); >> + migrate_folio_done(src, reason); >> + return MIGRATEPAGE_SUCCESS; >> + } >> + >> + newpage = get_new_page(&src->page, private); >> + if (!newpage) >> + return -ENOMEM; >> + dst = page_folio(newpage); >> + *dstp = dst; >> + >> + dst->private = NULL; >> >> if (!folio_trylock(src)) { >> if (!force || mode == MIGRATE_ASYNC) >> @@ -1119,6 +1153,7 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst, >> >> folio_lock(src); >> } >> + locked = true; >> >> if (folio_test_writeback(src)) { >> /* >> @@ -1133,10 +1168,10 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst, >> break; >> default: >> rc = -EBUSY; >> - goto out_unlock; >> + goto out; >> } >> if (!force) >> - goto out_unlock; >> + goto out; >> folio_wait_writeback(src); >> } >> >> @@ -1166,7 +1201,8 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst, >> * This is much like races on refcount of oldpage: just don't BUG(). >> */ >> if (unlikely(!folio_trylock(dst))) >> - goto out_unlock; >> + goto out; >> + dst_locked = true; >> >> if (unlikely(!is_lru)) { >> __migrate_folio_record(dst, page_was_mapped, anon_vma); >> @@ -1188,7 +1224,7 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst, >> if (!src->mapping) { >> if (folio_test_private(src)) { >> try_to_free_buffers(src); >> - goto out_unlock_both; >> + goto out; >> } >> } else if (folio_mapped(src)) { >> /* Establish migration ptes */ >> @@ -1203,74 +1239,26 @@ static int __migrate_folio_unmap(struct folio *src, struct folio *dst, >> return MIGRATEPAGE_UNMAP; >> } >> >> - if (page_was_mapped) >> - remove_migration_ptes(src, src, false); >> - >> -out_unlock_both: >> - folio_unlock(dst); >> -out_unlock: >> - /* Drop an anon_vma reference if we took one */ >> - if (anon_vma) >> - put_anon_vma(anon_vma); >> - folio_unlock(src); >> out: >> - >> - return rc; >> -} >> - >> -/* Obtain the lock on page, remove all ptes. */ >> -static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page, >> - unsigned long private, struct folio *src, >> - struct folio **dstp, int force, bool force_lock, >> - enum migrate_mode mode, enum migrate_reason reason, >> - struct list_head *ret) >> -{ >> - struct folio *dst; >> - int rc = MIGRATEPAGE_UNMAP; >> - struct page *newpage = NULL; >> - >> - if (!thp_migration_supported() && folio_test_transhuge(src)) >> - return -ENOSYS; >> - >> - if (folio_ref_count(src) == 1) { >> - /* Folio was freed from under us. So we are done. */ >> - folio_clear_active(src); >> - folio_clear_unevictable(src); >> - /* free_pages_prepare() will clear PG_isolated. */ >> - list_del(&src->lru); >> - migrate_folio_done(src, reason); >> - return MIGRATEPAGE_SUCCESS; >> - } >> - >> - newpage = get_new_page(&src->page, private); >> - if (!newpage) >> - return -ENOMEM; >> - dst = page_folio(newpage); >> - *dstp = dst; >> - >> - dst->private = NULL; >> - rc = __migrate_folio_unmap(src, dst, force, force_lock, mode); >> - if (rc == MIGRATEPAGE_UNMAP) >> - return rc; >> - >> /* >> * A page that has not been migrated will have kept its >> * references and be restored. >> */ >> /* restore the folio to right list. */ > > This comment is stale. Probably should be > /* Keep the folio and we will try it again later */ Good catch! Will revise this in the next version. Best Regards, Huang, Ying >> - if (rc != -EAGAIN && rc != -EDEADLOCK) >> - list_move_tail(&src->lru, ret); >> + if (rc == -EAGAIN || rc == -EDEADLOCK) >> + ret = NULL; >> >> - if (put_new_page) >> - put_new_page(&dst->page, private); >> - else >> - folio_put(dst); >> + migrate_folio_undo_src(src, page_was_mapped, anon_vma, locked, ret); >> + migrate_folio_undo_dst(dst, dst_locked, put_new_page, private); >> >> return rc; >> } >> >> -static int __migrate_folio_move(struct folio *src, struct folio *dst, >> - enum migrate_mode mode) >> +/* Migrate the folio to the newly allocated folio in dst. */ >> +static int migrate_folio_move(free_page_t put_new_page, unsigned long private, >> + struct folio *src, struct folio *dst, >> + enum migrate_mode mode, enum migrate_reason reason, >> + struct list_head *ret) >> { >> int rc; >> int page_was_mapped = 0; >> @@ -1283,12 +1271,8 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst, >> list_del(&dst->lru); >> >> rc = move_to_new_folio(dst, src, mode); >> - >> - if (rc == -EAGAIN) { >> - list_add(&dst->lru, prev); >> - __migrate_folio_record(dst, page_was_mapped, anon_vma); >> - return rc; >> - } >> + if (rc) >> + goto out; >> >> if (unlikely(!is_lru)) >> goto out_unlock_both; >> @@ -1302,70 +1286,45 @@ static int __migrate_folio_move(struct folio *src, struct folio *dst, >> * unsuccessful, and other cases when a page has been temporarily >> * isolated from the unevictable LRU: but this case is the easiest. >> */ >> - if (rc == MIGRATEPAGE_SUCCESS) { >> - folio_add_lru(dst); >> - if (page_was_mapped) >> - lru_add_drain(); >> - } >> + folio_add_lru(dst); >> + if (page_was_mapped) >> + lru_add_drain(); >> >> if (page_was_mapped) >> - remove_migration_ptes(src, >> - rc == MIGRATEPAGE_SUCCESS ? dst : src, false); >> + remove_migration_ptes(src, dst, false); >> >> out_unlock_both: >> folio_unlock(dst); >> - /* Drop an anon_vma reference if we took one */ >> - if (anon_vma) >> - put_anon_vma(anon_vma); >> - folio_unlock(src); >> + set_page_owner_migrate_reason(&dst->page, reason); >> /* >> * If migration is successful, decrease refcount of dst, >> * which will not free the page because new page owner increased >> * refcounter. >> */ >> - if (rc == MIGRATEPAGE_SUCCESS) >> - folio_put(dst); >> - >> - return rc; >> -} >> - >> -/* Migrate the folio to the newly allocated folio in dst. */ >> -static int migrate_folio_move(free_page_t put_new_page, unsigned long private, >> - struct folio *src, struct folio *dst, >> - enum migrate_mode mode, enum migrate_reason reason, >> - struct list_head *ret) >> -{ >> - int rc; >> - >> - rc = __migrate_folio_move(src, dst, mode); >> - if (rc == MIGRATEPAGE_SUCCESS) >> - set_page_owner_migrate_reason(&dst->page, reason); >> - >> - if (rc != -EAGAIN) { >> - /* >> - * A folio that has been migrated has all references >> - * removed and will be freed. A folio that has not been >> - * migrated will have kept its references and be restored. >> - */ >> - list_del(&src->lru); >> - } >> + folio_put(dst); >> >> /* >> - * If migration is successful, releases reference grabbed during >> - * isolation. Otherwise, restore the folio to right list unless >> - * we want to retry. >> + * A page that has been migrated has all references removed >> + * and will be freed. >> */ >> - if (rc == MIGRATEPAGE_SUCCESS) { >> - migrate_folio_done(src, reason); >> - } else if (rc != -EAGAIN) { >> - list_add_tail(&src->lru, ret); >> + list_del(&src->lru); >> + /* Drop an anon_vma reference if we took one */ >> + if (anon_vma) >> + put_anon_vma(anon_vma); >> + folio_unlock(src); >> + migrate_folio_done(src, reason); >> >> - if (put_new_page) >> - put_new_page(&dst->page, private); >> - else >> - folio_put(dst); >> + return rc; >> +out: >> + if (rc == -EAGAIN) { >> + list_add(&dst->lru, prev); >> + __migrate_folio_record(dst, page_was_mapped, anon_vma); >> + return rc; >> } >> >> + migrate_folio_undo_src(src, page_was_mapped, anon_vma, true, ret); >> + migrate_folio_undo_dst(dst, true, put_new_page, private); >> + >> return rc; >> } >> >> @@ -1897,9 +1856,9 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page, >> >> __migrate_folio_extract(dst, &page_was_mapped, &anon_vma); >> migrate_folio_undo_src(folio, page_was_mapped, anon_vma, >> - ret_folios); >> + true, ret_folios); >> list_del(&dst->lru); >> - migrate_folio_undo_dst(dst, put_new_page, private); >> + migrate_folio_undo_dst(dst, true, put_new_page, private); >> dst = dst2; >> dst2 = list_next_entry(dst, lru); >> } >> -- >> 2.35.1 > > Everything else looks good to me, just need to fix the two comments above. > Reviewed-by: Zi Yan > > -- > Best Regards, > Yan, Zi