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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 27A59C433ED for ; Thu, 8 Apr 2021 18:17:33 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 573C261108 for ; Thu, 8 Apr 2021 18:17:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 573C261108 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A45F76B0036; Thu, 8 Apr 2021 14:17:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9F7B56B006C; Thu, 8 Apr 2021 14:17:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 897B16B006E; Thu, 8 Apr 2021 14:17:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0055.hostedemail.com [216.40.44.55]) by kanga.kvack.org (Postfix) with ESMTP id 6FA0D6B0036 for ; Thu, 8 Apr 2021 14:17:31 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 26DCFA755 for ; Thu, 8 Apr 2021 18:17:31 +0000 (UTC) X-FDA: 78010007502.09.3CDE856 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf15.hostedemail.com (Postfix) with ESMTP id 7FD12A0003A6 for ; Thu, 8 Apr 2021 18:17:29 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 31D01B178; Thu, 8 Apr 2021 18:17:29 +0000 (UTC) Date: Thu, 8 Apr 2021 20:17:26 +0200 From: Oscar Salvador To: Yang Shi Cc: Dave Hansen , Linux MM , Linux Kernel Mailing List , Yang Shi , weixugc@google.com, Huang Ying , Dan Williams , David Hildenbrand Subject: Re: [PATCH 04/10] mm/migrate: make migrate_pages() return nr_succeeded Message-ID: References: <20210401183216.443C4443@viggo.jf.intel.com> <20210401183223.80F1E291@viggo.jf.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 7FD12A0003A6 X-Stat-Signature: g88s1m66ui8ss6ni4dpgxhzw4yiwxujp Received-SPF: none (suse.de>: No applicable sender policy available) receiver=imf15; identity=mailfrom; envelope-from=""; helo=mx2.suse.de; client-ip=195.135.220.15 X-HE-DKIM-Result: none/none X-HE-Tag: 1617905849-978405 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 Thu, Apr 08, 2021 at 10:26:54AM -0700, Yang Shi wrote: > Thanks, Oscar. Yes, kind of. But we have to remember to initialize > "nr_succedded" pointer properly for every migrate_pages() callsite, > right? And it doesn't prevent from returning wrong value if > migrate_pages() is called multiple times by one caller although there > might be not such case (calls migrate_pages() multiple times and care > about nr_succeded) for now. Hi Yang, I might be missing something but AFAICS you only need to initialize nr_succeded pointer where it matters. The local nr_succeeded in migrate_pages() doesn't go, and so it gets initialized every time you call in it to 0. And if you pass a valid pointer, *ret_succeeded == nr_succedeed. I am talking about this (not even compile-tested): diff --git a/include/linux/migrate.h b/include/linux/migrate.h index 3a389633b68f..fd661cb2ce13 100644 --- a/include/linux/migrate.h +++ b/include/linux/migrate.h @@ -40,7 +40,8 @@ extern int migrate_page(struct address_space *mapping, struct page *newpage, struct page *page, enum migrate_mode mode); extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, - unsigned long private, enum migrate_mode mode, int reason); + unsigned long private, enum migrate_mode mode, int reason, + unsigned int *ret_succeeded); extern struct page *alloc_migration_target(struct page *page, unsigned long private); extern int isolate_movable_page(struct page *page, isolate_mode_t mode); extern void putback_movable_page(struct page *page); @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(struct address_space *mapping, static inline void putback_movable_pages(struct list_head *l) {} static inline int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, unsigned long private, enum migrate_mode mode, - int reason) + int reason, unsigned int *ret_succeeded) { return -ENOSYS; } static inline struct page *alloc_migration_target(struct page *page, unsigned long private) diff --git a/mm/compaction.c b/mm/compaction.c index e04f4476e68e..7238e8faff04 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -2364,7 +2364,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) err = migrate_pages(&cc->migratepages, compaction_alloc, compaction_free, (unsigned long)cc, cc->mode, - MR_COMPACTION); + MR_COMPACTION, NULL); trace_mm_compaction_migratepages(cc->nr_migratepages, err, &cc->migratepages); diff --git a/mm/gup.c b/mm/gup.c index e40579624f10..b70d463aa1fc 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1606,7 +1606,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, put_page(pages[i]); if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { + (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, NULL)) { /* * some of the pages failed migration. Do get_user_pages * without migration. diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 24210c9bd843..a17e0f039076 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1852,7 +1852,8 @@ static int __soft_offline_page(struct page *page) if (isolate_page(hpage, &pagelist)) { ret = migrate_pages(&pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE); + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, + NULL); if (!ret) { bool release = !huge; diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 0cdbbfbc5757..28496376de94 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1466,7 +1466,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) if (nodes_empty(nmask)) node_set(mtc.nid, nmask); ret = migrate_pages(&source, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG); + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, + NULL); if (ret) { list_for_each_entry(page, &source, lru) { pr_warn("migrating pfn %lx failed ret:%d ", diff --git a/mm/mempolicy.c b/mm/mempolicy.c index ab51132547b8..df260ed12102 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1103,7 +1103,8 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest, if (!list_empty(&pagelist)) { err = migrate_pages(&pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, + NULL); if (err) putback_movable_pages(&pagelist); } @@ -1355,7 +1356,8 @@ static long do_mbind(unsigned long start, unsigned long len, if (!list_empty(&pagelist)) { WARN_ON_ONCE(flags & MPOL_MF_LAZY); nr_failed = migrate_pages(&pagelist, new_page, NULL, - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, + NULL); if (nr_failed) putback_movable_pages(&pagelist); } diff --git a/mm/migrate.c b/mm/migrate.c index 695a594e5860..087ed407b3ce 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1493,6 +1493,9 @@ static inline int try_split_thp(struct page *page, struct page **page2, * @mode: The migration mode that specifies the constraints for * page migration, if any. * @reason: The reason for page migration. + * @ret_succeeded: A pointer to place the value of the number of pages + * migrated successfully. The caller must pass a valid pointer + * if they care about it. * * The function returns after 10 attempts or if no pages are movable any more * because the list has become empty or no retryable pages exist any more. @@ -1503,7 +1506,7 @@ static inline int try_split_thp(struct page *page, struct page **page2, */ int migrate_pages(struct list_head *from, new_page_t get_new_page, free_page_t put_new_page, unsigned long private, - enum migrate_mode mode, int reason) + enum migrate_mode mode, int reason, unsigned int *ret_succeeded) { int retry = 1; int thp_retry = 1; @@ -1654,6 +1657,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, if (!swapwrite) current->flags &= ~PF_SWAPWRITE; + if (ret_succeeded) + *ret_succeeded = nr_succeeded; + return rc; } @@ -1723,7 +1729,8 @@ static int do_move_pages_to_node(struct mm_struct *mm, }; err = migrate_pages(pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, + NULL); if (err) putback_movable_pages(pagelist); return err; @@ -2230,7 +2237,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma, list_add(&page->lru, &migratepages); nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page, NULL, node, MIGRATE_ASYNC, - MR_NUMA_MISPLACED); + MR_NUMA_MISPLACED, NULL); if (nr_remaining) { if (!list_empty(&migratepages)) { list_del(&page->lru); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 46f3d594369d..0c1bbadd5ca3 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, cc->nr_migratepages -= nr_reclaimed; ret = migrate_pages(&cc->migratepages, alloc_migration_target, - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, + NULL); } if (ret < 0) { putback_movable_pages(&cc->migratepages); As I said I might be missing a point here, but I cannot see the problem you describe here. -- Oscar Salvador SUSE L3