Re: [PATCH -v3 00/10] THP swap: Delay splitting THP during swapping out

["Huang\, Ying" <ying.huang@intel.com>]

Minchan Kim <minchan@kernel.org> writes:

> On Tue, Sep 13, 2016 at 04:53:49PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>> > On Tue, Sep 13, 2016 at 02:40:00PM +0800, Huang, Ying wrote:
>> >> Minchan Kim <minchan@kernel.org> writes:
>> >> 
>> >> > Hi Huang,
>> >> >
>> >> > On Fri, Sep 09, 2016 at 01:35:12PM -0700, Huang, Ying wrote:
>> >> >
>> >> > < snip >
>> >> >
>> >> >> >> Recently, the performance of the storage devices improved so fast that
>> >> >> >> we cannot saturate the disk bandwidth when do page swap out even on a
>> >> >> >> high-end server machine.  Because the performance of the storage
>> >> >> >> device improved faster than that of CPU.  And it seems that the trend
>> >> >> >> will not change in the near future.  On the other hand, the THP
>> >> >> >> becomes more and more popular because of increased memory size.  So it
>> >> >> >> becomes necessary to optimize THP swap performance.
>> >> >> >> 
>> >> >> >> The advantages of the THP swap support include:
>> >> >> >> 
>> >> >> >> - Batch the swap operations for the THP to reduce lock
>> >> >> >>   acquiring/releasing, including allocating/freeing the swap space,
>> >> >> >>   adding/deleting to/from the swap cache, and writing/reading the swap
>> >> >> >>   space, etc.  This will help improve the performance of the THP swap.
>> >> >> >> 
>> >> >> >> - The THP swap space read/write will be 2M sequential IO.  It is
>> >> >> >>   particularly helpful for the swap read, which usually are 4k random
>> >> >> >>   IO.  This will improve the performance of the THP swap too.
>> >> >> >> 
>> >> >> >> - It will help the memory fragmentation, especially when the THP is
>> >> >> >>   heavily used by the applications.  The 2M continuous pages will be
>> >> >> >>   free up after THP swapping out.
>> >> >> >
>> >> >> > I just read patchset right now and still doubt why the all changes
>> >> >> > should be coupled with THP tightly. Many parts(e.g., you introduced
>> >> >> > or modifying existing functions for making them THP specific) could
>> >> >> > just take page_list and the number of pages then would handle them
>> >> >> > without THP awareness.
>> >> >> 
>> >> >> I am glad if my change could help normal pages swapping too.  And we can
>> >> >> change these functions to work for normal pages when necessary.
>> >> >
>> >> > Sure but it would be less painful that THP awareness swapout is
>> >> > based on multiple normal pages swapout. For exmaple, we don't
>> >> > touch delay THP split part(i.e., split a THP into 512 pages like
>> >> > as-is) and enhances swapout further like Tim's suggestion
>> >> > for mulitple normal pages swapout. With that, it might be enough
>> >> > for fast-storage without needing THP awareness.
>> >> >
>> >> > My *point* is let's approach step by step.
>> >> > First of all, go with batching normal pages swapout and if it's
>> >> > not enough, dive into further optimization like introducing
>> >> > THP-aware swapout.
>> >> >
>> >> > I believe it's natural development process to evolve things
>> >> > without over-engineering.
>> >> 
>> >> My target is not only the THP swap out acceleration, but also the full
>> >> THP swap out/in support without splitting THP.  This patchset is just
>> >> the first step of the full THP swap support.
>> >> 
>> >> >> > For example, if the nr_pages is larger than SWAPFILE_CLUSTER, we
>> >> >> > can try to allocate new cluster. With that, we could allocate new
>> >> >> > clusters to meet nr_pages requested or bail out if we fail to allocate
>> >> >> > and fallback to 0-order page swapout. With that, swap layer could
>> >> >> > support multiple order-0 pages by batch.
>> >> >> >
>> >> >> > IMO, I really want to land Tim Chen's batching swapout work first.
>> >> >> > With Tim Chen's work, I expect we can make better refactoring
>> >> >> > for batching swap before adding more confuse to the swap layer.
>> >> >> > (I expect it would share several pieces of code for or would be base
>> >> >> > for batching allocation of swapcache, swapslot)
>> >> >> 
>> >> >> I don't think there is hard conflict between normal pages swapping
>> >> >> optimizing and THP swap optimizing.  Some code may be shared between
>> >> >> them.  That is good for both sides.
>> >> >> 
>> >> >> > After that, we could enhance swap for big contiguous batching
>> >> >> > like THP and finally we might make it be aware of THP specific to
>> >> >> > enhance further.
>> >> >> >
>> >> >> > A thing I remember you aruged: you want to swapin 512 pages
>> >> >> > all at once unconditionally. It's really worth to discuss if
>> >> >> > your design is going for the way.
>> >> >> > I doubt it's generally good idea. Because, currently, we try to
>> >> >> > swap in swapped out pages in THP page with conservative approach
>> >> >> > but your direction is going to opposite way.
>> >> >> >
>> >> >> > [mm, thp: convert from optimistic swapin collapsing to conservative]
>> >> >> >
>> >> >> > I think general approach(i.e., less effective than targeting
>> >> >> > implement for your own specific goal but less hacky and better job
>> >> >> > for many cases) is to rely/improve on the swap readahead.
>> >> >> > If most of subpages of a THP page are really workingset, swap readahead
>> >> >> > could work well.
>> >> >> >
>> >> >> > Yeah, it's fairly vague feedback so sorry if I miss something clear.
>> >> >> 
>> >> >> Yes.  I want to go to the direction that to swap in 512 pages together.
>> >> >> And I think it is a good opportunity to discuss that now.  The advantages
>> >> >> of swapping in 512 pages together are:
>> >> >> 
>> >> >> - Improve the performance of swapping in IO via turning small read size
>> >> >>   into 512 pages big read size.
>> >> >> 
>> >> >> - Keep THP across swap out/in.  With the memory size become more and
>> >> >>   more large, the 4k pages bring more and more burden to memory
>> >> >>   management.  One solution is to use 2M pages as much as possible, that
>> >> >>   will reduce the management burden greatly, such as much reduced length
>> >> >>   of LRU list, etc.
>> >> >> 
>> >> >> The disadvantage are:
>> >> >> 
>> >> >> - Increase the memory pressure when swap in THP.
>> >> >> 
>> >> >> - Some pages swapped in may not needed in the near future.
>> >> >> 
>> >> >> Because of the disadvantages, the 512 pages swapping in should be made
>> >> >> optional.  But I don't think we should make it impossible.
>> >> >
>> >> > Yeb. No need to make it impossible but your design shouldn't be coupled
>> >> > with non-existing feature yet.
>> >> 
>> >> Sorry, what is the "non-existing feature"?  The full THP swap out/in
>> >
>> > THP swapin.
>> >
>> > You said you increased cluster size to fit a THP size for recording
>> > some meta in there for THP swapin.
>> 
>> And to find the head of the THP to swap in the whole THP when an address
>> in the middle of a THP is accessed.
>> 
>> > You gave number about how scale bad current swapout so try to enhance
>> > that path. I agree it alghouth I don't like your approach for first step.
>> > However, you didn't give any clue why we should swap in a THP. How bad
>> > current conservative swapin from khugepagd is really bad and why cannot
>> > enhance that.
>> >
>> >> support without splitting THP?  If so, this patchset is the just the
>> >> first step of that.  I plan to finish the the full THP swap out/in
>> >> support in 3 steps:
>> >> 
>> >> 1. Delay splitting the THP after adding it into swap cache
>> >> 
>> >> 2. Delay splitting the THP after swapping out being completed
>> >> 
>> >> 3. Avoid splitting the THP during swap out, and swap in the full THP if
>> >>    possible
>> >> 
>> >> I plan to do it step by step to make it easier to review the code.
>> >
>> > 1. If we solve batching swapout, then how is THP split for swapout bad?
>> > 2. Also, how is current conservatie swapin from khugepaged bad?
>> >
>> > I think it's one of decision point for the motivation of your work
>> > and for 1, we need batching swapout feature.
>> >
>> > I am saying again that I'm not against your goal but only concern
>> > is approach. If you don't agree, please ignore me.
>> 
>> I am glad to discuss my final goal, that is, swapping out/in the full
>> THP without splitting.  Why I want to do that is copied as below,
>
> Yes, it's your *final* goal but what if it couldn't be acceptable
> on second step you mentioned above, for example?
>
>         Unncessary binded implementation to rejected work.

So I want to discuss my final goal.  If people accept my final goal,
this is resolved.  If people don't accept, I will reconsider it.

> If you want to achieve your goal step by step, please consider if
> one of step you are thinking could be rejected but steps already
> merged should be self-contained without side-effect.

What is the side-effect or possible regressions of the step 1 as in this
patchset?  Lacks the opportunity to allocate consecutive 512 swap slots
in 2 non-free swap clusters?  I don't think that is a regression,
because the patchset will NOT make free swap clusters consumed faster
than that in current code.  Even if it were better to allocate
consecutive 512 swap slots in 2 non-free swap clusters, it could be an
incremental improvement to the simple solution in this patchset.  That
is, to allocate 512 swap slots, the simple solution is:

a) Try to allocate a free swap cluster
b) If a) fails, give up

The improved solution could be (if it were needed finally)

a) Try to allocate a free swap cluster
b) If a) fails, try to allocate consecutive 512 swap slots in 2 non-free
   swap clusters
c) If b) fails, give up

> If it's hard, send full patchset all at once so reviewers can think
> what you want of right direction and implementation is good for it.

Thanks for suggestion.

[snip]

Best Regards,
Huang, Ying