| Message ID | 20240130014208.565554-6-hannes@cmpxchg.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: zswap: cleanups | expand |
On 2024/1/30 09:36, Johannes Weiner wrote: > Remove stale comment and unnecessary local variable. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> Reviewed-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > mm/zswap.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 0c6adaf2fdb6..7a7e8da2b4f8 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -546,15 +546,11 @@ static void zswap_entry_get(struct zswap_entry *entry) > entry->refcount++; > } > > -/* caller must hold the tree lock > -* remove from the tree and free it, if nobody reference the entry > -*/ > +/* caller must hold the tree lock */ > static void zswap_entry_put(struct zswap_entry *entry) > { > - int refcount = --entry->refcount; > - > - WARN_ON_ONCE(refcount < 0); > - if (refcount == 0) { > + WARN_ON_ONCE(!entry->refcount); > + if (--entry->refcount == 0) { > WARN_ON_ONCE(!RB_EMPTY_NODE(&entry->rbnode)); > zswap_entry_free(entry); > }
On Mon, Jan 29, 2024 at 08:36:41PM -0500, Johannes Weiner wrote: > Remove stale comment and unnecessary local variable. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/zswap.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 0c6adaf2fdb6..7a7e8da2b4f8 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -546,15 +546,11 @@ static void zswap_entry_get(struct zswap_entry *entry) > entry->refcount++; > } > > -/* caller must hold the tree lock > -* remove from the tree and free it, if nobody reference the entry > -*/ > +/* caller must hold the tree lock */ We should replace all those "caller must hold the tree lock" comments with lockdep_assert_held() or assert_spin_locked() or something. I can send follow up patches on top if you don't want to resend this series. > static void zswap_entry_put(struct zswap_entry *entry) > { > - int refcount = --entry->refcount; > - > - WARN_ON_ONCE(refcount < 0); > - if (refcount == 0) { > + WARN_ON_ONCE(!entry->refcount); > + if (--entry->refcount == 0) { > WARN_ON_ONCE(!RB_EMPTY_NODE(&entry->rbnode)); > zswap_entry_free(entry); > } > -- > 2.43.0 >
On Mon, Jan 29, 2024 at 08:36:41PM -0500, Johannes Weiner wrote: > Remove stale comment and unnecessary local variable. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> Acked-by: Yosry Ahmed <yosryahmed@google.com>
On Mon, Jan 29, 2024 at 11:51 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > On Mon, Jan 29, 2024 at 08:36:41PM -0500, Johannes Weiner wrote: > > Remove stale comment and unnecessary local variable. > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > --- > > mm/zswap.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 0c6adaf2fdb6..7a7e8da2b4f8 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -546,15 +546,11 @@ static void zswap_entry_get(struct zswap_entry *entry) > > entry->refcount++; > > } > > > > -/* caller must hold the tree lock > > -* remove from the tree and free it, if nobody reference the entry > > -*/ > > +/* caller must hold the tree lock */ > > We should replace all those "caller must hold the tree lock" comments > with lockdep_assert_held() or assert_spin_locked() or something. > > I can send follow up patches on top if you don't want to resend this > series. Agree. There's also this: /* should be called under RCU */ #ifdef CONFIG_MEMCG static inline struct mem_cgroup *mem_cgroup_from_entry(struct zswap_entry *entry) { return entry->objcg ? obj_cgroup_memcg(entry->objcg) : NULL; } which you pointed out in the per-cgroup zswap LRU review :) > > > static void zswap_entry_put(struct zswap_entry *entry) > > { > > - int refcount = --entry->refcount; > > - > > - WARN_ON_ONCE(refcount < 0); > > - if (refcount == 0) { > > + WARN_ON_ONCE(!entry->refcount); > > + if (--entry->refcount == 0) { > > WARN_ON_ONCE(!RB_EMPTY_NODE(&entry->rbnode)); > > zswap_entry_free(entry); > > } > > -- > > 2.43.0 > > Reviewed-by: Nhat Pham <nphamcs@gmail.com>
On Tue, Jan 30, 2024 at 08:31:22AM -0800, Nhat Pham wrote: > On Mon, Jan 29, 2024 at 11:51 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Mon, Jan 29, 2024 at 08:36:41PM -0500, Johannes Weiner wrote: > > > Remove stale comment and unnecessary local variable. > > > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > > --- > > > mm/zswap.c | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > index 0c6adaf2fdb6..7a7e8da2b4f8 100644 > > > --- a/mm/zswap.c > > > +++ b/mm/zswap.c > > > @@ -546,15 +546,11 @@ static void zswap_entry_get(struct zswap_entry *entry) > > > entry->refcount++; > > > } > > > > > > -/* caller must hold the tree lock > > > -* remove from the tree and free it, if nobody reference the entry > > > -*/ > > > +/* caller must hold the tree lock */ > > > > We should replace all those "caller must hold the tree lock" comments > > with lockdep_assert_held() or assert_spin_locked() or something. > > > > I can send follow up patches on top if you don't want to resend this > > series. > > Agree. There's also this: > > /* should be called under RCU */ > #ifdef CONFIG_MEMCG > static inline struct mem_cgroup *mem_cgroup_from_entry(struct > zswap_entry *entry) > { > return entry->objcg ? obj_cgroup_memcg(entry->objcg) : NULL; > } > > which you pointed out in the per-cgroup zswap LRU review :) I will send out a patch this week or so.
diff --git a/mm/zswap.c b/mm/zswap.c index 0c6adaf2fdb6..7a7e8da2b4f8 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -546,15 +546,11 @@ static void zswap_entry_get(struct zswap_entry *entry) entry->refcount++; } -/* caller must hold the tree lock -* remove from the tree and free it, if nobody reference the entry -*/ +/* caller must hold the tree lock */ static void zswap_entry_put(struct zswap_entry *entry) { - int refcount = --entry->refcount; - - WARN_ON_ONCE(refcount < 0); - if (refcount == 0) { + WARN_ON_ONCE(!entry->refcount); + if (--entry->refcount == 0) { WARN_ON_ONCE(!RB_EMPTY_NODE(&entry->rbnode)); zswap_entry_free(entry); }
Remove stale comment and unnecessary local variable. Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/zswap.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)