Re: [PATCH v3 10/18] mm: Allow non-hugetlb large folios to be batch processed

[Matthew Wilcox <willy@infradead.org>]

On Sun, Mar 10, 2024 at 08:23:12AM +0000, Ryan Roberts wrote:
> It doesn't sound completely impossible to me that there is a rare error path that accidentally folio_put()s an extra time...

Your debug below seems to prove that it's an extra folio_put()
somewhere.

> >         list_for_each_entry_safe(folio, next, &ds_queue->split_queue,
> >                                                         _deferred_list) {
> > +		VM_BUG_ON_FOLIO(folio_nid(folio) != sc->nid, folio);
> > +		VM_BUG_ON_FOLIO(folio_order(folio) < 2, folio);
> >                 list_del_init(&folio->_deferred_list);
> > 
> > (also testing the hypothesis that somehow a split folio has ended up
> > on the deferred split list)
> 
> OK, ran with these checks, and get the following oops:
> 
> [  411.719461] page:0000000059c1826b refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x8c6a40
> [  411.720807] page:0000000059c1826b refcount:0 mapcount:-128 mapping:0000000000000000 index:0x1 pfn:0x8c6a40
> [  411.721792] flags: 0xbfffc0000000000(node=0|zone=2|lastcpupid=0xffff)
> [  411.722453] page_type: 0xffffff7f(buddy)
> [  411.722870] raw: 0bfffc0000000000 fffffc001227e808 fffffc002a857408 0000000000000000
> [  411.723672] raw: 0000000000000001 0000000000000004 00000000ffffff7f 0000000000000000
> [  411.724470] page dumped because: VM_BUG_ON_FOLIO(!folio_test_large(folio))
> [  411.725176] ------------[ cut here ]------------
> [  411.725642] kernel BUG at include/linux/mm.h:1191!
> [  411.726341] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> [  411.727021] Modules linked in:
> [  411.727329] CPU: 40 PID: 2704 Comm: usemem Not tainted 6.8.0-rc5-00391-g44b0dc848590-dirty #45
> [  411.728179] Hardware name: linux,dummy-virt (DT)
> [  411.728657] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  411.729381] pc : __dump_page+0x450/0x4a8
> [  411.729789] lr : __dump_page+0x450/0x4a8
> [  411.730187] sp : ffff80008b97b6f0
> [  411.730525] x29: ffff80008b97b6f0 x28: 00000000000000e2 x27: ffff80008b97b988
> [  411.731227] x26: ffff80008b97b988 x25: ffff800082105000 x24: 0000000000000001
> [  411.731926] x23: 0000000000000000 x22: 0000000000000001 x21: fffffc00221a9000
> [  411.732630] x20: fffffc00221a9000 x19: fffffc00221a9000 x18: ffffffffffffffff
> [  411.733331] x17: 3030303030303030 x16: 2066376666666666 x15: 076c076f07660721
> [  411.734035] x14: 0728074f0749074c x13: 076c076f07660721 x12: 0000000000000000
> [  411.734757] x11: 0720072007200729 x10: ffff0013f5e756c0 x9 : ffff80008014b604
> [  411.735473] x8 : 00000000ffffbfff x7 : ffff0013f5e756c0 x6 : 0000000000000000
> [  411.736198] x5 : ffff0013a5a24d88 x4 : 0000000000000000 x3 : 0000000000000000
> [  411.736923] x2 : 0000000000000000 x1 : ffff0000c2849b80 x0 : 000000000000003e
> [  411.737621] Call trace:
> [  411.737870]  __dump_page+0x450/0x4a8
> [  411.738229]  dump_page+0x2c/0x70
> [  411.738551]  deferred_split_scan+0x258/0x368
> [  411.738973]  do_shrink_slab+0x184/0x750
> 
> The new VM_BUG_ON_FOLIO(folio_order(folio) < 2, folio); is firing, but then when dump_page() does this:
> 
> 	if (compound) {
> 		pr_warn("head:%p order:%u entire_mapcount:%d nr_pages_mapped:%d pincount:%d\n",
> 				head, compound_order(head),
> 				folio_entire_mapcount(folio),
> 				folio_nr_pages_mapped(folio),
> 				atomic_read(&folio->_pincount));
> 	}
> 
> VM_BUG_ON_FOLIO(!folio_test_large(folio), folio); inside folio_entire_mapcount() fires so we have a nested oops.

Ah.  I'm not sure what 44b0dc848590 is -- probably a local commit, but
I guess you don't have fae7d834c43c in it which would prevent the nested
oops.  Nevertheless, the nested oops does tell us something interesting.

> So the very first line is from the first oops and the rest is from the second. I guess we are racing with the page being freed? I find the change in mapcount interesting; 0 -> -128. Not sure why this would happen?

That's PG_buddy being set in PageType.

> Given the NID check didn't fire, I wonder if this points more towards extra folio_put than corrupt folio nid?

Must be if PG_buddy got set.  But we're still left with the question of
how the page gets freed while still being on the deferred list and
doesn't trigger bad_page(page, "still on deferred list") ...

Anyway, we've made some progress.  We now understand how a freed page
gets its deferred list overwritten -- we've found a split page on the
deferred list with refcount 0, we _assumed_ it was still intact and
overwrote a different page's ->mapping.  And it makes sense that my
patch opened the window wider to hit this problem.

I just checked that free_unref_folios() still does the right thing, and
that also relies on the page not yet being split:

                unsigned int order = folio_order(folio);

                if (order > 0 && folio_test_large_rmappable(folio))
                        folio_undo_large_rmappable(folio);
                if (!free_unref_page_prepare(&folio->page, pfn, order))
                        continue;

so there shouldn't be a point in the page freeing process where the
folio is split before we take it off the deferred list.

split_huge_page_to_list_to_order() is also very careful to take the
ds_queue->split_queue_lock before freezing the folio ref, so it's
not a race with that.  I don't see what it is yet.