Skip to contentSkip to conversation

memcg: sink mem_cgroup_uncharge_folios() into free_unref_folios()

PATCH In review Ridong Chen 2 participants 3 messages Aug 27 – Aug 28

Subsystems (5)
Memory management MM core Page allocator MGLRU +1 more subsystems
lore
Share & tools
lore

Ridong Chen

author
#0 lore raw

PATCH 1/1 memcg: sink mem_cgroup_uncharge_folios() into free_unref_folios()

From: Ridong Chen <[email protected]>

Every caller of free_unref_folios() invokes mem_cgroup_uncharge_folios() on the same batch immediately beforehand. This pattern is duplicated across shrink_folio_list(), move_folios_to_lru(), folio_batch_move_lru() and folios_put_refs().

Move the uncharge into free_unref_folios() itself so the batch is uncharged in one place before the folios are freed. This removes the repeated boilerplate at every call site and makes it impossible to free a batch without uncharging it first. No functional change intended.

Signed-off-by Ridong Chen <[email protected]>
---
 mm/folio.c      | 5 +----
 mm/page_alloc.c | 1 +
 mm/vmscan.c     | 7 +------
 3 files changed, 3 insertions(+), 10 deletions(-)
mm/folio.c +1 -4
index c02dcea9c03c..2edbba47be1e 100644
@@ -199,10 +199,8 @@ static void folio_batch_move_lru(struct folio_batch *fbatch, move_fn_t move_fn)
 		lruvec_unlock_irqrestore(lruvec, flags);
 
 	/* Cleanup filtered dead folios. */
-	if (is_lru_add) {
-		mem_cgroup_uncharge_folios(&free_fbatch);
+	if (is_lru_add)
 		free_unref_folios(&free_fbatch);
-	}
 
 	folios_put(fbatch);
 }

@@ -1030,7 +1028,6 @@ void folios_put_refs(struct folio_batch *folios, unsigned int *refs)
 	}
 
 	folios->nr = j;
-	mem_cgroup_uncharge_folios(folios);
 	free_unref_folios(folios);
 }
 EXPORT_SYMBOL(folios_put_refs);
mm/page_alloc.c +1 -0
index 12fac9084c48..fb1ecab0ee78 100644
@@ -3013,6 +3013,7 @@ void free_unref_folios(struct folio_batch *folios)
 	struct zone *locked_zone = NULL;
 	int i, j;
 
+	mem_cgroup_uncharge_folios(folios);
 	/* Prepare folios for freeing */
 	for (i = 0, j = 0; i < folios->nr; i++) {
 		struct folio *folio = folios->folios[i];
mm/vmscan.c +1 -6
index e572d2742c8c..113e2e49447b 100644
@@ -1506,7 +1506,6 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 
 		folio_unqueue_deferred_split(folio);
 		if (folio_batch_add(&free_folios, folio) == 0) {
-			mem_cgroup_uncharge_folios(&free_folios);
 			try_to_unmap_flush();
 			free_unref_folios(&free_folios);
 		}

@@ -1575,7 +1574,6 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
 
 	pgactivate = stat->nr_activate[0] + stat->nr_activate[1];
 
-	mem_cgroup_uncharge_folios(&free_folios);
 	try_to_unmap_flush();
 	free_unref_folios(&free_folios);
 

@@ -1902,7 +1900,6 @@ static unsigned int move_folios_to_lru(struct list_head *list)
 			folio_unqueue_deferred_split(folio);
 			if (folio_batch_add(&free_folios, folio) == 0) {
 				lruvec_unlock_irq(lruvec);
-				mem_cgroup_uncharge_folios(&free_folios);
 				free_unref_folios(&free_folios);
 				lruvec = NULL;
 			}

@@ -1920,10 +1917,8 @@ static unsigned int move_folios_to_lru(struct list_head *list)
 	if (lruvec)
 		lruvec_unlock_irq(lruvec);
 
-	if (free_folios.nr) {
-		mem_cgroup_uncharge_folios(&free_folios);
+	if (free_folios.nr)
 		free_unref_folios(&free_folios);
-	}
 
 	return nr_moved;
 }
Signature · 1 line
2.34.1
maintainer
#1 lore raw
Quoted 11 lines · Ridong Chen

From: Ridong Chen <[email protected]>

Every caller of free_unref_folios() invokes mem_cgroup_uncharge_folios() on the same batch immediately beforehand. This pattern is duplicated across shrink_folio_list(), move_folios_to_lru(), folio_batch_move_lru() and folios_put_refs().

Move the uncharge into free_unref_folios() itself so the batch is uncharged in one place before the folios are freed. This removes the repeated boilerplate at every call site and makes it impossible to free a batch without uncharging it first. No functional change intended.

Hm, IMO this is confusing. You're making uncharging an included service in a subset of the page allocator freeing API. Batch freeing uncharges, but single page freeing does not.

I don't think saving 7 lines is worth that API caveat.

author
#2 lore raw
Quoted 13 lines · Johannes Weiner
Quoted 11 lines · Ridong Chen

From: Ridong Chen <[email protected]>

Every caller of free_unref_folios() invokes mem_cgroup_uncharge_folios() on the same batch immediately beforehand. This pattern is duplicated across shrink_folio_list(), move_folios_to_lru(), folio_batch_move_lru() and folios_put_refs().

Move the uncharge into free_unref_folios() itself so the batch is uncharged in one place before the folios are freed. This removes the repeated boilerplate at every call site and makes it impossible to free a batch without uncharging it first. No functional change intended.

Quoted 5 lines

Hm, IMO this is confusing. You're making uncharging an included service in a subset of the page allocator freeing API. Batch freeing uncharges, but single page freeing does not.

I don't think saving 7 lines is worth that API caveat.

Thanks Johannes.

I thought this could improve cohesion, since we have to uncharge the batch when freeing unreferenced folios. However, I didn't consider the symmetry with single folio freeing.

I'll just drop the patch if it's causing confusion.

Signature · 2 lines
Best regards
Ridong