diff mbox series

[14/20] mm: zswap: function ordering: public lru api

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

Commit Message

Johannes Weiner Jan. 30, 2024, 1:36 a.m. UTC 
  The zswap entry section sits awkwardly in the middle of LRU-related
functions. Group the external LRU API functions first.

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

Comments

Nhat Pham Jan. 30, 2024, 11:47 p.m. UTC | #1 
On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> The zswap entry section sits awkwardly in the middle of LRU-related
> functions. Group the external LRU API functions first.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/zswap.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index e650fc587116..511bfafc1456 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -746,6 +746,10 @@ static int zswap_enabled_param_set(const char *val,
>         return ret;
>  }
>
> +/*********************************
> +* lru functions
> +**********************************/
> +

nit: looks like there are 2 "lru functions" headers after this patch?
You remove the "lruvec functions" header, then add another "lru
functions" header it seems. The next patch removes one of them, so end
result is fine I guess - just seems a bit odd.

That asides:
Reviewed-by: Nhat Pham <nphamcs@gmail.com>


>  /* should be called under RCU */
>  #ifdef CONFIG_MEMCG
>  static inline struct mem_cgroup *mem_cgroup_from_entry(struct zswap_entry *entry)
> @@ -764,6 +768,21 @@ static inline int entry_to_nid(struct zswap_entry *entry)
>         return page_to_nid(virt_to_page(entry));
>  }
>
> +void zswap_lruvec_state_init(struct lruvec *lruvec)
> +{
> +       atomic_long_set(&lruvec->zswap_lruvec_state.nr_zswap_protected, 0);
> +}
> +
> +void zswap_folio_swapin(struct folio *folio)
> +{
> +       struct lruvec *lruvec;
> +
> +       if (folio) {
> +               lruvec = folio_lruvec(folio);
> +               atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_protected);
> +       }
> +}
> +
>  void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg)
>  {
>         struct zswap_pool *pool;
> @@ -798,24 +817,6 @@ static void zswap_entry_cache_free(struct zswap_entry *entry)
>         kmem_cache_free(zswap_entry_cache, entry);
>  }
>
> -/*********************************
> -* zswap lruvec functions
> -**********************************/

Here's the removed zswap lruvec functions header.

> -void zswap_lruvec_state_init(struct lruvec *lruvec)
> -{
> -       atomic_long_set(&lruvec->zswap_lruvec_state.nr_zswap_protected, 0);
> -}
> -
> -void zswap_folio_swapin(struct folio *folio)
> -{
> -       struct lruvec *lruvec;
> -
> -       if (folio) {
> -               lruvec = folio_lruvec(folio);
> -               atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_protected);
> -       }
> -}
> -
>  /*********************************
>  * lru functions
>  **********************************/
> --

Here's the second (original) lru functions header.

> 2.43.0
>
Johannes Weiner Jan. 31, 2024, 11:40 a.m. UTC | #2 
On Tue, Jan 30, 2024 at 03:47:25PM -0800, Nhat Pham wrote:
> On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > The zswap entry section sits awkwardly in the middle of LRU-related
> > functions. Group the external LRU API functions first.
> >
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > ---
> >  mm/zswap.c | 37 +++++++++++++++++++------------------
> >  1 file changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index e650fc587116..511bfafc1456 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -746,6 +746,10 @@ static int zswap_enabled_param_set(const char *val,
> >         return ret;
> >  }
> >
> > +/*********************************
> > +* lru functions
> > +**********************************/
> > +
> 
> nit: looks like there are 2 "lru functions" headers after this patch?
> You remove the "lruvec functions" header, then add another "lru
> functions" header it seems. The next patch removes one of them, so end
> result is fine I guess - just seems a bit odd.

Yeah that's an artifact of trying to make git produce readable
diffs. Since the lru functions are right next to the entry functions,
I went through several attempts where it wouldn't generate clean moves
but instead would interleave entry and lru functions line by line to
overwrite one with the other in place. I think the above helped in
making it not do that, although I'm not positive it would still be
required in the final form of this patch. It was kind of brute force.

> That asides:
> Reviewed-by: Nhat Pham <nphamcs@gmail.com>

Thanks!
diff mbox series

Patch

diff --git a/mm/zswap.c b/mm/zswap.c
index e650fc587116..511bfafc1456 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -746,6 +746,10 @@  static int zswap_enabled_param_set(const char *val,
 	return ret;
 }
 
+/*********************************
+* lru functions
+**********************************/
+
 /* should be called under RCU */
 #ifdef CONFIG_MEMCG
 static inline struct mem_cgroup *mem_cgroup_from_entry(struct zswap_entry *entry)
@@ -764,6 +768,21 @@  static inline int entry_to_nid(struct zswap_entry *entry)
 	return page_to_nid(virt_to_page(entry));
 }
 
+void zswap_lruvec_state_init(struct lruvec *lruvec)
+{
+	atomic_long_set(&lruvec->zswap_lruvec_state.nr_zswap_protected, 0);
+}
+
+void zswap_folio_swapin(struct folio *folio)
+{
+	struct lruvec *lruvec;
+
+	if (folio) {
+		lruvec = folio_lruvec(folio);
+		atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_protected);
+	}
+}
+
 void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg)
 {
 	struct zswap_pool *pool;
@@ -798,24 +817,6 @@  static void zswap_entry_cache_free(struct zswap_entry *entry)
 	kmem_cache_free(zswap_entry_cache, entry);
 }
 
-/*********************************
-* zswap lruvec functions
-**********************************/
-void zswap_lruvec_state_init(struct lruvec *lruvec)
-{
-	atomic_long_set(&lruvec->zswap_lruvec_state.nr_zswap_protected, 0);
-}
-
-void zswap_folio_swapin(struct folio *folio)
-{
-	struct lruvec *lruvec;
-
-	if (folio) {
-		lruvec = folio_lruvec(folio);
-		atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_protected);
-	}
-}
-
 /*********************************
 * lru functions
 **********************************/