diff mbox series

[05/20] mm: zswap: clean up zswap_entry_put()

Message ID 20240130014208.565554-6-hannes@cmpxchg.org
State New
Headers
Series mm: zswap: cleanups |

Commit Message

Johannes Weiner Jan. 30, 2024, 1:36 a.m. UTC 
  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(-)

Comments

Chengming Zhou Jan. 30, 2024, 3:39 a.m. UTC | #1 
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);
>  	}
Yosry Ahmed Jan. 30, 2024, 7:51 a.m. UTC | #2 
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
>
Yosry Ahmed Jan. 30, 2024, 8:10 a.m. UTC | #3 
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>
Nhat Pham Jan. 30, 2024, 4:31 p.m. UTC | #4 
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>
Yosry Ahmed Jan. 30, 2024, 5:02 p.m. UTC | #5 
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 mbox series

Patch

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);
 	}