mm/swap: fix swap cache memcg accounting

[PATCH] mm/swap: fix swap cache memcg accounting

Posted by Alexandre Ghiti 2 weeks, 1 day ago

The swap readahead path was recently refactored and while doing this,
the order between the charging of the folio in the memcg and the addition
of the folio in the swap cache was inverted.

Since the accounting of the folio is done while adding the folio to the
swap cache and the folio is not charged in the memcg yet, the accounting
is then done at the node level, which is wrong.

Fix this by charging the folio in the memcg before adding it to the swap cache.

Fixes: 2732acda82c9 ("mm, swap: use swap cache as the swap in synchronize layer")
Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
---
 mm/swap_state.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/swap_state.c b/mm/swap_state.c
index 6d0eef7470be..48aff2c917c0 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -494,6 +494,10 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
 
 	__folio_set_locked(folio);
 	__folio_set_swapbacked(folio);
+
+	if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry))
+		goto failed;
+
 	for (;;) {
 		ret = swap_cache_add_folio(folio, entry, &shadow);
 		if (!ret)
@@ -514,11 +518,6 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
 			goto failed;
 	}
 
-	if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry)) {
-		swap_cache_del_folio(folio);
-		goto failed;
-	}
-
 	memcg1_swapin(entry, folio_nr_pages(folio));
 	if (shadow)
 		workingset_refault(folio, shadow);
-- 
2.53.0

Re: [PATCH] mm/swap: fix swap cache memcg accounting

Posted by Chris Li 1 week, 6 days ago

On Thu, Mar 19, 2026 at 10:06 PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> The swap readahead path was recently refactored and while doing this,
> the order between the charging of the folio in the memcg and the addition
> of the folio in the swap cache was inverted.
>
> Since the accounting of the folio is done while adding the folio to the
> swap cache and the folio is not charged in the memcg yet, the accounting
> is then done at the node level, which is wrong.
>
> Fix this by charging the folio in the memcg before adding it to the swap cache.
>
> Fixes: 2732acda82c9 ("mm, swap: use swap cache as the swap in synchronize layer")
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>

Acked-by: Chris Li <chrisl@kernel.org>

Chris

> ---
>  mm/swap_state.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6d0eef7470be..48aff2c917c0 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -494,6 +494,10 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>
>         __folio_set_locked(folio);
>         __folio_set_swapbacked(folio);
> +
> +       if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry))
> +               goto failed;
> +
>         for (;;) {
>                 ret = swap_cache_add_folio(folio, entry, &shadow);
>                 if (!ret)
> @@ -514,11 +518,6 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>                         goto failed;
>         }
>
> -       if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry)) {
> -               swap_cache_del_folio(folio);
> -               goto failed;
> -       }
> -
>         memcg1_swapin(entry, folio_nr_pages(folio));
>         if (shadow)
>                 workingset_refault(folio, shadow);
> --
> 2.53.0
>
>

Re: [PATCH] mm/swap: fix swap cache memcg accounting

Posted by Nhat Pham 2 weeks ago

On Thu, Mar 19, 2026 at 10:06 PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> The swap readahead path was recently refactored and while doing this,
> the order between the charging of the folio in the memcg and the addition
> of the folio in the swap cache was inverted.
>
> Since the accounting of the folio is done while adding the folio to the
> swap cache and the folio is not charged in the memcg yet, the accounting
> is then done at the node level, which is wrong.
>
> Fix this by charging the folio in the memcg before adding it to the swap cache.
>
> Fixes: 2732acda82c9 ("mm, swap: use swap cache as the swap in synchronize layer")
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> ---
>  mm/swap_state.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6d0eef7470be..48aff2c917c0 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -494,6 +494,10 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>
>         __folio_set_locked(folio);
>         __folio_set_swapbacked(folio);
> +
> +       if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry))
> +               goto failed;
> +
>         for (;;) {
>                 ret = swap_cache_add_folio(folio, entry, &shadow);
>                 if (!ret)
> @@ -514,11 +518,6 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>                         goto failed;
>         }
>

Reviewed-by: Nhat Pham <nphamcs@gmail.com>

Re: [PATCH] mm/swap: fix swap cache memcg accounting

Posted by Johannes Weiner 2 weeks ago

On Fri, Mar 20, 2026 at 06:05:59AM +0100, Alexandre Ghiti wrote:
> The swap readahead path was recently refactored and while doing this,
> the order between the charging of the folio in the memcg and the addition
> of the folio in the swap cache was inverted.
> 
> Since the accounting of the folio is done while adding the folio to the
> swap cache and the folio is not charged in the memcg yet, the accounting
> is then done at the node level, which is wrong.
> 
> Fix this by charging the folio in the memcg before adding it to the swap cache.
> 
> Fixes: 2732acda82c9 ("mm, swap: use swap cache as the swap in synchronize layer")
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] mm/swap: fix swap cache memcg accounting

Posted by Kairui Song 2 weeks, 1 day ago

On Fri, Mar 20, 2026 at 06:05:59AM +0800, Alexandre Ghiti wrote:
> The swap readahead path was recently refactored and while doing this,
> the order between the charging of the folio in the memcg and the addition
> of the folio in the swap cache was inverted.
> 
> Since the accounting of the folio is done while adding the folio to the
> swap cache and the folio is not charged in the memcg yet, the accounting
> is then done at the node level, which is wrong.
> 
> Fix this by charging the folio in the memcg before adding it to the swap cache.
> 
> Fixes: 2732acda82c9 ("mm, swap: use swap cache as the swap in synchronize layer")
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> ---
>  mm/swap_state.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6d0eef7470be..48aff2c917c0 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -494,6 +494,10 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>  
>  	__folio_set_locked(folio);
>  	__folio_set_swapbacked(folio);
> +
> +	if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry))
> +		goto failed;
> +

Thanks! I was worrying charging it first could cause thrashing, raced
swapin will cause multiple charge and failed one will uncharge, so
I put the charge after insert into cache, completely forgot about
the lruvec static info part.

For a quick fix this this is clean and good enough, and it
doesn't effect the most performance sensitive part, mTHP and
ZRAM would have `charged == true` here so they are not effected.

Later we can move the lruvec static info out of
__swap_cache_add_folio after the charge, maybe I'll do this with
someother follow up optimzation together, then everything should
be perfect.

And I think we need Cc stable here.

Acked-by: Kairui Song <kasong@tencent.com>