On Thu, Mar 07, 2024 at 08:56:27AM +0000, Ryan Roberts wrote:
> On 06/03/2024 21:55, Matthew Wilcox wrote:
> > On Wed, Mar 06, 2024 at 07:55:50PM +0000, Matthew Wilcox wrote:
> >> Hang on, I think I see it. It is a race between folio freeing and
> >> deferred_split_scan(), but page migration is absolved. Look:
> >>
> >> CPU 1: deferred_split_scan:
> >> spin_lock_irqsave(split_queue_lock)
> >> list_for_each_entry_safe()
> >> folio_try_get()
> >> list_move(&folio->_deferred_list, &list);
> >> spin_unlock_irqrestore(split_queue_lock)
> >> list_for_each_entry_safe() {
> >> folio_trylock() <- fails
> >> folio_put(folio);
> >>
> >> CPU 2: folio_put:
> >> folio_undo_large_rmappable
> >> ds_queue = get_deferred_split_queue(folio);
> >> spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
> >> list_del_init(&folio->_deferred_list);
> >> *** at this point CPU 1 is not holding the split_queue_lock; the
> >> folio is on the local list. Which we just corrupted ***
>
> Wow, this would have taken me weeks...
It certainly took me hours of staring at the code ...
> I just want to make sure I've understood correctly: CPU1's folio_put()
> is not the last reference, and it keeps iterating through the local
> list. Then CPU2 does the final folio_put() which causes list_del_init()
> to modify the local list concurrently with CPU1's iteration, so CPU1
> probably goes into the weeds?
That is my suggestion for what the problem is, yes.
> > I looked at a few options, but I think we need to keep the refcount
> > elevated until we've got the folios back on the deferred split list.
> > And we can't call folio_put() while holding the split_queue_lock or
> > we'll deadlock. So we need to maintain a list of folios that isn't
> > linked through deferred_list. Anyway, this is basically untested,
> > except that it compiles.
>
> If we can't call folio_put() under spinlock, then I agree.
>
> >
> > Opinions? Better patches?
>
> I assume the fact that one scan is limited to freeing a batch-worth of folios is not a problem? The shrinker will keep calling while there are folios on the deferred list?
I don't think it's a problem. There's no particular requirement as to
how much work a shrinker does, just that it tries to make some progress
(afaik).
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index fd745bcc97ff..0120a47ea7a1 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -3312,7 +3312,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
> > struct pglist_data *pgdata = NODE_DATA(sc->nid);
> > struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
> > unsigned long flags;
> > - LIST_HEAD(list);
> > + struct folio_batch batch;
> > struct folio *folio, *next;
> > int split = 0;
> >
> > @@ -3321,37 +3321,41 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
> > ds_queue = &sc->memcg->deferred_split_queue;
> > #endif
> >
> > + folio_batch_init(&batch);
> > spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
> > /* Take pin on all head pages to avoid freeing them under us */
> > list_for_each_entry_safe(folio, next, &ds_queue->split_queue,
> > _deferred_list) {
> > - if (folio_try_get(folio)) {
> > - list_move(&folio->_deferred_list, &list);
> > - } else {
> > - /* We lost race with folio_put() */
> > - list_del_init(&folio->_deferred_list);
> > - ds_queue->split_queue_len--;
> > + if (!folio_try_get(folio))
> > + continue;
> > + if (!folio_trylock(folio))
> > + continue;
> > + list_del_init(&folio->_deferred_list);
> > + if (folio_batch_add(&batch, folio) == 0) {
> > + --sc->nr_to_scan;
> > + break;
> > }
> > if (!--sc->nr_to_scan)
> > break;
> > }
> > spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
> >
> > - list_for_each_entry_safe(folio, next, &list, _deferred_list) {
> > - if (!folio_trylock(folio))
> > - goto next;
> > - /* split_huge_page() removes page from list on success */
> > + while ((folio = folio_batch_next(&batch)) != NULL) {
> > if (!split_folio(folio))
> > split++;
> > folio_unlock(folio);
> > -next:
> > - folio_put(folio);
> > }
> >
> > spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
> > - list_splice_tail(&list, &ds_queue->split_queue);
> > + while ((folio = folio_batch_next(&batch)) != NULL) {
> > + if (!folio_test_large(folio))
> > + continue;
> > + list_add_tail(&folio->_deferred_list, &ds_queue->split_queue);
> > + }
> > spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
> >
> > + folios_put(&batch);
> > +
> > /*
> > * Stop shrinker if we didn't split any page, but the queue is empty.
> > * This can happen if pages were freed under us.
>
> I've added this patch to my branch and tested (still without the patch that I fingered as the culprit originally, for now). Unfortuantely it is still blowing up at about the same rate, although it looks very different now. I've seen bad things twice. The first time was RCU stalls, but systemd had turned the log level down so no stack trace and I didn't manage to get any further information. The second time, this:
>
> [ 338.519401] Unable to handle kernel paging request at virtual address fffc001b13a8c870
> [ 338.519402] Unable to handle kernel paging request at virtual address fffc001b13a8c870
> [ 338.519407] Mem abort info:
> [ 338.519407] ESR = 0x0000000096000004
> [ 338.519408] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 338.519588] Unable to handle kernel paging request at virtual address fffc001b13a8c870
> [ 338.519591] Mem abort info:
> [ 338.519592] ESR = 0x0000000096000004
> [ 338.519593] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 338.519594] SET = 0, FnV = 0
> [ 338.519595] EA = 0, S1PTW = 0
> [ 338.519596] FSC = 0x04: level 0 translation fault
> [ 338.519597] Data abort info:
> [ 338.519597] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [ 338.519598] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 338.519599] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 338.519600] [fffc001b13a8c870] address between user and kernel address ranges
> [ 338.519602] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [ 338.519605] Modules linked in:
> [ 338.519607] CPU: 43 PID: 3234 Comm: usemem Not tainted 6.8.0-rc5-00465-g279cb41b481e-dirty #3
> [ 338.519610] Hardware name: linux,dummy-virt (DT)
> [ 338.519611] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 338.519613] pc : down_read_trylock+0x2c/0xd0
> [ 338.519618] lr : folio_lock_anon_vma_read+0x74/0x2c8
> [ 338.519623] sp : ffff800087f935c0
> [ 338.519623] x29: ffff800087f935c0 x28: 0000000000000000 x27: ffff800087f937e0
> [ 338.519626] x26: 0000000000000001 x25: ffff800087f937a8 x24: fffffc0007258180
> [ 338.519628] x23: ffff800087f936c8 x22: fffc001b13a8c870 x21: ffff0000f7d51d69
> [ 338.519630] x20: ffff0000f7d51d68 x19: fffffc0007258180 x18: 0000000000000000
> [ 338.519632] x17: 0000000000000001 x16: ffff0000c90ab458 x15: 0000000000000040
> [ 338.519634] x14: ffff0000c8c7b558 x13: 0000000000000228 x12: 000040f22f534640
> [ 338.519637] x11: 0000000000000000 x10: 0000000000000000 x9 : ffff800080338b3c
> [ 338.519639] x8 : ffff800087f93618 x7 : 0000000000000000 x6 : ffff0000c9692f50
> [ 338.519641] x5 : ffff800087f936b0 x4 : 0000000000000001 x3 : ffff0000d70d9140
> [ 338.519643] x2 : 0000000000000001 x1 : fffc001b13a8c870 x0 : fffc001b13a8c870
> [ 338.519645] Call trace:
> [ 338.519646] down_read_trylock+0x2c/0xd0
> [ 338.519648] folio_lock_anon_vma_read+0x74/0x2c8
> [ 338.519650] rmap_walk_anon+0x1d8/0x2c0
> [ 338.519652] folio_referenced+0x1b4/0x1e0
> [ 338.519655] shrink_folio_list+0x768/0x10c8
> [ 338.519658] shrink_lruvec+0x5dc/0xb30
> [ 338.519660] shrink_node+0x4d8/0x8b0
> [ 338.519662] do_try_to_free_pages+0xe0/0x5a8
> [ 338.519665] try_to_free_mem_cgroup_pages+0x128/0x2d0
> [ 338.519667] try_charge_memcg+0x114/0x658
> [ 338.519671] __mem_cgroup_charge+0x6c/0xd0
> [ 338.519672] __handle_mm_fault+0x42c/0x1640
> [ 338.519675] handle_mm_fault+0x70/0x290
> [ 338.519677] do_page_fault+0xfc/0x4d8
> [ 338.519681] do_translation_fault+0xa4/0xc0
> [ 338.519682] do_mem_abort+0x4c/0xa8
> [ 338.519685] el0_da+0x2c/0x78
> [ 338.519687] el0t_64_sync_handler+0xb8/0x130
> [ 338.519689] el0t_64_sync+0x190/0x198
> [ 338.519692] Code: aa0003e1 b9400862 11000442 b9000862 (f9400000)
> [ 338.519693] ---[ end trace 0000000000000000 ]---
>
> The fault is when trying to do an atomic_long_read(&sem->count) here:
>
> struct anon_vma *folio_lock_anon_vma_read(struct folio *folio,
> struct rmap_walk_control *rwc)
> {
> struct anon_vma *anon_vma = NULL;
> struct anon_vma *root_anon_vma;
> unsigned long anon_mapping;
>
> retry:
> rcu_read_lock();
> anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
> if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
> goto out;
> if (!folio_mapped(folio))
> goto out;
>
> anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
> root_anon_vma = READ_ONCE(anon_vma->root);
> if (down_read_trylock(&root_anon_vma->rwsem)) { <<<<<<<
>
> I guess we are still corrupting folios?
I guess so ...
The thought occurs that we don't need to take the folios off the list.
I don't know that will fix anything, but this will fix your "running out
of memory" problem -- I forgot to drop the reference if folio_trylock()
failed. Of course, I can't call folio_put() inside the lock, so may
as well move the trylock back to the second loop.
Again, compile-tessted only.
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fd745bcc97ff..4a2ab17f802d 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3312,7 +3312,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
struct pglist_data *pgdata = NODE_DATA(sc->nid);
struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
unsigned long flags;
- LIST_HEAD(list);
+ struct folio_batch batch;
struct folio *folio, *next;
int split = 0;
@@ -3321,36 +3321,31 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
ds_queue = &sc->memcg->deferred_split_queue;
#endif
+ folio_batch_init(&batch);
spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
- /* Take pin on all head pages to avoid freeing them under us */
+ /* Take ref on all folios to avoid freeing them under us */
list_for_each_entry_safe(folio, next, &ds_queue->split_queue,
_deferred_list) {
- if (folio_try_get(folio)) {
- list_move(&folio->_deferred_list, &list);
- } else {
- /* We lost race with folio_put() */
- list_del_init(&folio->_deferred_list);
- ds_queue->split_queue_len--;
+ if (!folio_try_get(folio))
+ continue;
+ if (folio_batch_add(&batch, folio) == 0) {
+ --sc->nr_to_scan;
+ break;
}
if (!--sc->nr_to_scan)
break;
}
spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
- list_for_each_entry_safe(folio, next, &list, _deferred_list) {
+ while ((folio = folio_batch_next(&batch)) != NULL) {
if (!folio_trylock(folio))
- goto next;
- /* split_huge_page() removes page from list on success */
+ continue;
if (!split_folio(folio))
split++;
folio_unlock(folio);
-next:
- folio_put(folio);
}
- spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
- list_splice_tail(&list, &ds_queue->split_queue);
- spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
+ folios_put(&batch);
/*
* Stop shrinker if we didn't split any page, but the queue is empty.
