| Message ID | 20231213-zswap-dstmem-v3-4-4eac09b94ece@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/zswap: dstmem reuse optimizations and cleanups | expand |
On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou <zhouchengming@bytedance.com> wrote: > > After the common decompress part goes to __zswap_load(), we can cleanup > the zswap_load() a little. > > Reviewed-by: Yosry Ahmed <yosryahmed@google.com> > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > mm/zswap.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 3433bd6b3cef..86886276cb81 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1759,7 +1759,6 @@ bool zswap_load(struct folio *folio) > struct zswap_tree *tree = zswap_trees[type]; > struct zswap_entry *entry; > u8 *dst; > - bool ret; > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > > @@ -1776,19 +1775,16 @@ bool zswap_load(struct folio *folio) > dst = kmap_local_page(page); > zswap_fill_page(dst, entry->value); > kunmap_local(dst); > - ret = true; > - goto stats; > + } else { > + __zswap_load(entry, page); Very minor nitpick. I think this change you only take out the "ret", you don't have to remove the goto. Personally I prefer the one with the goto because If (!entry->length) is a rare case, having them indented match the normal execution flow is the streamlined one without indentation. If you keep the else statement without the goto. You can move __zswap_load(entry,page) to the if statement so most common case go through the if statement rather than else. I also think this commit can fold into the previous one. As I said, this is minor comment, it is your call. Acked-by: Chis Li <chrisl@kernel.org> (Google) Chris
On 2023/12/19 20:47, Chris Li wrote: > On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou > <zhouchengming@bytedance.com> wrote: >> >> After the common decompress part goes to __zswap_load(), we can cleanup >> the zswap_load() a little. >> >> Reviewed-by: Yosry Ahmed <yosryahmed@google.com> >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> mm/zswap.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/mm/zswap.c b/mm/zswap.c >> index 3433bd6b3cef..86886276cb81 100644 >> --- a/mm/zswap.c >> +++ b/mm/zswap.c >> @@ -1759,7 +1759,6 @@ bool zswap_load(struct folio *folio) >> struct zswap_tree *tree = zswap_trees[type]; >> struct zswap_entry *entry; >> u8 *dst; >> - bool ret; >> >> VM_WARN_ON_ONCE(!folio_test_locked(folio)); >> >> @@ -1776,19 +1775,16 @@ bool zswap_load(struct folio *folio) >> dst = kmap_local_page(page); >> zswap_fill_page(dst, entry->value); >> kunmap_local(dst); >> - ret = true; >> - goto stats; >> + } else { >> + __zswap_load(entry, page); > > Very minor nitpick. I think this change you only take out the "ret", > you don't have to remove the goto. Personally I prefer the one with > the goto because If (!entry->length) is a rare case, having them > indented match the normal execution flow is the streamlined one > without indentation. > > If you keep the else statement without the goto. You can move > __zswap_load(entry,page) to the if statement so most common case go > through the if statement rather than else. Make sense. I will change if a new version is needed. > > I also think this commit can fold into the previous one. As I said, > this is minor comment, it is your call. > > Acked-by: Chis Li <chrisl@kernel.org> (Google) Thanks for review!
diff --git a/mm/zswap.c b/mm/zswap.c index 3433bd6b3cef..86886276cb81 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1759,7 +1759,6 @@ bool zswap_load(struct folio *folio) struct zswap_tree *tree = zswap_trees[type]; struct zswap_entry *entry; u8 *dst; - bool ret; VM_WARN_ON_ONCE(!folio_test_locked(folio)); @@ -1776,19 +1775,16 @@ bool zswap_load(struct folio *folio) dst = kmap_local_page(page); zswap_fill_page(dst, entry->value); kunmap_local(dst); - ret = true; - goto stats; + } else { + __zswap_load(entry, page); } - __zswap_load(entry, page); - ret = true; -stats: count_vm_event(ZSWPIN); if (entry->objcg) count_objcg_event(entry->objcg, ZSWPIN); spin_lock(&tree->lock); - if (ret && zswap_exclusive_loads_enabled) { + if (zswap_exclusive_loads_enabled) { zswap_invalidate_entry(tree, entry); folio_mark_dirty(folio); } else if (entry->length) { @@ -1798,7 +1794,7 @@ bool zswap_load(struct folio *folio) zswap_entry_put(tree, entry); spin_unlock(&tree->lock); - return ret; + return true; } void zswap_invalidate(int type, pgoff_t offset)