[Patch v2 1/3] mm/swapfile.c: found_free could be represented by (tmp < max)

[Patch v2 1/3] mm/swapfile.c: found_free could be represented by (tmp < max)

[Wei Yang]

This is not necessary to use the variable found_free to record the
status. Just check tmp and max is enough.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/swapfile.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 469ab417ed43..d203cdc6750a 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -601,7 +601,6 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
 {
 	struct percpu_cluster *cluster;
 	struct swap_cluster_info *ci;
-	bool found_free;
 	unsigned long tmp, max;
 
 new_cluster:
@@ -623,8 +622,6 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
 			return false;
 	}
 
-	found_free = false;
-
 	/*
 	 * Other CPUs can use our cluster if they can't find a free cluster,
 	 * check if there is still free entry in the cluster
@@ -638,21 +635,19 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
 	}
 	ci = lock_cluster(si, tmp);
 	while (tmp < max) {
-		if (!si->swap_map[tmp]) {
-			found_free = true;
+		if (!si->swap_map[tmp])
 			break;
-		}
 		tmp++;
 	}
 	unlock_cluster(ci);
-	if (!found_free) {
+	if (tmp >= max) {
 		cluster_set_null(&cluster->index);
 		goto new_cluster;
 	}
 	cluster->next = tmp + 1;
 	*offset = tmp;
 	*scan_base = tmp;
-	return found_free;
+	return tmp < max;
 }
 
 static void __del_from_avail_list(struct swap_info_struct *p)
-- 
2.23.0

[Patch v2 2/3] mm/swapfile.c: tmp is always smaller than max

[Wei Yang]

If tmp is bigger or equal to max, we would jump to new_cluster.

Return true directly.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/swapfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index d203cdc6750a..bc435c2eb916 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -647,7 +647,7 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
 	cluster->next = tmp + 1;
 	*offset = tmp;
 	*scan_base = tmp;
-	return tmp < max;
+	return true;
 }
 
 static void __del_from_avail_list(struct swap_info_struct *p)
-- 
2.23.0

[Patch v2 3/3] mm/swapfile.c: omit a duplicate code by compare tmp and max first

[Wei Yang]

There are two duplicate code to handle the case when there is no
available swap entry. To avoid this, we can compare tmp and max first
and let the second guard do its job.

No functional change is expected.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

---
v2: Huang Ying suggest to do this check to avoid lock contention
---
 mm/swapfile.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index bc435c2eb916..f3eee6d8cddf 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -629,17 +629,15 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
 	tmp = cluster->next;
 	max = min_t(unsigned long, si->max,
 		    (cluster_next(&cluster->index) + 1) * SWAPFILE_CLUSTER);
-	if (tmp >= max) {
-		cluster_set_null(&cluster->index);
-		goto new_cluster;
-	}
-	ci = lock_cluster(si, tmp);
-	while (tmp < max) {
-		if (!si->swap_map[tmp])
-			break;
-		tmp++;
+	if (tmp < max) {
+		ci = lock_cluster(si, tmp);
+		while (tmp < max) {
+			if (!si->swap_map[tmp])
+				break;
+			tmp++;
+		}
+		unlock_cluster(ci);
 	}
-	unlock_cluster(ci);
 	if (tmp >= max) {
 		cluster_set_null(&cluster->index);
 		goto new_cluster;
-- 
2.23.0

Re: [Patch v2 1/3] mm/swapfile.c: found_free could be represented by (tmp < max)

[Huang, Ying]

Wei Yang <richard.weiyang@gmail.com> writes:

> This is not necessary to use the variable found_free to record the
> status. Just check tmp and max is enough.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

All 3 patches looks good to me.  Thanks!

Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

Best Regards,
Huang, Ying

> ---
>  mm/swapfile.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 469ab417ed43..d203cdc6750a 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -601,7 +601,6 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>  {
>  	struct percpu_cluster *cluster;
>  	struct swap_cluster_info *ci;
> -	bool found_free;
>  	unsigned long tmp, max;
>  
>  new_cluster:
> @@ -623,8 +622,6 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>  			return false;
>  	}
>  
> -	found_free = false;
> -
>  	/*
>  	 * Other CPUs can use our cluster if they can't find a free cluster,
>  	 * check if there is still free entry in the cluster
> @@ -638,21 +635,19 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>  	}
>  	ci = lock_cluster(si, tmp);
>  	while (tmp < max) {
> -		if (!si->swap_map[tmp]) {
> -			found_free = true;
> +		if (!si->swap_map[tmp])
>  			break;
> -		}
>  		tmp++;
>  	}
>  	unlock_cluster(ci);
> -	if (!found_free) {
> +	if (tmp >= max) {
>  		cluster_set_null(&cluster->index);
>  		goto new_cluster;
>  	}
>  	cluster->next = tmp + 1;
>  	*offset = tmp;
>  	*scan_base = tmp;
> -	return found_free;
> +	return tmp < max;
>  }
>  
>  static void __del_from_avail_list(struct swap_info_struct *p)