| Message ID | 20220509131416.17553-7-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup patches for swap | expand |
On 09.05.22 15:14, Miaohe Lin wrote: > refill_swap_slots_cache is always called when cache->nr is 0. And if > cache->nr != 0, we should return cache->nr instead of 0. So remove > such buggy and confusing check. Not sure about the "cache->nr != 0, we should return cache->nr instead of 0" part, I'd just drop that from the patch description. We'd actually end up overwriting cache->nr after your change, which doesn't sound right and also different to what you describe here. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/swap_slots.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/swap_slots.c b/mm/swap_slots.c > index 2f877e6f87d7..2a65a89b5b4d 100644 > --- a/mm/swap_slots.c > +++ b/mm/swap_slots.c > @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void) > /* called with swap slot cache's alloc lock held */ > static int refill_swap_slots_cache(struct swap_slots_cache *cache) > { > - if (!use_swap_slot_cache || cache->nr) > + if (!use_swap_slot_cache) > return 0; > > cache->cur = 0; I feel like if this function would be called with cache->nr, it would be a BUG. So I'm fine with removing it, but we could also think about turning it into some sort of WARN/BG to make it clearer that this is unexpected. Anyhow, Acked-by: David Hildenbrand <david@redhat.com>
On 2022/5/12 21:37, David Hildenbrand wrote: > On 09.05.22 15:14, Miaohe Lin wrote: >> refill_swap_slots_cache is always called when cache->nr is 0. And if >> cache->nr != 0, we should return cache->nr instead of 0. So remove >> such buggy and confusing check. > > Not sure about the "cache->nr != 0, we should return cache->nr instead > of 0" part, I'd just drop that from the patch description. We'd actually > end up overwriting cache->nr after your change, which doesn't sound > right and also different to what you describe here. Will do. > >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/swap_slots.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/swap_slots.c b/mm/swap_slots.c >> index 2f877e6f87d7..2a65a89b5b4d 100644 >> --- a/mm/swap_slots.c >> +++ b/mm/swap_slots.c >> @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void) >> /* called with swap slot cache's alloc lock held */ >> static int refill_swap_slots_cache(struct swap_slots_cache *cache) >> { >> - if (!use_swap_slot_cache || cache->nr) >> + if (!use_swap_slot_cache) >> return 0; >> >> cache->cur = 0; > > I feel like if this function would be called with cache->nr, it would be > a BUG. So I'm fine with removing it, but we could also think about > turning it into some sort of WARN/BG to make it clearer that this is > unexpected. Since refill_swap_slots_cache is only called by folio_alloc_swap when cache->nr == 0. I think it might be too overkill to add a WARN/BG. > > > Anyhow, > > Acked-by: David Hildenbrand <david@redhat.com> Many thanks for comment and Acked-by tag! >
On Mon, 16 May 2022 10:00:43 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > On 2022/5/12 21:37, David Hildenbrand wrote: > > On 09.05.22 15:14, Miaohe Lin wrote: > >> refill_swap_slots_cache is always called when cache->nr is 0. And if > >> cache->nr != 0, we should return cache->nr instead of 0. So remove > >> such buggy and confusing check. > > > > Not sure about the "cache->nr != 0, we should return cache->nr instead > > of 0" part, I'd just drop that from the patch description. We'd actually > > end up overwriting cache->nr after your change, which doesn't sound > > right and also different to what you describe here. > > Will do. I've updated the changleog to simply : refill_swap_slots_cache is always called when cache->nr is 0. So remove : such buggy and confusing check.
On Mon, May 09, 2022 at 09:14:07PM +0800, Miaohe Lin wrote: > refill_swap_slots_cache is always called when cache->nr is 0. And if > cache->nr != 0, we should return cache->nr instead of 0. So remove > such buggy and confusing check. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> With Andrew's ammendment: Reviewed-by: Oscar Salvador <osalvador@suse.de> > --- > mm/swap_slots.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/swap_slots.c b/mm/swap_slots.c > index 2f877e6f87d7..2a65a89b5b4d 100644 > --- a/mm/swap_slots.c > +++ b/mm/swap_slots.c > @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void) > /* called with swap slot cache's alloc lock held */ > static int refill_swap_slots_cache(struct swap_slots_cache *cache) > { > - if (!use_swap_slot_cache || cache->nr) > + if (!use_swap_slot_cache) > return 0; > > cache->cur = 0; > -- > 2.23.0 > >
diff --git a/mm/swap_slots.c b/mm/swap_slots.c index 2f877e6f87d7..2a65a89b5b4d 100644 --- a/mm/swap_slots.c +++ b/mm/swap_slots.c @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void) /* called with swap slot cache's alloc lock held */ static int refill_swap_slots_cache(struct swap_slots_cache *cache) { - if (!use_swap_slot_cache || cache->nr) + if (!use_swap_slot_cache) return 0; cache->cur = 0;
refill_swap_slots_cache is always called when cache->nr is 0. And if cache->nr != 0, we should return cache->nr instead of 0. So remove such buggy and confusing check. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/swap_slots.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)