Date: Thu, 20 Jun 2024 20:11:44 +1200
From: Barry Song <21cnbao@...il.com>
To: "Huang, Ying" <ying.huang@...el.com>
Cc: akpm@...ux-foundation.org, shuah@...nel.org, linux-mm@...ck.org, 
	ryan.roberts@....com, chrisl@...nel.org, david@...hat.com, hughd@...gle.com, 
	kaleshsingh@...gle.com, kasong@...cent.com, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, Barry Song <v-songbaohua@...o.com>
Subject: Re: [PATCH] selftests/mm: Introduce a test program to assess swap
 entry allocation for thp_swapout

On Thu, Jun 20, 2024 at 8:01 PM Huang, Ying <ying.huang@...el.com> wrote:
>
> Barry Song <21cnbao@...il.com> writes:
>
> > On Thu, Jun 20, 2024 at 6:36 PM Huang, Ying <ying.huang@...el.com> wrote:
> >>
> >> Barry Song <21cnbao@...il.com> writes:
> >>
> >> > On Thu, Jun 20, 2024 at 5:22 PM Huang, Ying <ying.huang@...el.com> wrote:
> >> >>
> >> >> Barry Song <21cnbao@...il.com> writes:
> >> >>
> >> >> > On Thu, Jun 20, 2024 at 1:55 PM Huang, Ying <ying.huang@...el.com> wrote:
> >> >> >>
> >> >> >> Barry Song <21cnbao@...il.com> writes:
> >> >> >>
> >> >> >> > From: Barry Song <v-songbaohua@...o.com>
> >> >> >> >
> >> >> >> > Both Ryan and Chris have been utilizing the small test program to aid
> >> >> >> > in debugging and identifying issues with swap entry allocation. While
> >> >> >> > a real or intricate workload might be more suitable for assessing the
> >> >> >> > correctness and effectiveness of the swap allocation policy, a small
> >> >> >> > test program presents a simpler means of understanding the problem and
> >> >> >> > initially verifying the improvements being made.
> >> >> >> >
> >> >> >> > Let's endeavor to integrate it into the self-test suite. Although it
> >> >> >> > presently only accommodates 64KB and 4KB, I'm optimistic that we can
> >> >> >> > expand its capabilities to support multiple sizes and simulate more
> >> >> >> > complex systems in the future as required.
> >> >> >>
> >> >> >> IIUC, this is a performance test program instead of functionality test
> >> >> >> program.  Does it match the purpose of the kernel selftest?
> >> >> >
> >> >> > I have a differing perspective. I maintain that the functionality is
> >> >> > not functioning
> >> >> > as expected. Despite having all the necessary resources for allocation, failure
> >> >> > persists, indicating a lack of functionality.
> >> >>
> >> >> Is there any user visual functionality issue?
> >> >
> >> > Definitely not. If a plane can't take off, taking a train and pretending
> >> > there's no functionality issue isn't a solution.
> >>
> >> I always think that performance optimization is great work.  However, it
> >> is not functionality work.
> >>
> >> > I have never assigned blame for any mistakes here. On the contrary,
> >> > I have 100% appreciation for Ryan's work in at least initiating mTHP
> >> > swapout w/o being split.
> >> >
> >> > It took countless experiments for humans to make airplanes commercially
> >> > viable, but the person who created the first flying airplane remains the
> >> > greatest. Similarly, Ryan's efforts, combined with your review of his patch,
> >> > have enabled us to achieve a better goal here. Without your work, we can't
> >> > get here at all.
> >>
> >> Thanks!
> >>
> >> > However, this is never a reason to refuse to acknowledge that this feature
> >> > is not actually working.
> >>
> >> It just works for some workloads, not for some others.
> >>
> >> >>
> >> >> >>
> >> >> >> > Signed-off-by: Barry Song <v-songbaohua@...o.com>
> >> >> >> > ---
> >> >> >> >  tools/testing/selftests/mm/Makefile           |   1 +
> >> >> >> >  .../selftests/mm/thp_swap_allocator_test.c    | 192 ++++++++++++++++++
> >> >> >> >  2 files changed, 193 insertions(+)
> >> >> >> >  create mode 100644 tools/testing/selftests/mm/thp_swap_allocator_test.c
> >> >> >> >
> >> >> >> > diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> >> >> >> > index e1aa09ddaa3d..64164ad66835 100644
> >> >> >> > --- a/tools/testing/selftests/mm/Makefile
> >> >> >> > +++ b/tools/testing/selftests/mm/Makefile
> >> >> >> > @@ -65,6 +65,7 @@ TEST_GEN_FILES += mseal_test
> >> >> >> >  TEST_GEN_FILES += seal_elf
> >> >> >> >  TEST_GEN_FILES += on-fault-limit
> >> >> >> >  TEST_GEN_FILES += pagemap_ioctl
> >> >> >> > +TEST_GEN_FILES += thp_swap_allocator_test
> >> >> >> >  TEST_GEN_FILES += thuge-gen
> >> >> >> >  TEST_GEN_FILES += transhuge-stress
> >> >> >> >  TEST_GEN_FILES += uffd-stress
> >> >> >> > diff --git a/tools/testing/selftests/mm/thp_swap_allocator_test.c b/tools/testing/selftests/mm/thp_swap_allocator_test.c
> >> >> >> > new file mode 100644
> >> >> >> > index 000000000000..4443a906d0f8
> >> >> >> > --- /dev/null
> >> >> >> > +++ b/tools/testing/selftests/mm/thp_swap_allocator_test.c
> >> >> >> > @@ -0,0 +1,192 @@
> >> >> >> > +// SPDX-License-Identifier: GPL-2.0-or-later
> >> >> >> > +/*
> >> >> >> > + * thp_swap_allocator_test
> >> >> >> > + *
> >> >> >> > + * The purpose of this test program is helping check if THP swpout
> >> >> >> > + * can correctly get swap slots to swap out as a whole instead of
> >> >> >> > + * being split. It randomly releases swap entries through madvise
> >> >> >> > + * DONTNEED and do swapout on two memory areas: a memory area for
> >> >> >> > + * 64KB THP and the other area for small folios. The second memory
> >> >> >> > + * can be enabled by "-s".
> >> >> >> > + * Before running the program, we need to setup a zRAM or similar
> >> >> >> > + * swap device by:
> >> >> >> > + *  echo lzo > /sys/block/zram0/comp_algorithm
> >> >> >> > + *  echo 64M > /sys/block/zram0/disksize
> >> >> >> > + *  echo never > /sys/kernel/mm/transparent_hugepage/hugepages-2048kB/enabled
> >> >> >> > + *  echo always > /sys/kernel/mm/transparent_hugepage/hugepages-64kB/enabled
> >> >> >> > + *  mkswap /dev/zram0
> >> >> >> > + *  swapon /dev/zram0
> >> >> >> > + * The expected result should be 0% anon swpout fallback ratio w/ or
> >> >> >> > + * w/o "-s".
> >> >> >> > + *
> >> >> >> > + * Author(s): Barry Song <v-songbaohua@...o.com>
> >> >> >> > + */
> >> >> >> > +
> >> >> >> > +#define _GNU_SOURCE
> >> >> >> > +#include <stdio.h>
> >> >> >> > +#include <stdlib.h>
> >> >> >> > +#include <unistd.h>
> >> >> >> > +#include <string.h>
> >> >> >> > +#include <sys/mman.h>
> >> >> >> > +#include <errno.h>
> >> >> >> > +#include <time.h>
> >> >> >> > +
> >> >> >> > +#define MEMSIZE_MTHP (60 * 1024 * 1024)
> >> >> >> > +#define MEMSIZE_SMALLFOLIO (1 * 1024 * 1024)
> >> >> >> > +#define ALIGNMENT_MTHP (64 * 1024)
> >> >> >> > +#define ALIGNMENT_SMALLFOLIO (4 * 1024)
> >> >> >> > +#define TOTAL_DONTNEED_MTHP (16 * 1024 * 1024)
> >> >> >> > +#define TOTAL_DONTNEED_SMALLFOLIO (768 * 1024)
> >> >> >> > +#define MTHP_FOLIO_SIZE (64 * 1024)
> >> >> >> > +
> >> >> >> > +#define SWPOUT_PATH \
> >> >> >> > +     "/sys/kernel/mm/transparent_hugepage/hugepages-64kB/stats/swpout"
> >> >> >> > +#define SWPOUT_FALLBACK_PATH \
> >> >> >> > +     "/sys/kernel/mm/transparent_hugepage/hugepages-64kB/stats/swpout_fallback"
> >> >> >> > +
> >> >> >> > +static void *aligned_alloc_mem(size_t size, size_t alignment)
> >> >> >> > +{
> >> >> >> > +     void *mem = NULL;
> >> >> >> > +
> >> >> >> > +     if (posix_memalign(&mem, alignment, size) != 0) {
> >> >> >> > +             perror("posix_memalign");
> >> >> >> > +             return NULL;
> >> >> >> > +     }
> >> >> >> > +     return mem;
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > +static void random_madvise_dontneed(void *mem, size_t mem_size,
> >> >> >> > +             size_t align_size, size_t total_dontneed_size)
> >> >> >> > +{
> >> >> >> > +     size_t num_pages = total_dontneed_size / align_size;
> >> >> >> > +     size_t i;
> >> >> >> > +     size_t offset;
> >> >> >> > +     void *addr;
> >> >> >> > +
> >> >> >> > +     for (i = 0; i < num_pages; ++i) {
> >> >> >> > +             offset = (rand() % (mem_size / align_size)) * align_size;
> >> >> >> > +             addr = (char *)mem + offset;
> >> >> >> > +             if (madvise(addr, align_size, MADV_DONTNEED) != 0)
> >> >> >> > +                     perror("madvise dontneed");
> >> >> >>
> >> >> >> IIUC, this simulates align_size (generally 64KB) swap-in.  That is, it
> >> >> >> simulate the effect of large size swap-in when it's not available in
> >> >> >> kernel.  If we have large size swap-in in kernel in the future, this
> >> >> >> becomes unnecessary.
> >> >> >>
> >> >> >> Additionally, we have not reached the consensus that we should always
> >> >> >> swap-in with swapped-out size.  So, I suspect that this test may not
> >> >> >> reflect real situation in the future.  Although it doesn't reflect
> >> >> >> current situation too.
> >> >> >
> >> >> > Disagree again. releasing the whole mTHP swaps is the best case. Even in
> >> >> > the best-case scenario, if we fail, it raises concerns for handling potentially
> >> >> > more challenging situations.
> >> >>
> >> >> Repeating sequential anonymous pages writing is the best case.
> >> >
> >> > I define the best case as the scenario with the least chance of creating
> >> > fragments within swapfiles for mTHP to swap out. There is no real
> >> > difference whether this is done through swapin or madv_dontneed.
> >>
> >> IMO, swapin is much more important than madv_dontneed.  Because most
> >> users use swapin automatically, but few use madv_dontneed by hand.  So,
> >> I think swapin/swapout test is much more important than madv_dontneed.
> >> I don't like this test case because madv_dontneed isn't typical or
> >> basic.
> >
> > Disliking DONTNEED isn't a sufficient reason to reject this test program because
> > no single small program can report swapout counters, swapout fallback counters,
> > and fallback ratios within several minutes for 100 iterations. That's
> > precisely why
> > we need it, at least initially. We can enhance it further if it lacks
> > certain functionalities
> > that people desire.
> >
> > The entire purpose of MADV_DONTNEED is to simulate a scenario where all
> > slots are released as a whole, preventing the creation of fragments, which is
> > most favorable for swap allocation. I believe there is no difference between
> > using MADV_DONTNEED or swapin for this purpose. But I am perfectly fine
> > with switching to swapin to replace MADV_DONTNEED in v2.
>
> Great!  Thanks for doing this!
>
> And even better, can we not make swap-in address aligned and size
> aligned?  It's too unrealistic.  It's good to consider some level of
> spatial locality, for example, swap-in random number of pages
> sequentially at some random addresses.  That could be a good general
> test program.  We can use it to evaluate further swap optimizations, for
> example, to evaluate the memory wastage of some swap-in size policy.

I wholeheartedly agree with everything mentioned above; these are
actually part of my plan as incremental patches. This initial commit
serves as the first step of the three I proposed in the last email.

>
> And, we don't need PAGEOUT too, just use large virtual address space in
> test programs.  We can trigger swapout in more common way.

I'm not particularly enthusiastic about this idea, as I expect the test program
to run quickly. A large virtual address space would result in long waiting times
for the test results, as it relies on vmscan. Therefore, I hope we can use real
workloads to achieve this instead.

>
> [snip]
>
> --
> Best Regards,
> Huang, Ying

Thanks
Barry

Powered by blists - more mailing lists