On Fri, Mar 08, 2024 at 11:44:35AM +0000, Ryan Roberts wrote:
> > 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.
I think this was a bad thought ...
> Dumping all the CPU back traces with gdb, all the cores (except one) are
> contending on the the deferred split lock.
I'm pretty sure that we can call the shrinker on multiple CPUs at the
same time (can you confirm from the backtrace?)
struct pglist_data *pgdata = NODE_DATA(sc->nid);
struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
so if two CPUs try to shrink the same node, they're going to try to
process the same set of folios. Which means the split will keep failing
because each of them will have a refcount on the folio, and ... yeah.
If so, we need to take the folios off the list (or otherwise mark them)
so that they can't be processed by more than one CPU at a time. And
that leads me to this patch (yes, folio_prep_large_rmappable() is
now vestigial, but removing it increases the churn a bit much for this
stage of debugging)
This time I've boot-tested it. I'm running my usual test-suite against
it now with little expectation that it will trigger. If I have time
I'll try to recreate your setup.
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fd745bcc97ff..2ca033a6c3d8 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -792,8 +792,6 @@ void folio_prep_large_rmappable(struct folio *folio)
{
if (!folio || !folio_test_large(folio))
return;
- if (folio_order(folio) > 1)
- INIT_LIST_HEAD(&folio->_deferred_list);
folio_set_large_rmappable(folio);
}
@@ -3312,7 +3310,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 +3319,40 @@ 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 {
+ list_del_init(&folio->_deferred_list);
+ sc->nr_to_scan--;
+ if (!folio_try_get(folio)) {
/* We lost race with folio_put() */
- list_del_init(&folio->_deferred_list);
ds_queue->split_queue_len--;
+ } else if (folio_batch_add(&batch, folio) == 0) {
+ break;
}
- if (!--sc->nr_to_scan)
+ 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);
+ 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.
diff --git a/mm/internal.h b/mm/internal.h
index 1dfdc3bde1b0..14c21d06f233 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -432,6 +432,8 @@ static inline void prep_compound_head(struct page *page, unsigned int order)
atomic_set(&folio->_entire_mapcount, -1);
atomic_set(&folio->_nr_pages_mapped, 0);
atomic_set(&folio->_pincount, 0);
+ if (order > 1)
+ INIT_LIST_HEAD(&folio->_deferred_list);
}
static inline void prep_compound_tail(struct page *head, int tail_idx)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 025ad1a7df7b..fc9c7ca24c4c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1007,9 +1007,12 @@ static int free_tail_page_prepare(struct page *head_page, struct page *page)
break;
case 2:
/*
- * the second tail page: ->mapping is
- * deferred_list.next -- ignore value.
+ * the second tail page: ->mapping is deferred_list.next
*/
+ if (unlikely(!list_empty(&folio->_deferred_list))) {
+ bad_page(page, "still on deferred list");
+ goto out;
+ }
break;
default:
if (page->mapping != TAIL_MAPPING) {
