Received-SPF: pass (google.com: domain of linux-kernel+bounces-214062-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <3c11000dd6c1df83015a8321a859e9775ebbc23e.1718266112.git.baolin.wang@linux.alibaba.com>
In-Reply-To: <3c11000dd6c1df83015a8321a859e9775ebbc23e.1718266112.git.baolin.wang@linux.alibaba.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 13 Jun 2024 15:24:48 -0700
Message-ID: <CAJD7tkYj_+edtqxTWH6Hmakv48Fx0GMUWwvU82L-V8SxmNg2qQ@mail.gmail.com>
Subject: Re: [PATCH] mm: shmem: fix getting incorrect lruvec when replacing a
 shmem folio
To: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: akpm@linux-foundation.org, hughd@google.com, hannes@cmpxchg.org, 
	nphamcs@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 13, 2024 at 1:21=E2=80=AFAM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> When testing shmem swapin, I encountered the warning below on my machine.
> The reason is that replacing an old shmem folio with a new one causes
> mem_cgroup_migrate() to clear the old folio's memcg data. As a result,
> the old folio cannot get the correct memcg's lruvec needed to remove itse=
lf
> from the LRU list when it is being freed. This could lead to possible ser=
ious
> problems, such as LRU list crashes due to holding the wrong LRU lock, and
> incorrect LRU statistics.
>
> To fix this issue, we can fallback to use the mem_cgroup_replace_folio()
> to replace the old shmem folio.
>
> [ 5241.100311] page: refcount:0 mapcount:0 mapping:0000000000000000 index=
:0x0 pfn:0x5d9960
> [ 5241.100317] head: order:4 mapcount:0 entire_mapcount:0 nr_pages_mapped=
:0 pincount:0
> [ 5241.100319] flags: 0x17fffe0000040068(uptodate|lru|head|swapbacked|nod=
e=3D0|zone=3D2|lastcpupid=3D0x3ffff)
> [ 5241.100323] raw: 17fffe0000040068 fffffdffd6687948 fffffdffd69ae008 00=
00000000000000
> [ 5241.100325] raw: 0000000000000000 0000000000000000 00000000ffffffff 00=
00000000000000
> [ 5241.100326] head: 17fffe0000040068 fffffdffd6687948 fffffdffd69ae008 0=
000000000000000
> [ 5241.100327] head: 0000000000000000 0000000000000000 00000000ffffffff 0=
000000000000000
> [ 5241.100328] head: 17fffe0000000204 fffffdffd6665801 ffffffffffffffff 0=
000000000000000
> [ 5241.100329] head: 0000000a00000010 0000000000000000 00000000ffffffff 0=
000000000000000
> [ 5241.100330] page dumped because: VM_WARN_ON_ONCE_FOLIO(!memcg && !mem_=
cgroup_disabled())
> [ 5241.100338] ------------[ cut here ]------------
> [ 5241.100339] WARNING: CPU: 19 PID: 78402 at include/linux/memcontrol.h:=
775 folio_lruvec_lock_irqsave+0x140/0x150
> [...]
> [ 5241.100374] pc : folio_lruvec_lock_irqsave+0x140/0x150
> [ 5241.100375] lr : folio_lruvec_lock_irqsave+0x138/0x150
> [ 5241.100376] sp : ffff80008b38b930
> [...]
> [ 5241.100398] Call trace:
> [ 5241.100399]  folio_lruvec_lock_irqsave+0x140/0x150
> [ 5241.100401]  __page_cache_release+0x90/0x300
> [ 5241.100404]  __folio_put+0x50/0x108
> [ 5241.100406]  shmem_replace_folio+0x1b4/0x240
> [ 5241.100409]  shmem_swapin_folio+0x314/0x528
> [ 5241.100411]  shmem_get_folio_gfp+0x3b4/0x930
> [ 5241.100412]  shmem_fault+0x74/0x160
> [ 5241.100414]  __do_fault+0x40/0x218
> [ 5241.100417]  do_shared_fault+0x34/0x1b0
> [ 5241.100419]  do_fault+0x40/0x168
> [ 5241.100420]  handle_pte_fault+0x80/0x228
> [ 5241.100422]  __handle_mm_fault+0x1c4/0x440
> [ 5241.100424]  handle_mm_fault+0x60/0x1f0
> [ 5241.100426]  do_page_fault+0x120/0x488
> [ 5241.100429]  do_translation_fault+0x4c/0x68
> [ 5241.100431]  do_mem_abort+0x48/0xa0
> [ 5241.100434]  el0_da+0x38/0xc0
> [ 5241.100436]  el0t_64_sync_handler+0x68/0xc0
> [ 5241.100437]  el0t_64_sync+0x14c/0x150
> [ 5241.100439] ---[ end trace 0000000000000000 ]---
>
> Fixes: 85ce2c517ade ("memcontrol: only transfer the memcg data for migrat=
ion")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/shmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 99bd3c34f0fb..4acaf02bfe44 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1908,7 +1908,7 @@ static int shmem_replace_folio(struct folio **folio=
p, gfp_t gfp,
>         xa_lock_irq(&swap_mapping->i_pages);
>         error =3D shmem_replace_entry(swap_mapping, swap_index, old, new)=
;
>         if (!error) {
> -               mem_cgroup_migrate(old, new);
> +               mem_cgroup_replace_folio(old, new);

The comment above the definition of mem_cgroup_replace_folio() needs
to be updated with this change. It mentions that it is only used in
replace_page_cache_folio(), which is no longer true.

>                 __lruvec_stat_mod_folio(new, NR_FILE_PAGES, 1);
>                 __lruvec_stat_mod_folio(new, NR_SHMEM, 1);
>                 __lruvec_stat_mod_folio(old, NR_FILE_PAGES, -1);
> --
> 2.39.3
>
>