Message-ID: <CAGsJ_4xfwB6kWgfthikMaqj1zQS1cB=MNKBNxHhMfHt_-cdF8Q@mail.gmail.com>
Date: Thu, 21 Mar 2024 18:12:29 +1300
From: Barry Song <21cnbao@...il.com>
To: "Huang, Ying" <ying.huang@...el.com>
Cc: Ryan Roberts <ryan.roberts@....com>, Matthew Wilcox <willy@...radead.org>, akpm@...ux-foundation.org, 
	linux-mm@...ck.org, chengming.zhou@...ux.dev, chrisl@...nel.org, 
	david@...hat.com, hannes@...xchg.org, kasong@...cent.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	mhocko@...e.com, nphamcs@...il.com, shy828301@...il.com, steven.price@....com, 
	surenb@...gle.com, wangkefeng.wang@...wei.com, xiang@...nel.org, 
	yosryahmed@...gle.com, yuzhao@...gle.com, Chuanhua Han <hanchuanhua@...o.com>, 
	Barry Song <v-songbaohua@...o.com>
Subject: Re: [RFC PATCH v3 5/5] mm: support large folios swapin as a whole

On Thu, Mar 21, 2024 at 5:25 PM Huang, Ying <ying.huang@...el.com> wrote:
>
> Barry Song <21cnbao@...il.com> writes:
>
> > On Wed, Mar 20, 2024 at 7:22 PM Huang, Ying <ying.huang@...el.com> wrote:
> >>
> >> Barry Song <21cnbao@...il.com> writes:
> >>
> >> > On Wed, Mar 20, 2024 at 3:20 PM Huang, Ying <ying.huang@...el.com> wrote:
> >> >>
> >> >> Ryan Roberts <ryan.roberts@....com> writes:
> >> >>
> >> >> > On 19/03/2024 09:20, Huang, Ying wrote:
> >> >> >> Ryan Roberts <ryan.roberts@....com> writes:
> >> >> >>
> >> >> >>>>>> I agree phones are not the only platform. But Rome wasn't built in a
> >> >> >>>>>> day. I can only get
> >> >> >>>>>> started on a hardware which I can easily reach and have enough hardware/test
> >> >> >>>>>> resources on it. So we may take the first step which can be applied on
> >> >> >>>>>> a real product
> >> >> >>>>>> and improve its performance, and step by step, we broaden it and make it
> >> >> >>>>>> widely useful to various areas  in which I can't reach :-)
> >> >> >>>>>
> >> >> >>>>> We must guarantee the normal swap path runs correctly and has no
> >> >> >>>>> performance regression when developing SWP_SYNCHRONOUS_IO optimization.
> >> >> >>>>> So we have to put some effort on the normal path test anyway.
> >> >> >>>>>
> >> >> >>>>>> so probably we can have a sysfs "enable" entry with default "n" or
> >> >> >>>>>> have a maximum
> >> >> >>>>>> swap-in order as Ryan's suggestion [1] at the beginning,
> >> >> >>>>>>
> >> >> >>>>>> "
> >> >> >>>>>> So in the common case, swap-in will pull in the same size of folio as was
> >> >> >>>>>> swapped-out. Is that definitely the right policy for all folio sizes? Certainly
> >> >> >>>>>> it makes sense for "small" large folios (e.g. up to 64K IMHO). But I'm not sure
> >> >> >>>>>> it makes sense for 2M THP; As the size increases the chances of actually needing
> >> >> >>>>>> all of the folio reduces so chances are we are wasting IO. There are similar
> >> >> >>>>>> arguments for CoW, where we currently copy 1 page per fault - it probably makes
> >> >> >>>>>> sense to copy the whole folio up to a certain size.
> >> >> >>>>>> "
> >> >> >>>
> >> >> >>> I thought about this a bit more. No clear conclusions, but hoped this might help
> >> >> >>> the discussion around policy:
> >> >> >>>
> >> >> >>> The decision about the size of the THP is made at first fault, with some help
> >> >> >>> from user space and in future we might make decisions to split based on
> >> >> >>> munmap/mremap/etc hints. In an ideal world, the fact that we have had to swap
> >> >> >>> the THP out at some point in its lifetime should not impact on its size. It's
> >> >> >>> just being moved around in the system and the reason for our original decision
> >> >> >>> should still hold.
> >> >> >>>
> >> >> >>> So from that PoV, it would be good to swap-in to the same size that was
> >> >> >>> swapped-out.
> >> >> >>
> >> >> >> Sorry, I don't agree with this.  It's better to swap-in and swap-out in
> >> >> >> smallest size if the page is only accessed seldom to avoid to waste
> >> >> >> memory.
> >> >> >
> >> >> > If we want to optimize only for memory consumption, I'm sure there are many
> >> >> > things we would do differently. We need to find a balance between memory and
> >> >> > performance. The benefits of folios are well documented and the kernel is
> >> >> > heading in the direction of managing memory in variable-sized blocks. So I don't
> >> >> > think it's as simple as saying we should always swap-in the smallest possible
> >> >> > amount of memory.
> >> >>
> >> >> It's conditional, that is,
> >> >>
> >> >> "if the page is only accessed seldom"
> >> >>
> >> >> Then, the page swapped-in will be swapped-out soon and adjacent pages in
> >> >> the same large folio will not be accessed during this period.
> >> >>
> >> >> So, I suggest to create an algorithm to decide swap-in order based on
> >> >> swap-readahead information automatically.  It can detect the situation
> >> >> above via reduced swap readahead window size.  And, if the page is
> >> >> accessed for quite long time, and the adjacent pages in the same large
> >> >> folio are accessed too, swap-readahead window will increase and large
> >> >> swap-in order will be used.
> >> >
> >> > The original size of do_anonymous_page() should be honored, considering it
> >> > embodies a decision influenced by not only sysfs settings and per-vma
> >> > HUGEPAGE hints but also architectural characteristics, for example
> >> > CONT-PTE.
> >> >
> >> > The model you're proposing may offer memory-saving benefits or reduce I/O,
> >> > but it entirely disassociates the size of the swap in from the size prior to the
> >> > swap out.
> >>
> >> Readahead isn't the only factor to determine folio order.  For example,
> >> we must respect "never" policy to allocate order-0 folio always.
> >> There's no requirements to use swap-out order in swap-in too.  Memory
> >> allocation has different performance character of storage reading.
> >
> > Still quite unclear.
> >
> > If users have only enabled 64KiB (4-ORDER) large folios in sysfs, and the
> > readahead algorithm requires 16KiB, what should be set as the large folio size?
> > Setting it to 16KiB doesn't align with users' requirements, while
> > setting it to 64KiB
> > would be wasteful according to your criteria.
>
> IIUC, enabling 64KB means you can use 64KB mTHP if appropriate, doesn't
> mean that you must use 64KB mTHP.  If so, we should use 16KB mTHP in
> that situation.

A specific large folio size inherently denotes a high-quality
resource. For example,
a 64KiB folio necessitates only one TLB on ARM64, just as a 2MB large folio
accommodates only one TLB. I am skeptical whether a size determined by
readahead offers any tangible advantages over simply having small folios.

>
> >> > Moreover, there's no guarantee that the large folio generated by
> >> > the readahead window is contiguous in the swap and can be added to the
> >> > swap cache, as we are currently dealing with folio->swap instead of
> >> > subpage->swap.
> >>
> >> Yes.  We can optimize only when all conditions are satisfied.  Just like
> >> other optimization.
> >>
> >> > Incidentally, do_anonymous_page() serves as the initial location for allocating
> >> > large folios. Given that memory conservation is a significant consideration in
> >> > do_swap_page(), wouldn't it be even more crucial in do_anonymous_page()?
> >>
> >> Yes.  We should consider that too.  IIUC, that is why mTHP support is
> >> off by default for now.  After we find a way to solve the memory usage
> >> issue.  We may make default "on".
> >
> > It's challenging to establish a universal solution because various systems
> > exhibit diverse hardware characteristics, and VMAs may require different
> > alignments. The current sysfs and per-vma hints allow users the opportunity
> > o customize settings according to their specific requirements.
>
> IIUC, Linux kernel is trying to provide a reasonable default behavior in
> all situations.  We are trying to optimize default behavior in the first
> place, only introduce customization if we fail to do that.  I don't
> think that it's a good idea to introduce too much customization if we
> haven't tried to optimize the default behavior.

I've never been opposed to the readahead case, but I feel it's a second step.

My point is to begin with the simplest and most practical approaches
that can generate
genuine value and contribution. The SWP_SYNCHRONOUS_IO case has been
implemented on millions of OPPO's phones and has demonstrated product success.

>
> >>
> >> > A large folio, by its nature, represents a high-quality resource that has the
> >> > potential to leverage hardware characteristics for the benefit of the
> >> > entire system.
> >>
> >> But not at the cost of memory wastage.
> >>
> >> > Conversely, I don't believe that a randomly determined size dictated by the
> >> > readahead window possesses the same advantageous qualities.
> >>
> >> There's a readahead algorithm which is not pure random.
> >>
> >> > SWP_SYNCHRONOUS_IO devices are not reliant on readahead whatsoever,
> >> > their needs should also be respected.
> >>
> >> I understand that there are special requirements for SWP_SYNCHRONOUS_IO
> >> devices.  I just suggest to work on general code before specific
> >> optimization.
> >
> > I disagree with your definition of "special" and "general". According
> > to your logic,
> > non-SWP_SYNCHRONOUS_IO devices could also be classified as "special".
>
> SWP_SYNCHRONOUS_IO devices also use general code path.  They just use
> some special optimization in some special situation (__swap_count(entry)
> == 1).  Optimization in general path benefits everyone.
>
> > Furthermore, the number of systems running SWP_SYNCHRONOUS_IO is
> > significantly greater than those running non-SWP_SYNCHRONOUS_IO,
> > contradicting your assertion.
> >
> > SWP_SYNCHRONOUS_IO devices have a minor chance of being involved
> > in readahead.
>
> Then it loses an opportunity to determine the appropriate folio order.
> We can consider how to balance between the overhead and benefit of
> readahead.  IIUC, compared with original SWP_SYNCHRONOUS_IO swap-in,
> mTHP is a kind of readahead too.
>
> BTW, because we have added more and more swap cache related operations
> (swapcache_prepare(), clear_shadow_from_swap_cache(), swapcache_clear(),
> etc.) in SWP_SYNCHRONOUS_IO code path, I suspect whether the benefit of
> SWP_SYNCHRONOUS_IO is still large enough.  We may need to re-evaluate
> it.

Obviously SWP_SYNCHRONOUS_IO is still quite valuable as Kairui has the data
in his commit  13ddaf26be324a ("mm/swap: fix race when skipping swapcache")

"Performance overhead is minimal, microbenchmark swapin 10G from 32G zram:
Before: 10934698 us
After: 11157121 us
Cached: 13155355 us (Dropping SWP_SYNCHRONOUS_IO flag) "

>
> > However, in OPPO's code, which hasn't been sent in the
> > LKML yet, we use the exact same size as do_anonymous_page for readahead.
> > Without a clear description of how you want the new readahead
> > algorithm to balance memory waste and users' hints from sysfs and
> > per-vma flags, it appears to be an ambiguous area to address.
> >
> > Please provide a clear description of how you would like the new readahead
> > algorithm to function. I believe this clarity will facilitate others
> > in attempting to
> > implement it.
>
> For example, if __swapin_nr_pages() > 4, we can try to allocate an
> order-2 mTHP if other conditions are satisfied.

There is no evidence suggesting that an order-2 or any other orders
determined by readahead are superior to having four small folios.

>
> >>
> >> >> > You also said we should swap *out* in smallest size possible. Have I
> >> >> > misunderstood you? I thought the case for swapping-out a whole folio without
> >> >> > splitting was well established and non-controversial?
> >> >>
> >> >> That is conditional too.
> >> >>
> >> >> >>
> >> >> >>> But we only kind-of keep that information around, via the swap
> >> >> >>> entry contiguity and alignment. With that scheme it is possible that multiple
> >> >> >>> virtually adjacent but not physically contiguous folios get swapped-out to
> >> >> >>> adjacent swap slot ranges and then they would be swapped-in to a single, larger
> >> >> >>> folio. This is not ideal, and I think it would be valuable to try to maintain
> >> >> >>> the original folio size information with the swap slot. One way to do this would
> >> >> >>> be to store the original order for which the cluster was allocated in the
> >> >> >>> cluster. Then we at least know that a given swap slot is either for a folio of
> >> >> >>> that order or an order-0 folio (due to cluster exhaustion/scanning). Can we
> >> >> >>> steal a bit from swap_map to determine which case it is? Or are there better
> >> >> >>> approaches?
> >> >> >>
> >> >> >> [snip]
> >>
>
> --
> Best Regards,
> Huang, Ying

Thanks
Barry

Powered by blists - more mailing lists