On Thu, Sep 08, 2016 at 11:15:52AM +0530, Anshuman Khandual wrote:
> On 09/07/2016 10:16 PM, Huang, Ying wrote:
> > From: Huang Ying <ying.hu...@intel.com>
> > 
> > In this patch, the size of the swap cluster is changed to that of the
> > THP (Transparent Huge Page) on x86_64 architecture (512).  This is for
> > the THP swap support on x86_64.  Where one swap cluster will be used to
> > hold the contents of each THP swapped out.  And some information of the
> > swapped out THP (such as compound map count) will be recorded in the
> > swap_cluster_info data structure.
> > 
> > For other architectures which want THP swap support, THP_SWAP_CLUSTER
> > need to be selected in the Kconfig file for the architecture.
> > 
> > In effect, this will enlarge swap cluster size by 2 times on x86_64.
> > Which may make it harder to find a free cluster when the swap space
> > becomes fragmented.  So that, this may reduce the continuous swap space
> > allocation and sequential write in theory.  The performance test in 0day
> > shows no regressions caused by this.
> This patch needs to be split into two separate ones
> (1) Add THP_SWAP_CLUSTER config option
> (2) Enable CONFIG_THP_SWAP_CLUSTER for X86_64

No, don't do that. This is a bit of an anti-pattern in this series,
where it introduces a thing in one patch, and a user for it in a later
patch. However, in order to judge whether that thing is good or not, I
need to know how exactly it's being used.

So, please, split your series into logical steps, not geographical
ones. When you introduce a function, config option, symbol, add it
along with the code that actually *uses* it, in the same patch.

It goes for this patch, but also stuff like the memcg accounting
functions, get_huge_swap_page() etc.

Start with the logical change, then try to isolate independent changes
that could make sense even without the rest of the series. If that
results in a large patch, then so be it. If a big change is hard to
review, then making me switch back and forth between emails will make
it harder, not easier, to make make sense of it.


Reply via email to