LinuxLists.cc - [PATCH v2] mm: page_alloc: dump migrate-failed pages

2021-03-08 20:24:48

Subject: [PATCH v2] mm: page_alloc: dump migrate-failed pages

alloc_contig_range is usually used on cma area or movable zone.
It's critical if the page migration fails on those areas so
dump more debugging message.

page refcount, mapcount with page flags on dump_page are
helpful information to deduce the culprit. Furthermore,
dump_page_owner was super helpful to find long term pinner
who initiated the page allocation.

Admin could enable the dump like this(by default, disabled)

echo "func dump_migrate_failure_pages +p" > control

Admin could disable it.

echo "func dump_migrate_failure_pages =_" > control

Signed-off-by: Minchan Kim <[email protected]>
---
* from v1 - https://lore.kernel.org/linux-mm/[email protected]/
* use dynamic debugging with system wide instead of per-call site - mhocko

mm/page_alloc.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3e4b29ee2b1e..bb0aeca2069c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8453,6 +8453,34 @@ static unsigned long pfn_max_align_up(unsigned long pfn)
pageblock_nr_pages));
}

+#if defined(CONFIG_DYNAMIC_DEBUG) || \
+ (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
+static DEFINE_RATELIMIT_STATE(alloc_contig_ratelimit_state,
+ DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
+int alloc_contig_ratelimit(void)
+{
+ return __ratelimit(&alloc_contig_ratelimit_state);
+}
+
+void dump_migrate_failure_pages(struct list_head *page_list)
+{
+ DEFINE_DYNAMIC_DEBUG_METADATA(descriptor,
+ "migrate failure");
+ if (DYNAMIC_DEBUG_BRANCH(descriptor) &&
+ alloc_contig_ratelimit()) {
+ struct page *page;
+
+ WARN(1, "failed callstack");
+ list_for_each_entry(page, page_list, lru)
+ dump_page(page, "migration failure");
+ }
+}
+#else
+static inline void dump_migrate_failure_pages(struct list_head *page_list)
+{
+}
+#endif
+
/* [start, end) must belong to a single zone. */
static int __alloc_contig_migrate_range(struct compact_control *cc,
unsigned long start, unsigned long end)
@@ -8496,6 +8524,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE);
}
if (ret < 0) {
+ dump_migrate_failure_pages(&cc->migratepages);
putback_movable_pages(&cc->migratepages);
return ret;
}
--
2.30.1.766.gb4fecdf3b7-goog

2021-03-08 21:36:26

On Wed, Mar 10, 2021 at 09:06:48AM -0800, Minchan Kim wrote:
> On Wed, Mar 10, 2021 at 05:46:56PM +0100, Michal Hocko wrote:
> > On Wed 10-03-21 08:05:36, Minchan Kim wrote:
> > > On Wed, Mar 10, 2021 at 02:07:05PM +0100, Michal Hocko wrote:
> > [...]
> > > > The is a lot of churn indeed. Have you considered adding $FOO_lglvl
> > > > variants for those so that you can use them for your particular case
> > > > without affecting most of existing users? Something similar we have
> > > > discussed in other email thread regarding lru_add_drain_all?
> > >
> > > I thought that way but didn't try since it couldn't make them
> > > atomic(For example, other printk place in other context will
> > > affect by the $FOO_lglvl).
> >
> > I do not follow. I meant something like the following (likely incomplete
> > but you should get an idea).
>
> Oh, I thought you wanted to override loglevel temporally.
>
> old_lvl = save_printk_lvl(new level);
> dump_page();
> restore_printk_lvl(old_lvl);
>
> >
> > diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
> > index 3468794f83d2..71b402eb8f78 100644
> > --- a/include/linux/page_owner.h
> > +++ b/include/linux/page_owner.h
> > @@ -14,7 +14,7 @@ extern void __set_page_owner(struct page *page,
> > extern void __split_page_owner(struct page *page, unsigned int nr);
> > extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
> > extern void __set_page_owner_migrate_reason(struct page *page, int reason);
> > -extern void __dump_page_owner(struct page *page);
> > +extern void __dump_page_owner(struct page *page, const char *loglvl);
> > extern void pagetypeinfo_showmixedcount_print(struct seq_file *m,
> > pg_data_t *pgdat, struct zone *zone);
> >
> > @@ -46,10 +46,10 @@ static inline void set_page_owner_migrate_reason(struct page *page, int reason)
> > if (static_branch_unlikely(&page_owner_inited))
> > __set_page_owner_migrate_reason(page, reason);
> > }
> > -static inline void dump_page_owner(struct page *page)
> > +static inline void dump_page_owner(struct page *page, const char *loglvl)
> > {
> > if (static_branch_unlikely(&page_owner_inited))
> > - __dump_page_owner(page);
> > + __dump_page_owner(page, loglvl);
> > }
> > #else
> > static inline void reset_page_owner(struct page *page, unsigned int order)
> > @@ -69,7 +69,7 @@ static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> > static inline void set_page_owner_migrate_reason(struct page *page, int reason)
> > {
> > }
> > -static inline void dump_page_owner(struct page *page)
> > +static inline void dump_page_owner(struct page *page, const char *loglvl)
> > {
> > }
> > #endif /* CONFIG_PAGE_OWNER */
> > diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> > index 9f8117c7cfdd..1b13135d9916 100644
> > --- a/kernel/stacktrace.c
> > +++ b/kernel/stacktrace.c
> > @@ -14,6 +14,18 @@
> > #include <linux/kallsyms.h>
> > #include <linux/stacktrace.h>
> >
> > +void __stack_trace_print(const unsigned long *entries, unsigned int nr_entries,
> > + int spacesconst, char *loglvl)
> > +{
> > + unsigned int i;
> > +
> > + if (WARN_ON(!entries))
> > + return;
> > +
> > + for (i = 0; i < nr_entries; i++)
> > + printk("%s%*c%pS\n", loglvl, 1 + spaces, ' ', (void *)entries[i]);
> > +}
>
> That's exactly I did with introducing pr_loglevel. I wanted to address
> *all places* to use dump_page and stack_trace_print since some folks
> might ask me to fix all the broken place all at once. I'm getting tired
> with such hassle.
>
> void dump_page(const char *log_lvl, struct page *page, const char *reason)
> {
> __dump_page(log_lvl, page, reason);
> dump_page_owner(log_lvl, page);
> }
> EXPORT_SYMBOL(dump_page);

Since it's good to have regardless of the patch, I posted next revision
with removeing ratelimit and put something more in description to
proceed the work.

Posted v3 - https://lore.kernel.org/linux-mm/[email protected]/