diff mbox series

[11/20] mm: zswap: function ordering: pool refcounting

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

Commit Message

Johannes Weiner Jan. 30, 2024, 1:36 a.m. UTC 
  Move pool refcounting functions into the pool section. First the
destroy functions, then the get and put which uses them.

__zswap_pool_empty() has an upward reference to the global
zswap_pools, to sanity check it's not the currently active pool that's
being freed. That gets the forward decl for zswap_pool_cuyrrent().

This puts the get and put function above all callers, so kill the
forward decls as well.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/zswap.c | 94 +++++++++++++++++++++++++++---------------------------
 1 file changed, 47 insertions(+), 47 deletions(-)

Comments

Nhat Pham Jan. 30, 2024, 8:13 p.m. UTC | #1 
On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> Move pool refcounting functions into the pool section. First the
> destroy functions, then the get and put which uses them.
>
> __zswap_pool_empty() has an upward reference to the global
> zswap_pools, to sanity check it's not the currently active pool that's
> being freed. That gets the forward decl for zswap_pool_cuyrrent().

nit: zswap_pool_cuyrrent() -> zswap_pool_current() :-)

Also, would it make sense to move zswap_pool_current() above
__zswap_pool_empty() to get rid of the forward declaration? I guess
it's now grouped with current_get() etc. - those don't seem to use the
empty check, so maybe they can also go above __zswap_pool_empty()?


>
> This puts the get and put function above all callers, so kill the
> forward decls as well.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/zswap.c | 94 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 805d9a35f633..33775f2224b7 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -278,8 +278,6 @@ static inline struct zswap_tree *swap_zswap_tree(swp_entry_t swp)
>
>  static int zswap_writeback_entry(struct zswap_entry *entry,
>                                  swp_entry_t swpentry);
> -static int zswap_pool_get(struct zswap_pool *pool);
> -static void zswap_pool_put(struct zswap_pool *pool);
>
>  static bool zswap_is_full(void)
>  {
> @@ -472,6 +470,53 @@ static void zswap_pool_destroy(struct zswap_pool *pool)
>         kfree(pool);
>  }
>
> +static void __zswap_pool_release(struct work_struct *work)
> +{
> +       struct zswap_pool *pool = container_of(work, typeof(*pool),
> +                                               release_work);
> +
> +       synchronize_rcu();
> +
> +       /* nobody should have been able to get a kref... */
> +       WARN_ON(kref_get_unless_zero(&pool->kref));
> +
> +       /* pool is now off zswap_pools list and has no references. */
> +       zswap_pool_destroy(pool);
> +}
> +
> +static struct zswap_pool *zswap_pool_current(void);
> +
> +static void __zswap_pool_empty(struct kref *kref)
> +{
> +       struct zswap_pool *pool;
> +
> +       pool = container_of(kref, typeof(*pool), kref);
> +
> +       spin_lock(&zswap_pools_lock);
> +
> +       WARN_ON(pool == zswap_pool_current());
> +
> +       list_del_rcu(&pool->list);
> +
> +       INIT_WORK(&pool->release_work, __zswap_pool_release);
> +       schedule_work(&pool->release_work);
> +
> +       spin_unlock(&zswap_pools_lock);
> +}
> +
> +static int __must_check zswap_pool_get(struct zswap_pool *pool)
> +{
> +       if (!pool)
> +               return 0;
> +
> +       return kref_get_unless_zero(&pool->kref);
> +}
> +
> +static void zswap_pool_put(struct zswap_pool *pool)
> +{
> +       kref_put(&pool->kref, __zswap_pool_empty);
> +}
> +
>  /* should be called under RCU */
>  #ifdef CONFIG_MEMCG
>  static inline struct mem_cgroup *mem_cgroup_from_entry(struct zswap_entry *entry)
> @@ -1122,51 +1167,6 @@ static void shrink_worker(struct work_struct *w)
>         zswap_pool_put(pool);
>  }
>
> -static int __must_check zswap_pool_get(struct zswap_pool *pool)
> -{
> -       if (!pool)
> -               return 0;
> -
> -       return kref_get_unless_zero(&pool->kref);
> -}
> -
> -static void __zswap_pool_release(struct work_struct *work)
> -{
> -       struct zswap_pool *pool = container_of(work, typeof(*pool),
> -                                               release_work);
> -
> -       synchronize_rcu();
> -
> -       /* nobody should have been able to get a kref... */
> -       WARN_ON(kref_get_unless_zero(&pool->kref));
> -
> -       /* pool is now off zswap_pools list and has no references. */
> -       zswap_pool_destroy(pool);
> -}
> -
> -static void __zswap_pool_empty(struct kref *kref)
> -{
> -       struct zswap_pool *pool;
> -
> -       pool = container_of(kref, typeof(*pool), kref);
> -
> -       spin_lock(&zswap_pools_lock);
> -
> -       WARN_ON(pool == zswap_pool_current());
> -
> -       list_del_rcu(&pool->list);
> -
> -       INIT_WORK(&pool->release_work, __zswap_pool_release);
> -       schedule_work(&pool->release_work);
> -
> -       spin_unlock(&zswap_pools_lock);
> -}
> -
> -static void zswap_pool_put(struct zswap_pool *pool)
> -{
> -       kref_put(&pool->kref, __zswap_pool_empty);
> -}
> -
>  /*********************************
>  * param callbacks
>  **********************************/
> --
> 2.43.0
>
Johannes Weiner Jan. 31, 2024, 11:23 a.m. UTC | #2 
On Tue, Jan 30, 2024 at 12:13:30PM -0800, Nhat Pham wrote:
> On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > Move pool refcounting functions into the pool section. First the
> > destroy functions, then the get and put which uses them.
> >
> > __zswap_pool_empty() has an upward reference to the global
> > zswap_pools, to sanity check it's not the currently active pool that's
> > being freed. That gets the forward decl for zswap_pool_cuyrrent().
> 
> nit: zswap_pool_cuyrrent() -> zswap_pool_current() :-)

Whoops, my bad.

Andrew, would you mind removing that typo inside your copy?

> Also, would it make sense to move zswap_pool_current() above
> __zswap_pool_empty() to get rid of the forward declaration? I guess
> it's now grouped with current_get() etc. - those don't seem to use the
> empty check, so maybe they can also go above __zswap_pool_empty()?

There is a grouping to these functions:

- low-level functions that create and destroy individual struct zswap_pool
  (create, destroy, get, release, empty, put)
- high-level functions that operate on pool collections, i.e. zswap_pools
  (current, last, find)

They were actually already grouped like that, just in the reverse
order. The only way to avoid ALL forward decls would be to interleave
the layers, but I think the grouping makes sense so I wanted to
preserve that. I went with low to high ordering, and forward decl the
odd one where a low-level function does one high-level sanity check.

Does that make sense?
Nhat Pham Jan. 31, 2024, 11:23 p.m. UTC | #3 
On Wed, Jan 31, 2024 at 3:23 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Tue, Jan 30, 2024 at 12:13:30PM -0800, Nhat Pham wrote:
> > On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@cmpxchgorg> wrote:
> > >
> > > Move pool refcounting functions into the pool section. First the
> > > destroy functions, then the get and put which uses them.
> > >
> > > __zswap_pool_empty() has an upward reference to the global
> > > zswap_pools, to sanity check it's not the currently active pool that's
> > > being freed. That gets the forward decl for zswap_pool_cuyrrent().
> >
> > nit: zswap_pool_cuyrrent() -> zswap_pool_current() :-)
>
> Whoops, my bad.
>
> Andrew, would you mind removing that typo inside your copy?
>
> > Also, would it make sense to move zswap_pool_current() above
> > __zswap_pool_empty() to get rid of the forward declaration? I guess
> > it's now grouped with current_get() etc. - those don't seem to use the
> > empty check, so maybe they can also go above __zswap_pool_empty()?
>
> There is a grouping to these functions:
>
> - low-level functions that create and destroy individual struct zswap_pool
>   (create, destroy, get, release, empty, put)
> - high-level functions that operate on pool collections, i.e. zswap_pools
>   (current, last, find)
>
> They were actually already grouped like that, just in the reverse
> order. The only way to avoid ALL forward decls would be to interleave
> the layers, but I think the grouping makes sense so I wanted to
> preserve that. I went with low to high ordering, and forward decl the
> odd one where a low-level function does one high-level sanity check.
>
> Does that make sense?

Makes sense to me - just double checking :)
Reviewed-by: Nhat Pham <nphamcs@gmail.com>
diff mbox series

Patch

diff --git a/mm/zswap.c b/mm/zswap.c
index 805d9a35f633..33775f2224b7 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -278,8 +278,6 @@  static inline struct zswap_tree *swap_zswap_tree(swp_entry_t swp)
 
 static int zswap_writeback_entry(struct zswap_entry *entry,
 				 swp_entry_t swpentry);
-static int zswap_pool_get(struct zswap_pool *pool);
-static void zswap_pool_put(struct zswap_pool *pool);
 
 static bool zswap_is_full(void)
 {
@@ -472,6 +470,53 @@  static void zswap_pool_destroy(struct zswap_pool *pool)
 	kfree(pool);
 }
 
+static void __zswap_pool_release(struct work_struct *work)
+{
+	struct zswap_pool *pool = container_of(work, typeof(*pool),
+						release_work);
+
+	synchronize_rcu();
+
+	/* nobody should have been able to get a kref... */
+	WARN_ON(kref_get_unless_zero(&pool->kref));
+
+	/* pool is now off zswap_pools list and has no references. */
+	zswap_pool_destroy(pool);
+}
+
+static struct zswap_pool *zswap_pool_current(void);
+
+static void __zswap_pool_empty(struct kref *kref)
+{
+	struct zswap_pool *pool;
+
+	pool = container_of(kref, typeof(*pool), kref);
+
+	spin_lock(&zswap_pools_lock);
+
+	WARN_ON(pool == zswap_pool_current());
+
+	list_del_rcu(&pool->list);
+
+	INIT_WORK(&pool->release_work, __zswap_pool_release);
+	schedule_work(&pool->release_work);
+
+	spin_unlock(&zswap_pools_lock);
+}
+
+static int __must_check zswap_pool_get(struct zswap_pool *pool)
+{
+	if (!pool)
+		return 0;
+
+	return kref_get_unless_zero(&pool->kref);
+}
+
+static void zswap_pool_put(struct zswap_pool *pool)
+{
+	kref_put(&pool->kref, __zswap_pool_empty);
+}
+
 /* should be called under RCU */
 #ifdef CONFIG_MEMCG
 static inline struct mem_cgroup *mem_cgroup_from_entry(struct zswap_entry *entry)
@@ -1122,51 +1167,6 @@  static void shrink_worker(struct work_struct *w)
 	zswap_pool_put(pool);
 }
 
-static int __must_check zswap_pool_get(struct zswap_pool *pool)
-{
-	if (!pool)
-		return 0;
-
-	return kref_get_unless_zero(&pool->kref);
-}
-
-static void __zswap_pool_release(struct work_struct *work)
-{
-	struct zswap_pool *pool = container_of(work, typeof(*pool),
-						release_work);
-
-	synchronize_rcu();
-
-	/* nobody should have been able to get a kref... */
-	WARN_ON(kref_get_unless_zero(&pool->kref));
-
-	/* pool is now off zswap_pools list and has no references. */
-	zswap_pool_destroy(pool);
-}
-
-static void __zswap_pool_empty(struct kref *kref)
-{
-	struct zswap_pool *pool;
-
-	pool = container_of(kref, typeof(*pool), kref);
-
-	spin_lock(&zswap_pools_lock);
-
-	WARN_ON(pool == zswap_pool_current());
-
-	list_del_rcu(&pool->list);
-
-	INIT_WORK(&pool->release_work, __zswap_pool_release);
-	schedule_work(&pool->release_work);
-
-	spin_unlock(&zswap_pools_lock);
-}
-
-static void zswap_pool_put(struct zswap_pool *pool)
-{
-	kref_put(&pool->kref, __zswap_pool_empty);
-}
-
 /*********************************
 * param callbacks
 **********************************/