| Message ID | 20220509131416.17553-12-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup patches for swap | expand |
On Mon, 09 May 2022, Miaohe Lin wrote: > Add helper swap_offset_available() to remove some duplicated codes. > Minor readability improvement. I don't think that putting the spin_lock() inside the inline helper is good for readability. If the function was called swap_offset_available_and_locked() it might be ok. Otherwise I would rather the spin_lock() was called when the function returned true. Thanks, NeilBrown > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/swapfile.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index c90298a0561a..d5d3e2d03d28 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -776,6 +776,21 @@ static void set_cluster_next(struct swap_info_struct *si, unsigned long next) > this_cpu_write(*si->cluster_next_cpu, next); > } > > +static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset) > +{ > + if (data_race(!si->swap_map[offset])) { > + spin_lock(&si->lock); > + return true; > + } > + > + if (vm_swap_full() && READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { > + spin_lock(&si->lock); > + return true; > + } > + > + return false; > +} > + > static int scan_swap_map_slots(struct swap_info_struct *si, > unsigned char usage, int nr, > swp_entry_t slots[]) > @@ -953,15 +968,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si, > scan: > spin_unlock(&si->lock); > while (++offset <= READ_ONCE(si->highest_bit)) { > - if (data_race(!si->swap_map[offset])) { > - spin_lock(&si->lock); > + if (swap_offset_available(si, offset)) > goto checks; > - } > - if (vm_swap_full() && > - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { > - spin_lock(&si->lock); > - goto checks; > - } > if (unlikely(--latency_ration < 0)) { > cond_resched(); > latency_ration = LATENCY_LIMIT; > @@ -970,15 +978,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si, > } > offset = si->lowest_bit; > while (offset < scan_base) { > - if (data_race(!si->swap_map[offset])) { > - spin_lock(&si->lock); > + if (swap_offset_available(si, offset)) > goto checks; > - } > - if (vm_swap_full() && > - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { > - spin_lock(&si->lock); > - goto checks; > - } > if (unlikely(--latency_ration < 0)) { > cond_resched(); > latency_ration = LATENCY_LIMIT; > -- > 2.23.0 > >
On 2022/5/10 8:45, NeilBrown wrote: > On Mon, 09 May 2022, Miaohe Lin wrote: >> Add helper swap_offset_available() to remove some duplicated codes. >> Minor readability improvement. > > I don't think that putting the spin_lock() inside the inline helper is > good for readability. > If the function was called > swap_offset_available_and_locked() Yes, swap_offset_available_and_locked should be more suitable as we do the spin_lock inside it. Will do this in next version. Thanks! > > it might be ok. Otherwise I would rather the spin_lock() was called > when the function returned true. > > Thanks, > NeilBrown > >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/swapfile.c | 33 +++++++++++++++++---------------- >> 1 file changed, 17 insertions(+), 16 deletions(-) >> >> diff --git a/mm/swapfile.c b/mm/swapfile.c >> index c90298a0561a..d5d3e2d03d28 100644 >> --- a/mm/swapfile.c >> +++ b/mm/swapfile.c >> @@ -776,6 +776,21 @@ static void set_cluster_next(struct swap_info_struct *si, unsigned long next) >> this_cpu_write(*si->cluster_next_cpu, next); >> } >> >> +static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset) >> +{ >> + if (data_race(!si->swap_map[offset])) { >> + spin_lock(&si->lock); >> + return true; >> + } >> + >> + if (vm_swap_full() && READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { >> + spin_lock(&si->lock); >> + return true; >> + } >> + >> + return false; >> +} >> + >> static int scan_swap_map_slots(struct swap_info_struct *si, >> unsigned char usage, int nr, >> swp_entry_t slots[]) >> @@ -953,15 +968,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si, >> scan: >> spin_unlock(&si->lock); >> while (++offset <= READ_ONCE(si->highest_bit)) { >> - if (data_race(!si->swap_map[offset])) { >> - spin_lock(&si->lock); >> + if (swap_offset_available(si, offset)) >> goto checks; >> - } >> - if (vm_swap_full() && >> - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { >> - spin_lock(&si->lock); >> - goto checks; >> - } >> if (unlikely(--latency_ration < 0)) { >> cond_resched(); >> latency_ration = LATENCY_LIMIT; >> @@ -970,15 +978,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si, >> } >> offset = si->lowest_bit; >> while (offset < scan_base) { >> - if (data_race(!si->swap_map[offset])) { >> - spin_lock(&si->lock); >> + if (swap_offset_available(si, offset)) >> goto checks; >> - } >> - if (vm_swap_full() && >> - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { >> - spin_lock(&si->lock); >> - goto checks; >> - } >> if (unlikely(--latency_ration < 0)) { >> cond_resched(); >> latency_ration = LATENCY_LIMIT; >> -- >> 2.23.0 >> >> > . >
On Tue, 10 May 2022 10:03:19 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > On 2022/5/10 8:45, NeilBrown wrote: > > On Mon, 09 May 2022, Miaohe Lin wrote: > >> Add helper swap_offset_available() to remove some duplicated codes. > >> Minor readability improvement. > > > > I don't think that putting the spin_lock() inside the inline helper is > > good for readability. > > If the function was called > > swap_offset_available_and_locked() > > Yes, swap_offset_available_and_locked should be more suitable as we do the spin_lock > inside it. Will do this in next version. > --- a/mm/swapfile.c~mm-swap-add-helper-swap_offset_available-fix +++ a/mm/swapfile.c @@ -775,7 +775,8 @@ static void set_cluster_next(struct swap this_cpu_write(*si->cluster_next_cpu, next); } -static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset) +static bool swap_offset_available_and_locked(struct swap_info_struct *si, + unsigned long offset) { if (data_race(!si->swap_map[offset])) { spin_lock(&si->lock); @@ -967,7 +968,7 @@ done: scan: spin_unlock(&si->lock); while (++offset <= READ_ONCE(si->highest_bit)) { - if (swap_offset_available(si, offset)) + if (swap_offset_available_and_locked(si, offset)) goto checks; if (unlikely(--latency_ration < 0)) { cond_resched(); @@ -977,7 +978,7 @@ scan: } offset = si->lowest_bit; while (offset < scan_base) { - if (swap_offset_available(si, offset)) + if (swap_offset_available_and_locked(si, offset)) goto checks; if (unlikely(--latency_ration < 0)) { cond_resched();
diff --git a/mm/swapfile.c b/mm/swapfile.c index c90298a0561a..d5d3e2d03d28 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -776,6 +776,21 @@ static void set_cluster_next(struct swap_info_struct *si, unsigned long next) this_cpu_write(*si->cluster_next_cpu, next); } +static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset) +{ + if (data_race(!si->swap_map[offset])) { + spin_lock(&si->lock); + return true; + } + + if (vm_swap_full() && READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { + spin_lock(&si->lock); + return true; + } + + return false; +} + static int scan_swap_map_slots(struct swap_info_struct *si, unsigned char usage, int nr, swp_entry_t slots[]) @@ -953,15 +968,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si, scan: spin_unlock(&si->lock); while (++offset <= READ_ONCE(si->highest_bit)) { - if (data_race(!si->swap_map[offset])) { - spin_lock(&si->lock); + if (swap_offset_available(si, offset)) goto checks; - } - if (vm_swap_full() && - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { - spin_lock(&si->lock); - goto checks; - } if (unlikely(--latency_ration < 0)) { cond_resched(); latency_ration = LATENCY_LIMIT; @@ -970,15 +978,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si, } offset = si->lowest_bit; while (offset < scan_base) { - if (data_race(!si->swap_map[offset])) { - spin_lock(&si->lock); + if (swap_offset_available(si, offset)) goto checks; - } - if (vm_swap_full() && - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { - spin_lock(&si->lock); - goto checks; - } if (unlikely(--latency_ration < 0)) { cond_resched(); latency_ration = LATENCY_LIMIT;
Add helper swap_offset_available() to remove some duplicated codes. Minor readability improvement. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/swapfile.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)