Hi Barry,
We got a crash report from syzbot that has been bisect into this change.
Please see the comment below.
------------[ cut here ]------------
kernel BUG at mm/swap_cgroup.c:141 !
Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
CPU: 1 UID: 0 PID: 5371 Comm: syz.0.15 Not tainted
6.11.0-rc3-next-20240812-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 06/27/2024
RIP: 0010:swap_cgroup_record+0x2cd/0x2d0 mm/swap_cgroup.c:141
Code: e7 e8 a7 c9 f6 ff e9 64 fe ff ff e8 cd 41 8e ff 48 c7 c7 c0 db
a5 8e 48 89 de e8 2e 8c e8 02 e9 7a fd ff ff e8 b4 41 8e ff 90 <0f> 0b
90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f
RSP: 0018:ffffc90003e172f8 EFLAGS: 00010093
RAX: ffffffff82054c9c RBX: 000000000000000b RCX: ffff88802298bc00
RDX: 0000000000000000 RSI: 000000000000000a RDI: 0000000000000000
RBP: 0000000000000001 R08: ffffffff82054b43 R09: fffff520007c2e3c
R10: dffffc0000000000 R11: fffff520007c2e3c R12: ffff88801cf0f014
R13: 0000000000000000 R14: 000000000000000a R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff8880b9100000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f9332107a8c CR3: 000000000e734000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
__mem_cgroup_uncharge_swap+0x84/0x2e0 mm/memcontrol.c:5118
mem_cgroup_uncharge_swap include/linux/swap.h:668 [inline]
swap_entry_range_free+0x45f/0x1120 mm/swapfile.c:1556
__swap_entries_free mm/swapfile.c:1518 [inline]
free_swap_and_cache_nr+0xa65/0xae0 mm/swapfile.c:1876
zap_pte_range mm/memory.c:1653 [inline]
zap_pmd_range mm/memory.c:1736 [inline]
zap_pud_range mm/memory.c:1765 [inline]
zap_p4d_range mm/memory.c:1786 [inline]
unmap_page_range+0x1924/0x42c0 mm/memory.c:1807
unmap_vmas+0x3cc/0x5f0 mm/memory.c:1897
exit_mmap+0x267/0xc20 mm/mmap.c:1923
__mmput+0x115/0x390 kernel/fork.c:1347
exit_mm+0x220/0x310 kernel/exit.c:571
do_exit+0x9b2/0x28e0 kernel/exit.c:926
do_group_exit+0x207/0x2c0 kernel/exit.c:1088
__do_sys_exit_group kernel/exit.c:1099 [inline]
__se_sys_exit_group kernel/exit.c:1097 [inline]
__x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1097
x64_sys_call+0x2634/0x2640 arch/x86/include/generated/asm/syscalls_64.h:232
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
On Wed, Aug 7, 2024 at 2:59 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
>
> From: Barry Song <v-songbaohua@xxxxxxxx>
>
> Zhiguo reported that swap release could be a serious bottleneck
> during process exits[1]. With mTHP, we have the opportunity to
> batch free swaps.
> Thanks to the work of Chris and Kairui[2], I was able to achieve
> this optimization with minimal code changes by building on their
> efforts.
> If swap_count is 1, which is likely true as most anon memory are
> private, we can free all contiguous swap slots all together.
>
> Ran the below test program for measuring the bandwidth of munmap
> using zRAM and 64KiB mTHP:
>
> #include <sys/mman.h>
> #include <sys/time.h>
> #include <stdlib.h>
>
> unsigned long long tv_to_ms(struct timeval tv)
> {
> return tv.tv_sec * 1000 + tv.tv_usec / 1000;
> }
>
> main()
> {
> struct timeval tv_b, tv_e;
> int i;
> #define SIZE 1024*1024*1024
> void *p = mmap(NULL, SIZE, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> if (!p) {
> perror("fail to get memory");
> exit(-1);
> }
>
> madvise(p, SIZE, MADV_HUGEPAGE);
> memset(p, 0x11, SIZE); /* write to get mem */
>
> madvise(p, SIZE, MADV_PAGEOUT);
>
> gettimeofday(&tv_b, NULL);
> munmap(p, SIZE);
> gettimeofday(&tv_e, NULL);
>
> printf("munmap in bandwidth: %ld bytes/ms\n",
> SIZE/(tv_to_ms(tv_e) - tv_to_ms(tv_b)));
> }
>
> The result is as below (munmap bandwidth):
> mm-unstable mm-unstable-with-patch
> round1 21053761 63161283
> round2 21053761 63161283
> round3 21053761 63161283
> round4 20648881 67108864
> round5 20648881 67108864
>
> munmap bandwidth becomes 3X faster.
>
> [1] https://lore.kernel.org/linux-mm/20240731133318.527-1-justinjiang@xxxxxxxx/
> [2] https://lore.kernel.org/linux-mm/20240730-swap-allocator-v5-0-cb9c148b9297@xxxxxxxxxx/
>
> Cc: Kairui Song <kasong@xxxxxxxxxxx>
> Cc: Chris Li <chrisl@xxxxxxxxxx>
> Cc: "Huang, Ying" <ying.huang@xxxxxxxxx>
> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> Cc: Kalesh Singh <kaleshsingh@xxxxxxxxxx>
> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> ---
> mm/swapfile.c | 76 +++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 65 insertions(+), 11 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 35cb58373493..52e941b6d626 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -156,6 +156,25 @@ static bool swap_is_has_cache(struct swap_info_struct *si,
> return true;
> }
>
> +static bool swap_is_last_map(struct swap_info_struct *si,
> + unsigned long offset, int nr_pages, bool *has_cache)
> +{
> + unsigned char *map = si->swap_map + offset;
> + unsigned char *map_end = map + nr_pages;
> + unsigned char count = *map;
> +
> + if (swap_count(count) != 1)
> + return false;
> +
> + while (++map < map_end) {
> + if (*map != count)
> + return false;
> + }
> +
> + *has_cache = !!(count & SWAP_HAS_CACHE);
> + return true;
> +}
> +
> /*
> * returns number of pages in the folio that backs the swap entry. If positive,
> * the folio was reclaimed. If negative, the folio was not reclaimed. If 0, no
> @@ -1469,6 +1488,51 @@ static unsigned char __swap_entry_free(struct swap_info_struct *si,
> return usage;
> }
>
> +static bool __swap_entries_free(struct swap_info_struct *si,
> + swp_entry_t entry, int nr)
> +{
> + unsigned long offset = swp_offset(entry);
> + unsigned int type = swp_type(entry);
> + struct swap_cluster_info *ci;
> + bool has_cache = false;
> + unsigned char count;
> + int i;
> +
> + if (nr <= 1 || swap_count(data_race(si->swap_map[offset])) != 1)
> + goto fallback;
> + /* cross into another cluster */
> + if (nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER)
> + goto fallback;
> +
> + ci = lock_cluster_or_swap_info(si, offset);
> + if (!swap_is_last_map(si, offset, nr, &has_cache)) {
> + unlock_cluster_or_swap_info(si, ci);
> + goto fallback;
> + }
> + for (i = 0; i < nr; i++)
> + WRITE_ONCE(si->swap_map[offset + i], SWAP_HAS_CACHE);
> + unlock_cluster_or_swap_info(si, ci);
> +
> + if (!has_cache) {
> + spin_lock(&si->lock);
> + swap_entry_range_free(si, entry, nr);
Here it calls swap_entry_range_free() to free a range of the swap
entry. However the swap_entry_range_free() has the assumption that all
entries belong to the same folio and charge to the same memcg.
It eventually pass down to swap_cgroup_record(), which BUG on this line:
VM_BUG_ON(sc->id != old);
The root cause is that the swap entries are not from the same memcg.
Thankos Yosry for finding the root cause.
> + spin_unlock(&si->lock);
> + }
> + return has_cache;
> +
> +fallback:
> + for (i = 0; i < nr; i++) {
> + if (data_race(si->swap_map[offset + i])) {
> + count = __swap_entry_free(si, swp_entry(type, offset + i));
> + if (count == SWAP_HAS_CACHE)
> + has_cache = true;
> + } else {
> + WARN_ON_ONCE(1);
> + }
> + }
> + return has_cache;
> +}
> +
> /*
> * Drop the last HAS_CACHE flag of swap entries, caller have to
> * ensure all entries belong to the same cgroup.
> @@ -1792,11 +1856,9 @@ void free_swap_and_cache_nr(swp_entry_t entry, int nr)
> {
> const unsigned long start_offset = swp_offset(entry);
> const unsigned long end_offset = start_offset + nr;
> - unsigned int type = swp_type(entry);
> struct swap_info_struct *si;
> bool any_only_cache = false;
> unsigned long offset;
> - unsigned char count;
>
> if (non_swap_entry(entry))
> return;
> @@ -1811,15 +1873,7 @@ void free_swap_and_cache_nr(swp_entry_t entry, int nr)
> /*
> * First free all entries in the range.
> */
> - for (offset = start_offset; offset < end_offset; offset++) {
> - if (data_race(si->swap_map[offset])) {
> - count = __swap_entry_free(si, swp_entry(type, offset));
> - if (count == SWAP_HAS_CACHE)
> - any_only_cache = true;
> - } else {
> - WARN_ON_ONCE(1);
> - }
> - }
> + any_only_cache = __swap_entries_free(si, entry, nr);
Here we are just doing a page table walk, there is no guarantee the
'nr' number of swap entries came from the same folio and previously
charged to the same memcg. The swap_pte_batch() only checks they are
the same swap type, does not check they charge to the same memcg.
Chris
>
> /*
> * Short-circuit the below loop if none of the entries had their
> --
> 2.34.1
>
