| Message ID | 20210715033704.692967-63-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory folios | expand |
On 14 Jul 2021, at 23:35, Matthew Wilcox (Oracle) wrote: > This is the folio equivalent of migrate_page_copy(), which is retained > as a wrapper for filesystems which are not yet converted to folios. > Also convert copy_huge_page() to folio_copy(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > include/linux/migrate.h | 1 + > include/linux/mm.h | 2 +- > mm/folio-compat.c | 6 ++++++ > mm/hugetlb.c | 2 +- > mm/migrate.c | 14 +++++--------- > mm/util.c | 6 +++--- > 6 files changed, 17 insertions(+), 14 deletions(-) > LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com> — Best Regards, Yan, Zi
15.07.2021 06:35, Matthew Wilcox (Oracle) пишет: > This is the folio equivalent of migrate_page_copy(), which is retained > as a wrapper for filesystems which are not yet converted to folios. > Also convert copy_huge_page() to folio_copy(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > include/linux/migrate.h | 1 + > include/linux/mm.h | 2 +- > mm/folio-compat.c | 6 ++++++ > mm/hugetlb.c | 2 +- > mm/migrate.c | 14 +++++--------- > mm/util.c | 6 +++--- > 6 files changed, 17 insertions(+), 14 deletions(-) Hi, I'm getting warnings that might be related to this patch. [37020.191023] BUG: sleeping function called from invalid context at mm/util.c:761 [37020.191383] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 29, name: kcompactd0 [37020.191550] CPU: 1 PID: 29 Comm: kcompactd0 Tainted: G W 5.14.0-rc2-next-20210721-00201-g393e9d2093a1 #8880 [37020.191576] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [37020.191599] [<c010ce15>] (unwind_backtrace) from [<c0108fd5>] (show_stack+0x11/0x14) [37020.191667] [<c0108fd5>] (show_stack) from [<c0a74b1f>] (dump_stack_lvl+0x2b/0x34) [37020.191724] [<c0a74b1f>] (dump_stack_lvl) from [<c0141a41>] (___might_sleep+0xed/0x11c) [37020.191779] [<c0141a41>] (___might_sleep) from [<c0241e07>] (folio_copy+0x3f/0x84) [37020.191817] [<c0241e07>] (folio_copy) from [<c027a7b1>] (folio_migrate_copy+0x11/0x1c) [37020.191856] [<c027a7b1>] (folio_migrate_copy) from [<c027ab65>] (__buffer_migrate_page.part.0+0x215/0x238) [37020.191891] [<c027ab65>] (__buffer_migrate_page.part.0) from [<c027b73d>] (buffer_migrate_page_norefs+0x19/0x28) [37020.191927] [<c027b73d>] (buffer_migrate_page_norefs) from [<c027affd>] (move_to_new_page+0x4d/0x200) [37020.191960] [<c027affd>] (move_to_new_page) from [<c027bc91>] (migrate_pages+0x521/0x72c) [37020.191993] [<c027bc91>] (migrate_pages) from [<c024dbc1>] (compact_zone+0x589/0xb60) [37020.192031] [<c024dbc1>] (compact_zone) from [<c024e1eb>] (proactive_compact_node+0x53/0x6c) [37020.192064] [<c024e1eb>] (proactive_compact_node) from [<c024e713>] (kcompactd+0x20b/0x238) [37020.192096] [<c024e713>] (kcompactd) from [<c013b987>] (kthread+0x123/0x140) [37020.192134] [<c013b987>] (kthread) from [<c0100155>] (ret_from_fork+0x11/0x1c) [37020.192164] Exception stack(0xc1751fb0 to 0xc1751ff8)
On Thu, Jul 22, 2021 at 02:52:28PM +0300, Dmitry Osipenko wrote: > I'm getting warnings that might be related to this patch. Thank you! This is a good report. I've trimmed away some of the unnecessary bits from below: > BUG: sleeping function called from invalid context at mm/util.c:761 > in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 29, name: kcompactd0 This is absolutely a result of this patch: for (i = 0; i < nr; i++) { cond_resched(); copy_highpage(folio_page(dst, i), folio_page(src, i)); } cond_resched() can sleep, of course. This is new; previously only copying huge pages would call cond_resched(). Now every page copy calls cond_resched(). > (___might_sleep) from (folio_copy+0x3f/0x84) > (folio_copy) from (folio_migrate_copy+0x11/0x1c) > (folio_migrate_copy) from (__buffer_migrate_page.part.0+0x215/0x238) > (__buffer_migrate_page.part.0) from (buffer_migrate_page_norefs+0x19/0x28) __buffer_migrate_page() is where we become atomic: if (check_refs) spin_lock(&mapping->private_lock); ... migrate_page_copy(newpage, page); ... if (check_refs) spin_unlock(&mapping->private_lock); > (buffer_migrate_page_norefs) from (move_to_new_page+0x4d/0x200) > (move_to_new_page) from (migrate_pages+0x521/0x72c) > (migrate_pages) from (compact_zone+0x589/0xb60) The obvious solution is just to change folio_copy(): { - unsigned i, nr = folio_nr_pages(src); + unsigned i = 0; + unsigned nr = folio_nr_pages(src); - for (i = 0; i < nr; i++) { - cond_resched(); + for (;;) { copy_highpage(folio_page(dst, i), folio_page(src, i)); + if (i++ == nr) + break; + cond_resched(); } } now it only calls cond_resched() for multi-page folios. But that leaves us with a bit of an ... impediment to using multi-page folios for buffer-head based filesystems (and block devices). I must admit to not knowing the buffer_head locking scheme quite as well as I would like to. Is it possible to drop this spinlock earlier?
22.07.2021 15:29, Matthew Wilcox пишет: > On Thu, Jul 22, 2021 at 02:52:28PM +0300, Dmitry Osipenko wrote: ... > The obvious solution is just to change folio_copy(): > > { > - unsigned i, nr = folio_nr_pages(src); > + unsigned i = 0; > + unsigned nr = folio_nr_pages(src); > > - for (i = 0; i < nr; i++) { > - cond_resched(); > + for (;;) { > copy_highpage(folio_page(dst, i), folio_page(src, i)); > + if (i++ == nr) This works with the ++i precedence change. Thanks! > + break; > + cond_resched(); > } > } > > now it only calls cond_resched() for multi-page folios. ... Thank you for the explanation and for the fix! The fs/ and mm/ are mostly outside of my scope, hope you'll figure out the buffer-head case soon.
On Thu, Jul 22, 2021 at 04:45:59PM +0300, Dmitry Osipenko wrote: > 22.07.2021 15:29, Matthew Wilcox пишет: > > On Thu, Jul 22, 2021 at 02:52:28PM +0300, Dmitry Osipenko wrote: > ... > > The obvious solution is just to change folio_copy(): > > > > { > > - unsigned i, nr = folio_nr_pages(src); > > + unsigned i = 0; > > + unsigned nr = folio_nr_pages(src); > > > > - for (i = 0; i < nr; i++) { > > - cond_resched(); > > + for (;;) { > > copy_highpage(folio_page(dst, i), folio_page(src, i)); > > + if (i++ == nr) > > This works with the ++i precedence change. Thanks! Thanks for testing! (and fixing my bug) I just pushed out an update to for-next with this fix. > The fs/ and mm/ are mostly outside of my scope, hope you'll figure out > the buffer-head case soon. Thanks. We don't need it fixed yet, but probably in the next six months.
On 7/15/21 5:35 AM, Matthew Wilcox (Oracle) wrote: > This is the folio equivalent of migrate_page_copy(), which is retained > as a wrapper for filesystems which are not yet converted to folios. > Also convert copy_huge_page() to folio_copy(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> The way folio_copy() avoids cond_resched() for single page would IMHO deserve a comment though, so it's not buried only in this thread.
On Thu, Aug 12, 2021 at 01:56:24PM +0200, Vlastimil Babka wrote: > On 7/15/21 5:35 AM, Matthew Wilcox (Oracle) wrote: > > This is the folio equivalent of migrate_page_copy(), which is retained > > as a wrapper for filesystems which are not yet converted to folios. > > Also convert copy_huge_page() to folio_copy(). > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > > The way folio_copy() avoids cond_resched() for single page would IMHO deserve a > comment though, so it's not buried only in this thread. I think folio_copy() deserves kernel-doc. /** * folio_copy - Copy the contents of one folio to another. * @dst: Folio to copy to. * @src: Folio to copy from. * * The bytes in the folio represented by @src are copied to @dst. * Assumes the caller has validated that @dst is at least as large as @src. * Can be called in atomic context for order-0 folios, but if the folio is * larger, it may sleep. */
On 8/13/21 6:16 AM, Matthew Wilcox wrote: > On Thu, Aug 12, 2021 at 01:56:24PM +0200, Vlastimil Babka wrote: >> On 7/15/21 5:35 AM, Matthew Wilcox (Oracle) wrote: >> > This is the folio equivalent of migrate_page_copy(), which is retained >> > as a wrapper for filesystems which are not yet converted to folios. >> > Also convert copy_huge_page() to folio_copy(). >> > >> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >> >> Acked-by: Vlastimil Babka <vbabka@suse.cz> >> >> The way folio_copy() avoids cond_resched() for single page would IMHO deserve a >> comment though, so it's not buried only in this thread. > > I think folio_copy() deserves kernel-doc. > > /** > * folio_copy - Copy the contents of one folio to another. > * @dst: Folio to copy to. > * @src: Folio to copy from. > * > * The bytes in the folio represented by @src are copied to @dst. > * Assumes the caller has validated that @dst is at least as large as @src. > * Can be called in atomic context for order-0 folios, but if the folio is > * larger, it may sleep. > */ > LGTM.
diff --git a/include/linux/migrate.h b/include/linux/migrate.h index ba0a554b3eae..6a01de9faff5 100644 --- a/include/linux/migrate.h +++ b/include/linux/migrate.h @@ -52,6 +52,7 @@ extern int migrate_huge_page_move_mapping(struct address_space *mapping, extern int migrate_page_move_mapping(struct address_space *mapping, struct page *newpage, struct page *page, int extra_count); void folio_migrate_flags(struct folio *newfolio, struct folio *folio); +void folio_migrate_copy(struct folio *newfolio, struct folio *folio); int folio_migrate_mapping(struct address_space *mapping, struct folio *newfolio, struct folio *folio, int extra_count); #else diff --git a/include/linux/mm.h b/include/linux/mm.h index deb0f5efaa65..23276330ef4f 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -911,7 +911,7 @@ void __put_page(struct page *page); void put_pages_list(struct list_head *pages); void split_page(struct page *page, unsigned int order); -void copy_huge_page(struct page *dst, struct page *src); +void folio_copy(struct folio *dst, struct folio *src); /* * Compound pages have a destructor function. Provide a diff --git a/mm/folio-compat.c b/mm/folio-compat.c index 3f00ad92d1ff..2ccd8f213fc4 100644 --- a/mm/folio-compat.c +++ b/mm/folio-compat.c @@ -64,4 +64,10 @@ void migrate_page_states(struct page *newpage, struct page *page) folio_migrate_flags(page_folio(newpage), page_folio(page)); } EXPORT_SYMBOL(migrate_page_states); + +void migrate_page_copy(struct page *newpage, struct page *page) +{ + folio_migrate_copy(page_folio(newpage), page_folio(page)); +} +EXPORT_SYMBOL(migrate_page_copy); #endif diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 924553aa8f78..b46f9d09aa94 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5200,7 +5200,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, *pagep = NULL; goto out; } - copy_huge_page(page, *pagep); + folio_copy(page_folio(page), page_folio(*pagep)); put_page(*pagep); *pagep = NULL; } diff --git a/mm/migrate.c b/mm/migrate.c index a86be2bfc9a1..36cdae0a1235 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -613,16 +613,12 @@ void folio_migrate_flags(struct folio *newfolio, struct folio *folio) } EXPORT_SYMBOL(folio_migrate_flags); -void migrate_page_copy(struct page *newpage, struct page *page) +void folio_migrate_copy(struct folio *newfolio, struct folio *folio) { - if (PageHuge(page) || PageTransHuge(page)) - copy_huge_page(newpage, page); - else - copy_highpage(newpage, page); - - migrate_page_states(newpage, page); + folio_copy(newfolio, folio); + folio_migrate_flags(newfolio, folio); } -EXPORT_SYMBOL(migrate_page_copy); +EXPORT_SYMBOL(folio_migrate_copy); /************************************************************ * Migration functions @@ -650,7 +646,7 @@ int migrate_page(struct address_space *mapping, return rc; if (mode != MIGRATE_SYNC_NO_COPY) - migrate_page_copy(newpage, page); + folio_migrate_copy(newfolio, folio); else folio_migrate_flags(newfolio, folio); return MIGRATEPAGE_SUCCESS; diff --git a/mm/util.c b/mm/util.c index 149537120a91..904a75612307 100644 --- a/mm/util.c +++ b/mm/util.c @@ -728,13 +728,13 @@ int __page_mapcount(struct page *page) } EXPORT_SYMBOL_GPL(__page_mapcount); -void copy_huge_page(struct page *dst, struct page *src) +void folio_copy(struct folio *dst, struct folio *src) { - unsigned i, nr = compound_nr(src); + unsigned i, nr = folio_nr_pages(src); for (i = 0; i < nr; i++) { cond_resched(); - copy_highpage(nth_page(dst, i), nth_page(src, i)); + copy_highpage(folio_page(dst, i), folio_page(src, i)); } }
This is the folio equivalent of migrate_page_copy(), which is retained as a wrapper for filesystems which are not yet converted to folios. Also convert copy_huge_page() to folio_copy(). Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- include/linux/migrate.h | 1 + include/linux/mm.h | 2 +- mm/folio-compat.c | 6 ++++++ mm/hugetlb.c | 2 +- mm/migrate.c | 14 +++++--------- mm/util.c | 6 +++--- 6 files changed, 17 insertions(+), 14 deletions(-)