Re: [RFC PATCH 11/14] mm: Free folios in a batch in shrink_folio_list()

From: Matthew Wilcox <willy@infradead.org>
To: linux-mm@kvack.org
Cc: Mel Gorman <mgorman@suse.de>
Subject: Re: [RFC PATCH 11/14] mm: Free folios in a batch in shrink_folio_list()
Date: Mon, 4 Sep 2023 04:43:22 +0100	[thread overview]
Message-ID: <ZPVSWjcO/hiEtcvn@casper.infradead.org> (raw)
In-Reply-To: <20230825135918.4164671-12-willy@infradead.org>

On Fri, Aug 25, 2023 at 02:59:15PM +0100, Matthew Wilcox (Oracle) wrote:
> Use free_unref_page_batch() to free the folios.  This may increase
> the numer of IPIs from calling try_to_unmap_flush() more often,
> but that's going to be very workload-dependent.

I'd like to propose this as a replacement for this patch.  Queue the
mapped folios up so we can flush them all in one go.  Free the unmapped
ones, and the mapped ones after the flush.

It does change the ordering of mem_cgroup_uncharge_folios() and
the page flush.  I think that's OK.  This is only build-tested;
something has messed up my laptop and I can no longer launch VMs.

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6f13394b112e..526d5bb84622 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1706,14 +1706,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 		struct pglist_data *pgdat, struct scan_control *sc,
 		struct reclaim_stat *stat, bool ignore_references)
 {
+	struct folio_batch free_folios;
 	LIST_HEAD(ret_folios);
-	LIST_HEAD(free_folios);
+	LIST_HEAD(mapped_folios);
 	LIST_HEAD(demote_folios);
 	unsigned int nr_reclaimed = 0;
 	unsigned int pgactivate = 0;
 	bool do_demote_pass;
 	struct swap_iocb *plug = NULL;
 
+	folio_batch_init(&free_folios);
 	memset(stat, 0, sizeof(*stat));
 	cond_resched();
 	do_demote_pass = can_demote(pgdat->node_id, sc);
@@ -1723,7 +1725,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 		struct address_space *mapping;
 		struct folio *folio;
 		enum folio_references references = FOLIOREF_RECLAIM;
-		bool dirty, writeback;
+		bool dirty, writeback, mapped = false;
 		unsigned int nr_pages;
 
 		cond_resched();
@@ -1957,6 +1959,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 					stat->nr_lazyfree_fail += nr_pages;
 				goto activate_locked;
 			}
+			mapped = true;
 		}
 
 		/*
@@ -2111,14 +2114,12 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 		 */
 		nr_reclaimed += nr_pages;
 
-		/*
-		 * Is there need to periodically free_folio_list? It would
-		 * appear not as the counts should be low
-		 */
-		if (unlikely(folio_test_large(folio)))
-			destroy_large_folio(folio);
-		else
-			list_add(&folio->lru, &free_folios);
+		if (mapped) {
+			list_add(&folio->lru, &mapped_folios);
+		} else if (folio_batch_add(&free_folios, folio) == 0) {
+			mem_cgroup_uncharge_folios(&free_folios);
+			free_unref_folios(&free_folios);
+		}
 		continue;
 
 activate_locked_split:
@@ -2182,9 +2183,22 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 
 	pgactivate = stat->nr_activate[0] + stat->nr_activate[1];
 
-	mem_cgroup_uncharge_list(&free_folios);
 	try_to_unmap_flush();
-	free_unref_page_list(&free_folios);
+	while (!list_empty(&mapped_folios)) {
+		struct folio *folio = list_first_entry(&mapped_folios,
+					struct folio, lru);
+
+		list_del(&folio->lru);
+		if (folio_batch_add(&free_folios, folio) > 0)
+			continue;
+		mem_cgroup_uncharge_folios(&free_folios);
+		free_unref_folios(&free_folios);
+	}
+
+	if (free_folios.nr) {
+		mem_cgroup_uncharge_folios(&free_folios);
+		free_unref_folios(&free_folios);
+	}
 
 	list_splice(&ret_folios, folio_list);
 	count_vm_events(PGACTIVATE, pgactivate);

