| Message ID | 20200419013921.14390-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] mm/swapfile.c: found_free could be represented by (tmp < max) | expand |
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])
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 <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])
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)
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(-)