[PATCH 1/4] mm/swapfile.c: found_free could be represented by (tmp < max)

[PATCH 1/4] 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 c457f3be6944..654bad5173bc 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 2/4] 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 654bad5173bc..3aae700f9931 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 3/4] mm/swapfile.c: compare tmp and max after trying to iterate on swap_map

[Wei Yang]

There are two duplicate code to handle the case when there is no
available swap entry. Just let the code go through and do the check at
second place.

No functional change is expected.

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

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3aae700f9931..07b0bc095411 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -629,10 +629,6 @@ 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])
-- 
2.23.0

[PATCH 4/4] mm/swapfile.c: move new_cluster to check free_clusters directly

[Wei Yang]

Each time it needs jump to new_cluster, it is sure current
percpu_cluster is null.

Move the new_cluster to check free_clusters 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 07b0bc095411..78e92ff14c79 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -603,9 +603,9 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
 	struct swap_cluster_info *ci;
 	unsigned long tmp, max;
 
-new_cluster:
 	cluster = this_cpu_ptr(si->percpu_cluster);
 	if (cluster_is_null(&cluster->index)) {
+new_cluster:
 		if (!cluster_list_empty(&si->free_clusters)) {
 			cluster->index = si->free_clusters.head;
 			cluster->next = cluster_next(&cluster->index) *
-- 
2.23.0

Re: [PATCH 3/4] mm/swapfile.c: compare tmp and max after trying to iterate on swap_map

[Huang, Ying]

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

> There are two duplicate code to handle the case when there is no
> available swap entry. Just let the code go through and do the check at
> second place.
>
> No functional change is expected.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/swapfile.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3aae700f9931..07b0bc095411 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -629,10 +629,6 @@ 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;
> -	}

The code is to avoid to acquire the cluster lock unnecessarily.  So I think
we should keep this.

Best Regards,
Huang, Ying

>  	ci = lock_cluster(si, tmp);
>  	while (tmp < max) {
>  		if (!si->swap_map[tmp])

Re: [PATCH 4/4] mm/swapfile.c: move new_cluster to check free_clusters directly

[Huang, Ying]

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

> Each time it needs jump to new_cluster, it is sure current
> percpu_cluster is null.
>
> Move the new_cluster to check free_clusters 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 07b0bc095411..78e92ff14c79 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -603,9 +603,9 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>  	struct swap_cluster_info *ci;
>  	unsigned long tmp, max;
>  
> -new_cluster:
>  	cluster = this_cpu_ptr(si->percpu_cluster);
>  	if (cluster_is_null(&cluster->index)) {
> +new_cluster:
>  		if (!cluster_list_empty(&si->free_clusters)) {
>  			cluster->index = si->free_clusters.head;
>  			cluster->next = cluster_next(&cluster->index) *

In swap_do_scheduled_discard(), we will unlock si->lock, so the
percpu_cluster may be changed after we releasing the lock.  Or the
current thread may be moved to a different CPU.

Best Regards,
Huang, Ying

Re: [PATCH 3/4] mm/swapfile.c: compare tmp and max after trying to iterate on swap_map

[Wei Yang]

On Mon, Apr 20, 2020 at 09:03:43AM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
>> There are two duplicate code to handle the case when there is no
>> available swap entry. Just let the code go through and do the check at
>> second place.
>>
>> No functional change is expected.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  mm/swapfile.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 3aae700f9931..07b0bc095411 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -629,10 +629,6 @@ 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;
>> -	}
>
>The code is to avoid to acquire the cluster lock unnecessarily.  So I think
>we should keep this.
>

If you really want to avoid the lock, my suggestion is to add:

  if (tmp < max) {
      ci = lock_cluster(si, tmp);
          while (tmp < max) {
	  ...
	  }
      unlock_cluster(ci);
  }

Instead of do the similar thing twice.

>Best Regards,
>Huang, Ying
>
>>  	ci = lock_cluster(si, tmp);
>>  	while (tmp < max) {
>>  		if (!si->swap_map[tmp])

-- 
Wei Yang
Help you, Help me

Re: [PATCH 4/4] mm/swapfile.c: move new_cluster to check free_clusters directly

[Wei Yang]

On Mon, Apr 20, 2020 at 09:41:43AM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
>> Each time it needs jump to new_cluster, it is sure current
>> percpu_cluster is null.
>>
>> Move the new_cluster to check free_clusters 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 07b0bc095411..78e92ff14c79 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -603,9 +603,9 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>>  	struct swap_cluster_info *ci;
>>  	unsigned long tmp, max;
>>  
>> -new_cluster:
>>  	cluster = this_cpu_ptr(si->percpu_cluster);
>>  	if (cluster_is_null(&cluster->index)) {
>> +new_cluster:
>>  		if (!cluster_list_empty(&si->free_clusters)) {
>>  			cluster->index = si->free_clusters.head;
>>  			cluster->next = cluster_next(&cluster->index) *
>
>In swap_do_scheduled_discard(), we will unlock si->lock, so the
>percpu_cluster may be changed after we releasing the lock.  Or the
>current thread may be moved to a different CPU.

Thanks, you are right.

>
>Best Regards,
>Huang, Ying

-- 
Wei Yang
Help you, Help me

Re: [PATCH 3/4] mm/swapfile.c: compare tmp and max after trying to iterate on swap_map

[Huang, Ying]

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

> On Mon, Apr 20, 2020 at 09:03:43AM +0800, Huang, Ying wrote:
>>Wei Yang <richard.weiyang@gmail.com> writes:
>>
>>> There are two duplicate code to handle the case when there is no
>>> available swap entry. Just let the code go through and do the check at
>>> second place.
>>>
>>> No functional change is expected.
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> ---
>>>  mm/swapfile.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>>> index 3aae700f9931..07b0bc095411 100644
>>> --- a/mm/swapfile.c
>>> +++ b/mm/swapfile.c
>>> @@ -629,10 +629,6 @@ 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;
>>> -	}
>>
>>The code is to avoid to acquire the cluster lock unnecessarily.  So I think
>>we should keep this.
>>
>
> If you really want to avoid the lock, my suggestion is to add:
>
>   if (tmp < max) {
>       ci = lock_cluster(si, tmp);
>           while (tmp < max) {
> 	  ...
> 	  }
>       unlock_cluster(ci);
>   }
>
> Instead of do the similar thing twice.

This is a coding style problem.  The original code is common to avoid
too many nested code block.  But in this case, I think both works.

Best Regards,
Huang, Ying

>>Best Regards,
>>Huang, Ying
>>
>>>  	ci = lock_cluster(si, tmp);
>>>  	while (tmp < max) {
>>>  		if (!si->swap_map[tmp])