[PATCH v2 0/7] swap: THP optimizing refactoring

[PATCH v2 0/7] swap: THP optimizing refactoring

[Huang, Ying]

This patchset is based on 2018-07-13 head of mmotm tree.

Now the THP (Transparent Huge Page) swap optimizing is implemented in
the way like below,

#ifdef CONFIG_THP_SWAP
huge_function(...)
{
}
#else
normal_function(...)
{
}
#endif

general_function(...)
{
	if (huge)
		return thp_function(...);
	else
		return normal_function(...);
}

As pointed out by Dave Hansen, this will,

1. Created a new, wholly untested code path for huge page
2. Created two places to patch bugs
3. Are not reusing code when possible

This patchset is to address these problems via merging huge/normal
code path/functions if possible.

One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE.  The data shows that most refactoring
will only cause quite slight code size increase.

Best Regards,
Huang, Ying

[PATCH v2 1/7] swap: Add comments to lock_cluster_or_swap_info()

[Huang, Ying]

To improve the code readability.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index d8fddfb000ec..0a2a9643dd78 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -297,6 +297,12 @@ static inline void unlock_cluster(struct swap_cluster_info *ci)
 		spin_unlock(&ci->lock);
 }
 
+/*
+ * For non-HDD swap devices, the fine grained cluster lock is used to
+ * protect si->swap_map.  But cluster and cluster locks isn't
+ * available for HDD, so coarse grained si->lock will be used instead
+ * for that.
+ */
 static inline struct swap_cluster_info *lock_cluster_or_swap_info(
 	struct swap_info_struct *si,
 	unsigned long offset)
-- 
2.16.4

Re: [PATCH v2 1/7] swap: Add comments to lock_cluster_or_swap_info()

[Dave Hansen]

On 07/16/2018 05:55 PM, Huang, Ying wrote:
> +/*
> + * For non-HDD swap devices, the fine grained cluster lock is used to
> + * protect si->swap_map.  But cluster and cluster locks isn't
> + * available for HDD, so coarse grained si->lock will be used instead
> + * for that.
> + */
>  static inline struct swap_cluster_info *lock_cluster_or_swap_info(
>  	struct swap_info_struct *si,
>  	unsigned long offset)

This nomenclature is not consistent with the rest of the file.  We call
a "non-HDD" device an "ssd" absolutely everywhere else in the file.  Why
are you calling it a non-HDD here?  (fwiw, HDD _barely_ hits my acronym
cache anyway).

How about this?

/*
 * Determine the locking method in use for this device.  Return
 * swap_cluster_info if SSD-style cluster-based locking is in place.
 */
static inline struct swap_cluster_info *lock_cluster_or_swap_info(
        struct swap_info_struct *si,
        unsigned long offset)
{
        struct swap_cluster_info *ci;

	/* Try to use fine-grained SSD-style locking if available: */
        ci = lock_cluster(si, offset);

	/* Otherwise, fall back to traditional, coarse locking: */
        if (!ci)
                spin_lock(&si->lock);

        return ci;
}

Which reminds me?  Why do we even bother having two locking models?

Re: [PATCH v2 1/7] swap: Add comments to lock_cluster_or_swap_info()

[Huang, Ying]

Dave Hansen <dave.hansen@linux.intel.com> writes:

> On 07/16/2018 05:55 PM, Huang, Ying wrote:
>> +/*
>> + * For non-HDD swap devices, the fine grained cluster lock is used to
>> + * protect si->swap_map.  But cluster and cluster locks isn't
>> + * available for HDD, so coarse grained si->lock will be used instead
>> + * for that.
>> + */
>>  static inline struct swap_cluster_info *lock_cluster_or_swap_info(
>>  	struct swap_info_struct *si,
>>  	unsigned long offset)
>
> This nomenclature is not consistent with the rest of the file.  We call
> a "non-HDD" device an "ssd" absolutely everywhere else in the file.  Why
> are you calling it a non-HDD here?  (fwiw, HDD _barely_ hits my acronym
> cache anyway).
>
> How about this?
>
> /*
>  * Determine the locking method in use for this device.  Return
>  * swap_cluster_info if SSD-style cluster-based locking is in place.
>  */
> static inline struct swap_cluster_info *lock_cluster_or_swap_info(
>         struct swap_info_struct *si,
>         unsigned long offset)
> {
>         struct swap_cluster_info *ci;
>
> 	/* Try to use fine-grained SSD-style locking if available: */
>         ci = lock_cluster(si, offset);
>
> 	/* Otherwise, fall back to traditional, coarse locking: */
>         if (!ci)
>                 spin_lock(&si->lock);
>
>         return ci;
> }

This is better than my one, will use this.  Thanks!

> Which reminds me?  Why do we even bother having two locking models?

Because si->cluster_info is NULL for non-SSD, so we cannot use cluster
lock.

About why not use struct swap_cluster_info for non-SSD?  Per my
understanding, struct swap_cluster_info is optimized for SSD.
Especially it assumes seeking is cheap.  So different free swap slot
scanning policy is used for SSD and non-SSD.

Best Regards,
Huang, Ying

[PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()

[Huang, Ying]

In mm/swapfile.c, THP (Transparent Huge Page) swap specific code is
enclosed by #ifdef CONFIG_THP_SWAP/#endif to avoid code dilating when
THP isn't enabled.  But #ifdef/#endif in .c file hurt the code
readability, so Dave suggested to use IS_ENABLED(CONFIG_THP_SWAP)
instead and let compiler to do the dirty job for us.  This has
potential to remove some duplicated code too.  From output of `size`,

		text	   data	    bss	    dec	    hex	filename
THP=y:         26269	   2076	    340	  28685	   700d	mm/swapfile.o
ifdef/endif:   24115	   2028	    340	  26483	   6773	mm/swapfile.o
IS_ENABLED:    24179	   2028	    340	  26547	   67b3	mm/swapfile.o

IS_ENABLED() based solution works quite well, almost as good as that
of #ifdef/#endif.  And from the diffstat, the removed lines are more
than added lines.

One #ifdef for split_swap_cluster() is kept.  Because it is a public
function with a stub implementation for CONFIG_THP_SWAP=n in swap.h.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 56 ++++++++++++++++----------------------------------------
 1 file changed, 16 insertions(+), 40 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 0a2a9643dd78..dd9263411f11 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -870,7 +870,6 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
 	return n_ret;
 }
 
-#ifdef CONFIG_THP_SWAP
 static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot)
 {
 	unsigned long idx;
@@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot)
 	unsigned long offset, i;
 	unsigned char *map;
 
+	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
+		VM_WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	if (cluster_list_empty(&si->free_clusters))
 		return 0;
 
@@ -908,13 +912,6 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx)
 	unlock_cluster(ci);
 	swap_range_free(si, offset, SWAPFILE_CLUSTER);
 }
-#else
-static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot)
-{
-	VM_WARN_ON_ONCE(1);
-	return 0;
-}
-#endif /* CONFIG_THP_SWAP */
 
 static unsigned long scan_swap_map(struct swap_info_struct *si,
 				   unsigned char usage)
@@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry)
 	}
 }
 
-#ifdef CONFIG_THP_SWAP
 static void swapcache_free_cluster(swp_entry_t entry)
 {
 	unsigned long offset = swp_offset(entry);
@@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry)
 	unsigned int i, free_entries = 0;
 	unsigned char val;
 
+	if (!IS_ENABLED(CONFIG_THP_SWAP))
+		return;
+
 	si = _swap_info_get(entry);
 	if (!si)
 		return;
@@ -1306,6 +1305,7 @@ static void swapcache_free_cluster(swp_entry_t entry)
 	}
 }
 
+#ifdef CONFIG_THP_SWAP
 int split_swap_cluster(swp_entry_t entry)
 {
 	struct swap_info_struct *si;
@@ -1320,11 +1320,7 @@ int split_swap_cluster(swp_entry_t entry)
 	unlock_cluster(ci);
 	return 0;
 }
-#else
-static inline void swapcache_free_cluster(swp_entry_t entry)
-{
-}
-#endif /* CONFIG_THP_SWAP */
+#endif
 
 void put_swap_page(struct page *page, swp_entry_t entry)
 {
@@ -1483,7 +1479,6 @@ int swp_swapcount(swp_entry_t entry)
 	return count;
 }
 
-#ifdef CONFIG_THP_SWAP
 static bool swap_page_trans_huge_swapped(struct swap_info_struct *si,
 					 swp_entry_t entry)
 {
@@ -1494,6 +1489,9 @@ static bool swap_page_trans_huge_swapped(struct swap_info_struct *si,
 	int i;
 	bool ret = false;
 
+	if (!IS_ENABLED(CONFIG_THP_SWAP))
+		return swap_swapcount(si, entry) != 0;
+
 	ci = lock_cluster_or_swap_info(si, offset);
 	if (!ci || !cluster_is_huge(ci)) {
 		if (map[roffset] != SWAP_HAS_CACHE)
@@ -1516,7 +1514,7 @@ static bool page_swapped(struct page *page)
 	swp_entry_t entry;
 	struct swap_info_struct *si;
 
-	if (likely(!PageTransCompound(page)))
+	if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!PageTransCompound(page)))
 		return page_swapcount(page) != 0;
 
 	page = compound_head(page);
@@ -1540,10 +1538,8 @@ static int page_trans_huge_map_swapcount(struct page *page, int *total_mapcount,
 	/* hugetlbfs shouldn't call it */
 	VM_BUG_ON_PAGE(PageHuge(page), page);
 
-	if (likely(!PageTransCompound(page))) {
-		mapcount = atomic_read(&page->_mapcount) + 1;
-		if (total_mapcount)
-			*total_mapcount = mapcount;
+	if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!PageTransCompound(page))) {
+		mapcount = page_trans_huge_mapcount(page, total_mapcount);
 		if (PageSwapCache(page))
 			swapcount = page_swapcount(page);
 		if (total_swapcount)
@@ -1590,26 +1586,6 @@ static int page_trans_huge_map_swapcount(struct page *page, int *total_mapcount,
 
 	return map_swapcount;
 }
-#else
-#define swap_page_trans_huge_swapped(si, entry)	swap_swapcount(si, entry)
-#define page_swapped(page)			(page_swapcount(page) != 0)
-
-static int page_trans_huge_map_swapcount(struct page *page, int *total_mapcount,
-					 int *total_swapcount)
-{
-	int mapcount, swapcount = 0;
-
-	/* hugetlbfs shouldn't call it */
-	VM_BUG_ON_PAGE(PageHuge(page), page);
-
-	mapcount = page_trans_huge_mapcount(page, total_mapcount);
-	if (PageSwapCache(page))
-		swapcount = page_swapcount(page);
-	if (total_swapcount)
-		*total_swapcount = swapcount;
-	return mapcount + swapcount;
-}
-#endif
 
 /*
  * We can write to an anon page without COW if there are no other references
-- 
2.16.4

Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()

[Dave Hansen]

> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot)
>  	unsigned long offset, i;
>  	unsigned char *map;
>  
> +	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
> +		VM_WARN_ON_ONCE(1);
> +		return 0;
> +	}

I see you seized the opportunity to keep this code gloriously
unencumbered by pesky comments.  This seems like a time when you might
have slipped up and been temped to add a comment or two.  Guess not. :)

Seriously, though, does it hurt us to add a comment or two to say
something like:

	/*
	 * Should not even be attempting cluster allocations when
	 * huge page swap is disabled.  Warn and fail the allocation.
	 */
	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
		VM_WARN_ON_ONCE(1);
		return 0;
	}

Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()

[Huang, Ying]

Dave Hansen <dave.hansen@linux.intel.com> writes:

>> @@ -878,6 +877,11 @@ static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot)
>>  	unsigned long offset, i;
>>  	unsigned char *map;
>>  
>> +	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
>> +		VM_WARN_ON_ONCE(1);
>> +		return 0;
>> +	}
>
> I see you seized the opportunity to keep this code gloriously
> unencumbered by pesky comments.  This seems like a time when you might
> have slipped up and been temped to add a comment or two.  Guess not. :)
>
> Seriously, though, does it hurt us to add a comment or two to say
> something like:
>
> 	/*
> 	 * Should not even be attempting cluster allocations when
> 	 * huge page swap is disabled.  Warn and fail the allocation.
> 	 */
> 	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
> 		VM_WARN_ON_ONCE(1);
> 		return 0;
> 	}

I totally agree with you that we should add more comments for THP swap
to improve the code readability.  As for this specific case,
VM_WARN_ON_ONCE() here is just to capture some programming error during
development.  Do we really need comments here?

I will try to add more comments for other places in code regardless this
one.

Best Regards,
Huang, Ying

Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()

[Dave Hansen]

On 07/17/2018 08:25 PM, Huang, Ying wrote:
>> Seriously, though, does it hurt us to add a comment or two to say
>> something like:
>>
>> 	/*
>> 	 * Should not even be attempting cluster allocations when
>> 	 * huge page swap is disabled.  Warn and fail the allocation.
>> 	 */
>> 	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
>> 		VM_WARN_ON_ONCE(1);
>> 		return 0;
>> 	}
> I totally agree with you that we should add more comments for THP swap
> to improve the code readability.  As for this specific case,
> VM_WARN_ON_ONCE() here is just to capture some programming error during
> development.  Do we really need comments here?

If it's code in mainline, we need to know what it is doing.

If it's not useful to have in mainline, then let's remove it.

Re: [PATCH v2 2/7] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()

[Huang, Ying]

Dave Hansen <dave.hansen@linux.intel.com> writes:

> On 07/17/2018 08:25 PM, Huang, Ying wrote:
>>> Seriously, though, does it hurt us to add a comment or two to say
>>> something like:
>>>
>>> 	/*
>>> 	 * Should not even be attempting cluster allocations when
>>> 	 * huge page swap is disabled.  Warn and fail the allocation.
>>> 	 */
>>> 	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
>>> 		VM_WARN_ON_ONCE(1);
>>> 		return 0;
>>> 	}
>> I totally agree with you that we should add more comments for THP swap
>> to improve the code readability.  As for this specific case,
>> VM_WARN_ON_ONCE() here is just to capture some programming error during
>> development.  Do we really need comments here?
>
> If it's code in mainline, we need to know what it is doing.
>
> If it's not useful to have in mainline, then let's remove it.

OK.  Will add the comments.

Best Regards,
Huang, Ying

[PATCH v2 3/7] swap: Use swap_count() in swap_page_trans_huge_swapped()

[Huang, Ying]

In swap_page_trans_huge_swapped(), to identify whether there's any
page table mapping for a 4k sized swap entry, "si->swap_map[i] !=
SWAP_HAS_CACHE" is used.  This works correctly now, because all users
of the function will only call it after checking SWAP_HAS_CACHE.  But
as pointed out by Daniel, it is better to use "swap_count(map[i])"
here, because it works for "map[i] == 0" case too.

And this makes the implementation more consistent between normal and
huge swap entry.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index dd9263411f11..92c24402706c 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1494,12 +1494,12 @@ static bool swap_page_trans_huge_swapped(struct swap_info_struct *si,
 
 	ci = lock_cluster_or_swap_info(si, offset);
 	if (!ci || !cluster_is_huge(ci)) {
-		if (map[roffset] != SWAP_HAS_CACHE)
+		if (swap_count(map[roffset]))
 			ret = true;
 		goto unlock_out;
 	}
 	for (i = 0; i < SWAPFILE_CLUSTER; i++) {
-		if (map[offset + i] != SWAP_HAS_CACHE) {
+		if (swap_count(map[offset + i])) {
 			ret = true;
 			break;
 		}
-- 
2.16.4

[PATCH v2 4/7] swap: Unify normal/huge code path in swap_page_trans_huge_swapped()

[Huang, Ying]

As suggested by Dave, we should unify the code path for normal and
huge swap support if possible to avoid duplicated code, bugs, etc. and
make it easier to review code.

In this patch, the normal/huge code path in swap_page_trans_huge_swapped()
is unified, the added and removed lines are same.  And the binary size
is kept almost same when CONFIG_TRANSPARENT_HUGEPAGE=n.

		 text	   data	    bss	    dec	    hex	filename
base:		24179	   2028	    340	  26547	   67b3	mm/swapfile.o
unified:	24215	   2028	    340	  26583	   67d7	mm/swapfile.o

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 92c24402706c..a6d8b8117bc5 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -270,7 +270,10 @@ static inline void cluster_set_null(struct swap_cluster_info *info)
 
 static inline bool cluster_is_huge(struct swap_cluster_info *info)
 {
-	return info->flags & CLUSTER_FLAG_HUGE;
+	if (IS_ENABLED(CONFIG_THP_SWAP))
+		return info->flags & CLUSTER_FLAG_HUGE;
+	else
+		return false;
 }
 
 static inline void cluster_clear_huge(struct swap_cluster_info *info)
@@ -1489,9 +1492,6 @@ static bool swap_page_trans_huge_swapped(struct swap_info_struct *si,
 	int i;
 	bool ret = false;
 
-	if (!IS_ENABLED(CONFIG_THP_SWAP))
-		return swap_swapcount(si, entry) != 0;
-
 	ci = lock_cluster_or_swap_info(si, offset);
 	if (!ci || !cluster_is_huge(ci)) {
 		if (swap_count(map[roffset]))
-- 
2.16.4

[PATCH v2 5/7] swap: Unify normal/huge code path in put_swap_page()

[Huang, Ying]

In this patch, the normal/huge code path in put_swap_page() and
several helper functions are unified to avoid duplicated code, bugs,
etc. and make it easier to review the code.

The removed lines are more than added lines.  And the binary size is
kept exactly same when CONFIG_TRANSPARENT_HUGEPAGE=n.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 83 ++++++++++++++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 46 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index a6d8b8117bc5..622edc47b67a 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -205,8 +205,16 @@ static void discard_swap_cluster(struct swap_info_struct *si,
 
 #ifdef CONFIG_THP_SWAP
 #define SWAPFILE_CLUSTER	HPAGE_PMD_NR
+
+#define nr_swap_entries(nr)	(nr)
 #else
 #define SWAPFILE_CLUSTER	256
+
+/*
+ * Define nr_swap_entries() as constant to let compiler to optimize
+ * out some code if !CONFIG_THP_SWAP
+ */
+#define nr_swap_entries(nr)	1
 #endif
 #define LATENCY_LIMIT		256
 
@@ -1249,18 +1257,7 @@ void swap_free(swp_entry_t entry)
 /*
  * Called after dropping swapcache to decrease refcnt to swap entries.
  */
-static void swapcache_free(swp_entry_t entry)
-{
-	struct swap_info_struct *p;
-
-	p = _swap_info_get(entry);
-	if (p) {
-		if (!__swap_entry_free(p, entry, SWAP_HAS_CACHE))
-			free_swap_slot(entry);
-	}
-}
-
-static void swapcache_free_cluster(swp_entry_t entry)
+void put_swap_page(struct page *page, swp_entry_t entry)
 {
 	unsigned long offset = swp_offset(entry);
 	unsigned long idx = offset / SWAPFILE_CLUSTER;
@@ -1269,39 +1266,41 @@ static void swapcache_free_cluster(swp_entry_t entry)
 	unsigned char *map;
 	unsigned int i, free_entries = 0;
 	unsigned char val;
-
-	if (!IS_ENABLED(CONFIG_THP_SWAP))
-		return;
+	int nr = nr_swap_entries(hpage_nr_pages(page));
 
 	si = _swap_info_get(entry);
 	if (!si)
 		return;
 
-	ci = lock_cluster(si, offset);
-	VM_BUG_ON(!cluster_is_huge(ci));
-	map = si->swap_map + offset;
-	for (i = 0; i < SWAPFILE_CLUSTER; i++) {
-		val = map[i];
-		VM_BUG_ON(!(val & SWAP_HAS_CACHE));
-		if (val == SWAP_HAS_CACHE)
-			free_entries++;
-	}
-	if (!free_entries) {
-		for (i = 0; i < SWAPFILE_CLUSTER; i++)
-			map[i] &= ~SWAP_HAS_CACHE;
-	}
-	cluster_clear_huge(ci);
-	unlock_cluster(ci);
-	if (free_entries == SWAPFILE_CLUSTER) {
-		spin_lock(&si->lock);
+	if (nr == SWAPFILE_CLUSTER) {
 		ci = lock_cluster(si, offset);
-		memset(map, 0, SWAPFILE_CLUSTER);
+		VM_BUG_ON(!cluster_is_huge(ci));
+		map = si->swap_map + offset;
+		for (i = 0; i < SWAPFILE_CLUSTER; i++) {
+			val = map[i];
+			VM_BUG_ON(!(val & SWAP_HAS_CACHE));
+			if (val == SWAP_HAS_CACHE)
+				free_entries++;
+		}
+		if (!free_entries) {
+			for (i = 0; i < SWAPFILE_CLUSTER; i++)
+				map[i] &= ~SWAP_HAS_CACHE;
+		}
+		cluster_clear_huge(ci);
 		unlock_cluster(ci);
-		mem_cgroup_uncharge_swap(entry, SWAPFILE_CLUSTER);
-		swap_free_cluster(si, idx);
-		spin_unlock(&si->lock);
-	} else if (free_entries) {
-		for (i = 0; i < SWAPFILE_CLUSTER; i++, entry.val++) {
+		if (free_entries == SWAPFILE_CLUSTER) {
+			spin_lock(&si->lock);
+			ci = lock_cluster(si, offset);
+			memset(map, 0, SWAPFILE_CLUSTER);
+			unlock_cluster(ci);
+			mem_cgroup_uncharge_swap(entry, SWAPFILE_CLUSTER);
+			swap_free_cluster(si, idx);
+			spin_unlock(&si->lock);
+			return;
+		}
+	}
+	if (nr == 1 || free_entries) {
+		for (i = 0; i < nr; i++, entry.val++) {
 			if (!__swap_entry_free(si, entry, SWAP_HAS_CACHE))
 				free_swap_slot(entry);
 		}
@@ -1325,14 +1324,6 @@ int split_swap_cluster(swp_entry_t entry)
 }
 #endif
 
-void put_swap_page(struct page *page, swp_entry_t entry)
-{
-	if (!PageTransHuge(page))
-		swapcache_free(entry);
-	else
-		swapcache_free_cluster(entry);
-}
-
 static int swp_entry_cmp(const void *ent1, const void *ent2)
 {
 	const swp_entry_t *e1 = ent1, *e2 = ent2;
-- 
2.16.4

[PATCH v2 6/7] swap: Add __swap_entry_free_locked()

[Huang, Ying]

The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked().  Because we want to reuse that
piece of code in some other places.

Just mechanical code refactoring, there is no any functional change in
this function.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 622edc47b67a..fec28f6c05b0 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1180,16 +1180,13 @@ struct swap_info_struct *get_swap_device(swp_entry_t entry)
 	return NULL;
 }
 
-static unsigned char __swap_entry_free(struct swap_info_struct *p,
-				       swp_entry_t entry, unsigned char usage)
+static unsigned char __swap_entry_free_locked(struct swap_info_struct *p,
+					      unsigned long offset,
+					      unsigned char usage)
 {
-	struct swap_cluster_info *ci;
-	unsigned long offset = swp_offset(entry);
 	unsigned char count;
 	unsigned char has_cache;
 
-	ci = lock_cluster_or_swap_info(p, offset);
-
 	count = p->swap_map[offset];
 
 	has_cache = count & SWAP_HAS_CACHE;
@@ -1217,6 +1214,17 @@ static unsigned char __swap_entry_free(struct swap_info_struct *p,
 	usage = count | has_cache;
 	p->swap_map[offset] = usage ? : SWAP_HAS_CACHE;
 
+	return usage;
+}
+
+static unsigned char __swap_entry_free(struct swap_info_struct *p,
+				       swp_entry_t entry, unsigned char usage)
+{
+	struct swap_cluster_info *ci;
+	unsigned long offset = swp_offset(entry);
+
+	ci = lock_cluster_or_swap_info(p, offset);
+	usage = __swap_entry_free_locked(p, offset, usage);
 	unlock_cluster_or_swap_info(p, ci);
 
 	return usage;
-- 
2.16.4

[PATCH v2 7/7] swap, put_swap_page: Share more between huge/normal code path

[Huang, Ying]

In this patch, locking related code is shared between huge/normal code
path in put_swap_page() to reduce code duplication.  And `free_entries
== 0` case is merged into more general `free_entries !=
SWAPFILE_CLUSTER` case, because the new locking method makes it easy.

The added lines is same as the removed lines.  But the code size is
increased when CONFIG_TRANSPARENT_HUGEPAGE=n.

		text	   data	    bss	    dec	    hex	filename
base:	       24215	   2028	    340	  26583	   67d7	mm/swapfile.o
unified:       24577	   2028	    340	  26945	   6941	mm/swapfile.o

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Shaohua Li <shli@kernel.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 mm/swapfile.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index fec28f6c05b0..cd75f449896b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1280,8 +1280,8 @@ void put_swap_page(struct page *page, swp_entry_t entry)
 	if (!si)
 		return;
 
+	ci = lock_cluster_or_swap_info(si, offset);
 	if (nr == SWAPFILE_CLUSTER) {
-		ci = lock_cluster(si, offset);
 		VM_BUG_ON(!cluster_is_huge(ci));
 		map = si->swap_map + offset;
 		for (i = 0; i < SWAPFILE_CLUSTER; i++) {
@@ -1290,13 +1290,9 @@ void put_swap_page(struct page *page, swp_entry_t entry)
 			if (val == SWAP_HAS_CACHE)
 				free_entries++;
 		}
-		if (!free_entries) {
-			for (i = 0; i < SWAPFILE_CLUSTER; i++)
-				map[i] &= ~SWAP_HAS_CACHE;
-		}
 		cluster_clear_huge(ci);
-		unlock_cluster(ci);
 		if (free_entries == SWAPFILE_CLUSTER) {
+			unlock_cluster_or_swap_info(si, ci);
 			spin_lock(&si->lock);
 			ci = lock_cluster(si, offset);
 			memset(map, 0, SWAPFILE_CLUSTER);
@@ -1307,12 +1303,16 @@ void put_swap_page(struct page *page, swp_entry_t entry)
 			return;
 		}
 	}
-	if (nr == 1 || free_entries) {
-		for (i = 0; i < nr; i++, entry.val++) {
-			if (!__swap_entry_free(si, entry, SWAP_HAS_CACHE))
-				free_swap_slot(entry);
+	for (i = 0; i < nr; i++, entry.val++) {
+		if (!__swap_entry_free_locked(si, offset + i, SWAP_HAS_CACHE)) {
+			unlock_cluster_or_swap_info(si, ci);
+			free_swap_slot(entry);
+			if (i == nr - 1)
+				return;
+			lock_cluster_or_swap_info(si, offset);
 		}
 	}
+	unlock_cluster_or_swap_info(si, ci);
 }
 
 #ifdef CONFIG_THP_SWAP
-- 
2.16.4

Re: [PATCH v2 7/7] swap, put_swap_page: Share more between huge/normal code path

[Dave Hansen]

On 07/16/2018 05:55 PM, Huang, Ying wrote:
> 		text	   data	    bss	    dec	    hex	filename
> base:	       24215	   2028	    340	  26583	   67d7	mm/swapfile.o
> unified:       24577	   2028	    340	  26945	   6941	mm/swapfile.o

That's a bit more than I'd expect looking at the rest of the diff.  Make
me wonder if we missed an #ifdef somewhere or the compiler is getting
otherwise confused.

Might be worth a 10-minute look at the disassembly.

Re: [PATCH v2 7/7] swap, put_swap_page: Share more between huge/normal code path

[Huang, Ying]

Dave Hansen <dave.hansen@linux.intel.com> writes:

> On 07/16/2018 05:55 PM, Huang, Ying wrote:
>> 		text	   data	    bss	    dec	    hex	filename
>> base:	       24215	   2028	    340	  26583	   67d7	mm/swapfile.o
>> unified:       24577	   2028	    340	  26945	   6941	mm/swapfile.o
>
> That's a bit more than I'd expect looking at the rest of the diff.  Make
> me wonder if we missed an #ifdef somewhere or the compiler is getting
> otherwise confused.
>
> Might be worth a 10-minute look at the disassembly.

Dig one step deeper via 'size -A mm/swapfile.o' and diff between base
and unified,

--- b.s	2018-07-18 09:42:07.872501680 +0800
+++ h.s	2018-07-18 09:50:37.984499168 +0800
@@ -1,6 +1,6 @@
 mm/swapfile.o  :
 section                               size   addr
-.text                                17815      0
+.text                                17927      0
 .data                                 1288      0
 .bss                                   340      0
 ___ksymtab_gpl+nr_swap_pages             8      0
@@ -26,8 +26,8 @@
 .data.once                               1      0
 .comment                                35      0
 .note.GNU-stack                          0      0
-.orc_unwind_ip                        1380      0
-.orc_unwind                           2070      0
-Total                                26810
+.orc_unwind_ip                        1480      0
+.orc_unwind                           2220      0
+Total                                27172

The total difference is same: 27172 - 26810 = 362 = 24577 - 24215.

The text section difference is small: 17927 - 17815 = 112.  The
additional size change comes from unwinder information: (1480 + 2220) -
(1380 + 2070) = 250.  If the frame pointer unwinder is chosen, this cost
nothing, but if the ORC unwinder is chosen, this is the real difference.

For 112 text section difference, use 'objdump -t' to get symbol size and
compare,

--- b.od	2018-07-18 10:45:05.768483075 +0800
+++ h.od	2018-07-18 10:44:39.556483204 +0800
@@ -30,9 +30,9 @@
 00000000000000a3 cluster_list_add_tail
 000000000000001e __kunmap_atomic.isra.34
 000000000000018c swap_count_continued
-00000000000000ac __swap_entry_free
 000000000000000f put_swap_device.isra.35
 00000000000000b4 inc_cluster_info_page
+000000000000006f __swap_entry_free_locked
 000000000000004a _enable_swap_info
 0000000000000046 wait_on_page_writeback
 000000000000002e inode_to_bdi
@@ -53,8 +53,8 @@
 0000000000000012 __x64_sys_swapon
 0000000000000011 __ia32_sys_swapon
 000000000000007a get_swap_device
-0000000000000032 swap_free
-0000000000000035 put_swap_page
+000000000000006e swap_free
+0000000000000078 put_swap_page
 0000000000000267 swapcache_free_entries
 0000000000000058 page_swapcount
 000000000000003a __swap_count
@@ -64,7 +64,7 @@
 000000000000011a try_to_free_swap
 00000000000001fb get_swap_pages
 0000000000000098 get_swap_page_of_type
-00000000000001b8 free_swap_and_cache
+00000000000001e6 free_swap_and_cache
 0000000000000543 try_to_unuse
 000000000000000e __x64_sys_swapoff
 000000000000000d __ia32_sys_swapoff

The size of put_swap_page() change is small: 0x78 - 0x35 = 67.  But
__swap_entry_free() is inlined by compiler, which cause some code
dilating.

Best Regards,
Huang, Ying

Re: [PATCH v2 7/7] swap, put_swap_page: Share more between huge/normal code path

[Dave Hansen]

On 07/17/2018 07:56 PM, Huang, Ying wrote:
> -.orc_unwind_ip                        1380      0
> -.orc_unwind                           2070      0
> -Total                                26810
> +.orc_unwind_ip                        1480      0
> +.orc_unwind                           2220      0
> +Total                                27172
> 
> The total difference is same: 27172 - 26810 = 362 = 24577 - 24215.
> 
> The text section difference is small: 17927 - 17815 = 112.  The
> additional size change comes from unwinder information: (1480 + 2220) -
> (1380 + 2070) = 250.  If the frame pointer unwinder is chosen, this cost
> nothing, but if the ORC unwinder is chosen, this is the real difference.
> 
> For 112 text section difference, use 'objdump -t' to get symbol size and
> compare,

Cool, thanks for doing this!

I think what you've done here is great for readability and the binary
size increase is well worth the modest size increase.

Re: [PATCH v2 0/7] swap: THP optimizing refactoring

[Daniel Jordan]

On Tue, Jul 17, 2018 at 08:55:49AM +0800, Huang, Ying wrote:
> This patchset is based on 2018-07-13 head of mmotm tree.

Looks good.

Still think patch 7 would be easier to review if split into two logical
changes.  Either way, though.

For the series,
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Re: [PATCH v2 0/7] swap: THP optimizing refactoring

[Huang, Ying]

Daniel Jordan <daniel.m.jordan@oracle.com> writes:

> On Tue, Jul 17, 2018 at 08:55:49AM +0800, Huang, Ying wrote:
>> This patchset is based on 2018-07-13 head of mmotm tree.
>
> Looks good.
>
> Still think patch 7 would be easier to review if split into two logical
> changes.  Either way, though.
>
> For the series,
> Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Thanks a lot for your review!

Best Regards,
Huang, Ying