In preparation for computing recently evicted pages in cachestat,
refactor workingset_refault and lru_gen_refault to expose a helper
function that would test if an evicted page is recently evicted.
Signed-off-by: Nhat Pham <[email protected]>
---
include/linux/swap.h | 1 +
mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
2 files changed, 99 insertions(+), 45 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index a18cf4b7c724..dae6f6f955eb 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
}
/* linux/mm/workingset.c */
+bool workingset_test_recent(void *shadow, bool file, bool *workingset);
void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
void workingset_refault(struct folio *folio, void *shadow);
diff --git a/mm/workingset.c b/mm/workingset.c
index 79585d55c45d..44b331ce3040 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
}
+/*
+ * Test if the folio is recently evicted.
+ *
+ * As a side effect, also populates the references with
+ * values unpacked from the shadow of the evicted folio.
+ */
+static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
+ struct pglist_data **pgdat, unsigned long *token, bool *workingset)
+{
+ struct mem_cgroup *eviction_memcg;
+ struct lruvec *lruvec;
+ struct lru_gen_struct *lrugen;
+ unsigned long min_seq;
+
+ unpack_shadow(shadow, memcgid, pgdat, token, workingset);
+ eviction_memcg = mem_cgroup_from_id(*memcgid);
+
+ lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
+ lrugen = &lruvec->lrugen;
+
+ min_seq = READ_ONCE(lrugen->min_seq[file]);
+ return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
+}
+
static void lru_gen_refault(struct folio *folio, void *shadow)
{
int hist, tier, refs;
@@ -258,23 +282,24 @@ static void lru_gen_refault(struct folio *folio, void *shadow)
int type = folio_is_file_lru(folio);
int delta = folio_nr_pages(folio);
- unpack_shadow(shadow, &memcg_id, &pgdat, &token, &workingset);
-
- if (pgdat != folio_pgdat(folio))
- return;
rcu_read_lock();
memcg = folio_memcg_rcu(folio);
+
+ if (!lru_gen_test_recent(shadow, type, &memcg_id, &pgdat, &token,
+ &workingset))
+ goto unlock;
+
if (memcg_id != mem_cgroup_id(memcg))
goto unlock;
+ if (pgdat != folio_pgdat(folio))
+ goto unlock;
+
lruvec = mem_cgroup_lruvec(memcg, pgdat);
lrugen = &lruvec->lrugen;
-
min_seq = READ_ONCE(lrugen->min_seq[type]);
- if ((token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)))
- goto unlock;
hist = lru_hist_from_seq(min_seq);
/* see the comment in folio_lru_refs() */
@@ -306,6 +331,12 @@ static void *lru_gen_eviction(struct folio *folio)
return NULL;
}
+static bool lru_gen_test_recent(void *shadow, int type, int *memcgid,
+ struct pglist_data **pgdat, unsigned long *token, bool *workingset)
+{
+ return true;
+}
+
static void lru_gen_refault(struct folio *folio, void *shadow)
{
}
@@ -373,40 +404,32 @@ void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg)
folio_test_workingset(folio));
}
-/**
- * workingset_refault - Evaluate the refault of a previously evicted folio.
- * @folio: The freshly allocated replacement folio.
- * @shadow: Shadow entry of the evicted folio.
+/*
+ * Test if the folio is recently evicted by checking if
+ * refault distance of shadow exceeds workingset size.
*
- * Calculates and evaluates the refault distance of the previously
- * evicted folio in the context of the node and the memcg whose memory
- * pressure caused the eviction.
+ * As a side effect, populate workingset with the value
+ * unpacked from shadow.
*/
-void workingset_refault(struct folio *folio, void *shadow)
+bool workingset_test_recent(void *shadow, bool file, bool *workingset)
{
- bool file = folio_is_file_lru(folio);
struct mem_cgroup *eviction_memcg;
struct lruvec *eviction_lruvec;
unsigned long refault_distance;
unsigned long workingset_size;
- struct pglist_data *pgdat;
- struct mem_cgroup *memcg;
- unsigned long eviction;
- struct lruvec *lruvec;
unsigned long refault;
- bool workingset;
+
int memcgid;
- long nr;
+ struct pglist_data *pgdat;
+ unsigned long eviction;
- if (lru_gen_enabled()) {
- lru_gen_refault(folio, shadow);
- return;
- }
+ if (lru_gen_enabled())
+ return lru_gen_test_recent(shadow, file, &memcgid,
+ &pgdat, &eviction, workingset);
- unpack_shadow(shadow, &memcgid, &pgdat, &eviction, &workingset);
+ unpack_shadow(shadow, &memcgid, &pgdat, &eviction, workingset);
eviction <<= bucket_order;
- rcu_read_lock();
/*
* Look up the memcg associated with the stored ID. It might
* have been deleted since the folio's eviction.
@@ -425,7 +448,8 @@ void workingset_refault(struct folio *folio, void *shadow)
*/
eviction_memcg = mem_cgroup_from_id(memcgid);
if (!mem_cgroup_disabled() && !eviction_memcg)
- goto out;
+ return false;
+
eviction_lruvec = mem_cgroup_lruvec(eviction_memcg, pgdat);
refault = atomic_long_read(&eviction_lruvec->nonresident_age);
@@ -447,21 +471,6 @@ void workingset_refault(struct folio *folio, void *shadow)
*/
refault_distance = (refault - eviction) & EVICTION_MASK;
- /*
- * The activation decision for this folio is made at the level
- * where the eviction occurred, as that is where the LRU order
- * during folio reclaim is being determined.
- *
- * However, the cgroup that will own the folio is the one that
- * is actually experiencing the refault event.
- */
- nr = folio_nr_pages(folio);
- memcg = folio_memcg(folio);
- pgdat = folio_pgdat(folio);
- lruvec = mem_cgroup_lruvec(memcg, pgdat);
-
- mod_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file, nr);
-
mem_cgroup_flush_stats_delayed();
/*
* Compare the distance to the existing workingset size. We
@@ -483,8 +492,51 @@ void workingset_refault(struct folio *folio, void *shadow)
NR_INACTIVE_ANON);
}
}
- if (refault_distance > workingset_size)
+
+ return refault_distance <= workingset_size;
+}
+
+/**
+ * workingset_refault - Evaluate the refault of a previously evicted folio.
+ * @folio: The freshly allocated replacement folio.
+ * @shadow: Shadow entry of the evicted folio.
+ *
+ * Calculates and evaluates the refault distance of the previously
+ * evicted folio in the context of the node and the memcg whose memory
+ * pressure caused the eviction.
+ */
+void workingset_refault(struct folio *folio, void *shadow)
+{
+ bool file = folio_is_file_lru(folio);
+ struct pglist_data *pgdat;
+ struct mem_cgroup *memcg;
+ struct lruvec *lruvec;
+ bool workingset;
+ long nr;
+
+ if (lru_gen_enabled()) {
+ lru_gen_refault(folio, shadow);
+ return;
+ }
+
+ rcu_read_lock();
+
+ nr = folio_nr_pages(folio);
+ memcg = folio_memcg(folio);
+ pgdat = folio_pgdat(folio);
+ lruvec = mem_cgroup_lruvec(memcg, pgdat);
+
+ if (!workingset_test_recent(shadow, file, &workingset)) {
+ /*
+ * The activation decision for this folio is made at the level
+ * where the eviction occurred, as that is where the LRU order
+ * during folio reclaim is being determined.
+ *
+ * However, the cgroup that will own the folio is the one that
+ * is actually experiencing the refault event.
+ */
goto out;
+ }
folio_set_active(folio);
workingset_age_nonresident(lruvec, nr);
@@ -498,6 +550,7 @@ void workingset_refault(struct folio *folio, void *shadow)
mod_lruvec_state(lruvec, WORKINGSET_RESTORE_BASE + file, nr);
}
out:
+ mod_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file, nr);
rcu_read_unlock();
}
--
2.30.2
On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <[email protected]> wrote:
>
> In preparation for computing recently evicted pages in cachestat,
> refactor workingset_refault and lru_gen_refault to expose a helper
> function that would test if an evicted page is recently evicted.
>
> Signed-off-by: Nhat Pham <[email protected]>
> ---
> include/linux/swap.h | 1 +
> mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
> 2 files changed, 99 insertions(+), 45 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index a18cf4b7c724..dae6f6f955eb 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> }
>
> /* linux/mm/workingset.c */
> +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> void workingset_refault(struct folio *folio, void *shadow);
> diff --git a/mm/workingset.c b/mm/workingset.c
> index 79585d55c45d..44b331ce3040 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> }
>
> +/*
> + * Test if the folio is recently evicted.
> + *
> + * As a side effect, also populates the references with
> + * values unpacked from the shadow of the evicted folio.
> + */
> +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> +{
> + struct mem_cgroup *eviction_memcg;
> + struct lruvec *lruvec;
> + struct lru_gen_struct *lrugen;
> + unsigned long min_seq;
> +
> + unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> + eviction_memcg = mem_cgroup_from_id(*memcgid);
> +
> + lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> + lrugen = &lruvec->lrugen;
> +
> + min_seq = READ_ONCE(lrugen->min_seq[file]);
> + return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> +}
Nit: not refactoring actually looks cleaner to me -- there are only a
few lines of duplicated code and you can get rid of 4 parameters
including the unused workingset in the next patch.
On Mon, Dec 5, 2022 at 3:49 PM Yu Zhao <[email protected]> wrote:
>
> On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <[email protected]> wrote:
> >
> > In preparation for computing recently evicted pages in cachestat,
> > refactor workingset_refault and lru_gen_refault to expose a helper
> > function that would test if an evicted page is recently evicted.
> >
> > Signed-off-by: Nhat Pham <[email protected]>
> > ---
> > include/linux/swap.h | 1 +
> > mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
> > 2 files changed, 99 insertions(+), 45 deletions(-)
> >
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index a18cf4b7c724..dae6f6f955eb 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> > }
> >
> > /* linux/mm/workingset.c */
> > +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> > void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> > void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> > void workingset_refault(struct folio *folio, void *shadow);
> > diff --git a/mm/workingset.c b/mm/workingset.c
> > index 79585d55c45d..44b331ce3040 100644
> > --- a/mm/workingset.c
> > +++ b/mm/workingset.c
> > @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> > return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> > }
> >
> > +/*
> > + * Test if the folio is recently evicted.
> > + *
> > + * As a side effect, also populates the references with
> > + * values unpacked from the shadow of the evicted folio.
> > + */
> > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> > +{
> > + struct mem_cgroup *eviction_memcg;
> > + struct lruvec *lruvec;
> > + struct lru_gen_struct *lrugen;
> > + unsigned long min_seq;
> > +
> > + unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> > + eviction_memcg = mem_cgroup_from_id(*memcgid);
> > +
> > + lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> > + lrugen = &lruvec->lrugen;
> > +
> > + min_seq = READ_ONCE(lrugen->min_seq[file]);
> > + return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> > +}
>
> Nit: not refactoring actually looks cleaner to me -- there are only a
> few lines of duplicated code and you can get rid of 4 parameters
> including the unused workingset in the next patch.
(resending this because I forgot to forward to the rest of the
group!)
Thanks for the comment, Yu!
Personally, I prefer refactoring this piece of logic - I do think that
it is cleaner than copying the logic into the syscall implementation.
I believe that if I don't do the refactoring, I'll have to repeat the
unused parameters in the syscall, and make unpack_shadow
a non-static function (along with all the locally defined macros like
WORKINGSET_SHIFT). I think it would get quite messy there too.
But more importantly, I'm a bit concerned that the logic for
determining the recency of the eviction might change in the
future, which would break the cachestat syscall unknowingly...
Let me know what you think about this, Yu!
On Mon, Dec 5, 2022 at 6:19 PM Nhat Pham <[email protected]> wrote:
>
> On Mon, Dec 5, 2022 at 3:49 PM Yu Zhao <[email protected]> wrote:
> >
> > On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <[email protected]> wrote:
> > >
> > > In preparation for computing recently evicted pages in cachestat,
> > > refactor workingset_refault and lru_gen_refault to expose a helper
> > > function that would test if an evicted page is recently evicted.
> > >
> > > Signed-off-by: Nhat Pham <[email protected]>
> > > ---
> > > include/linux/swap.h | 1 +
> > > mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
> > > 2 files changed, 99 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > index a18cf4b7c724..dae6f6f955eb 100644
> > > --- a/include/linux/swap.h
> > > +++ b/include/linux/swap.h
> > > @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> > > }
> > >
> > > /* linux/mm/workingset.c */
> > > +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> > > void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> > > void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> > > void workingset_refault(struct folio *folio, void *shadow);
> > > diff --git a/mm/workingset.c b/mm/workingset.c
> > > index 79585d55c45d..44b331ce3040 100644
> > > --- a/mm/workingset.c
> > > +++ b/mm/workingset.c
> > > @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> > > return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> > > }
> > >
> > > +/*
> > > + * Test if the folio is recently evicted.
> > > + *
> > > + * As a side effect, also populates the references with
> > > + * values unpacked from the shadow of the evicted folio.
> > > + */
> > > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > > + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> > > +{
> > > + struct mem_cgroup *eviction_memcg;
> > > + struct lruvec *lruvec;
> > > + struct lru_gen_struct *lrugen;
> > > + unsigned long min_seq;
> > > +
> > > + unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> > > + eviction_memcg = mem_cgroup_from_id(*memcgid);
> > > +
> > > + lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> > > + lrugen = &lruvec->lrugen;
> > > +
> > > + min_seq = READ_ONCE(lrugen->min_seq[file]);
> > > + return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> > > +}
> >
> > Nit: not refactoring actually looks cleaner to me -- there are only a
> > few lines of duplicated code and you can get rid of 4 parameters
> > including the unused workingset in the next patch.
>
> (resending this because I forgot to forward to the rest of the
> group!)
>
> Thanks for the comment, Yu!
>
> Personally, I prefer refactoring this piece of logic - I do think that
> it is cleaner than copying the logic into the syscall implementation.
Let me clarify.
You can add
lru_gen_test_recent(void *shadow, bool file)
without refactoring the existing
lru_gen_refault().
Set the boilerplate aside, you only repeat one line of code, which is
the last line in the new function.
(The boilerplate code is repeated in many places, and that's why it's
called boilerplate.)
> I believe that if I don't do the refactoring, I'll have to repeat the
> unused parameters in the syscall, and make unpack_shadow
> a non-static function (along with all the locally defined macros like
> WORKINGSET_SHIFT). I think it would get quite messy there too.
>
> But more importantly, I'm a bit concerned that the logic for
> determining the recency of the eviction might change in the
> future, which would break the cachestat syscall unknowingly...
>
> Let me know what you think about this, Yu!
Your call :)
On Mon, Dec 5, 2022 at 5:29 PM Yu Zhao <[email protected]> wrote:
>
> On Mon, Dec 5, 2022 at 6:19 PM Nhat Pham <[email protected]> wrote:
> >
> > On Mon, Dec 5, 2022 at 3:49 PM Yu Zhao <[email protected]> wrote:
> > >
> > > On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <[email protected]> wrote:
> > > >
> > > > In preparation for computing recently evicted pages in cachestat,
> > > > refactor workingset_refault and lru_gen_refault to expose a helper
> > > > function that would test if an evicted page is recently evicted.
> > > >
> > > > Signed-off-by: Nhat Pham <[email protected]>
> > > > ---
> > > > include/linux/swap.h | 1 +
> > > > mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
> > > > 2 files changed, 99 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > > index a18cf4b7c724..dae6f6f955eb 100644
> > > > --- a/include/linux/swap.h
> > > > +++ b/include/linux/swap.h
> > > > @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> > > > }
> > > >
> > > > /* linux/mm/workingset.c */
> > > > +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> > > > void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> > > > void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> > > > void workingset_refault(struct folio *folio, void *shadow);
> > > > diff --git a/mm/workingset.c b/mm/workingset.c
> > > > index 79585d55c45d..44b331ce3040 100644
> > > > --- a/mm/workingset.c
> > > > +++ b/mm/workingset.c
> > > > @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> > > > return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> > > > }
> > > >
> > > > +/*
> > > > + * Test if the folio is recently evicted.
> > > > + *
> > > > + * As a side effect, also populates the references with
> > > > + * values unpacked from the shadow of the evicted folio.
> > > > + */
> > > > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > > > + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> > > > +{
> > > > + struct mem_cgroup *eviction_memcg;
> > > > + struct lruvec *lruvec;
> > > > + struct lru_gen_struct *lrugen;
> > > > + unsigned long min_seq;
> > > > +
> > > > + unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> > > > + eviction_memcg = mem_cgroup_from_id(*memcgid);
> > > > +
> > > > + lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> > > > + lrugen = &lruvec->lrugen;
> > > > +
> > > > + min_seq = READ_ONCE(lrugen->min_seq[file]);
> > > > + return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> > > > +}
> > >
> > > Nit: not refactoring actually looks cleaner to me -- there are only a
> > > few lines of duplicated code and you can get rid of 4 parameters
> > > including the unused workingset in the next patch.
> >
> > (resending this because I forgot to forward to the rest of the
> > group!)
> >
> > Thanks for the comment, Yu!
> >
> > Personally, I prefer refactoring this piece of logic - I do think that
> > it is cleaner than copying the logic into the syscall implementation.
>
> Let me clarify.
>
> You can add
> lru_gen_test_recent(void *shadow, bool file)
> without refactoring the existing
> lru_gen_refault().
>
> Set the boilerplate aside, you only repeat one line of code, which is
> the last line in the new function.
>
> (The boilerplate code is repeated in many places, and that's why it's
> called boilerplate.)
Oh maybe I've misunderstood your original comment. Your
suggestion is to repeat the eviction recency check in workingset.c
(i.e keep the *_gen_refault unchanged - just copy that logic to
*_test_recent)?
On Mon, Dec 5, 2022 at 7:22 PM Nhat Pham <[email protected]> wrote:
>
> On Mon, Dec 5, 2022 at 5:29 PM Yu Zhao <[email protected]> wrote:
> >
> > On Mon, Dec 5, 2022 at 6:19 PM Nhat Pham <[email protected]> wrote:
> > >
> > > On Mon, Dec 5, 2022 at 3:49 PM Yu Zhao <[email protected]> wrote:
> > > >
> > > > On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <[email protected]> wrote:
> > > > >
> > > > > In preparation for computing recently evicted pages in cachestat,
> > > > > refactor workingset_refault and lru_gen_refault to expose a helper
> > > > > function that would test if an evicted page is recently evicted.
> > > > >
> > > > > Signed-off-by: Nhat Pham <[email protected]>
> > > > > ---
> > > > > include/linux/swap.h | 1 +
> > > > > mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
> > > > > 2 files changed, 99 insertions(+), 45 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > > > index a18cf4b7c724..dae6f6f955eb 100644
> > > > > --- a/include/linux/swap.h
> > > > > +++ b/include/linux/swap.h
> > > > > @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> > > > > }
> > > > >
> > > > > /* linux/mm/workingset.c */
> > > > > +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> > > > > void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> > > > > void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> > > > > void workingset_refault(struct folio *folio, void *shadow);
> > > > > diff --git a/mm/workingset.c b/mm/workingset.c
> > > > > index 79585d55c45d..44b331ce3040 100644
> > > > > --- a/mm/workingset.c
> > > > > +++ b/mm/workingset.c
> > > > > @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> > > > > return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * Test if the folio is recently evicted.
> > > > > + *
> > > > > + * As a side effect, also populates the references with
> > > > > + * values unpacked from the shadow of the evicted folio.
> > > > > + */
> > > > > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > > > > + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> > > > > +{
> > > > > + struct mem_cgroup *eviction_memcg;
> > > > > + struct lruvec *lruvec;
> > > > > + struct lru_gen_struct *lrugen;
> > > > > + unsigned long min_seq;
> > > > > +
> > > > > + unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> > > > > + eviction_memcg = mem_cgroup_from_id(*memcgid);
> > > > > +
> > > > > + lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> > > > > + lrugen = &lruvec->lrugen;
> > > > > +
> > > > > + min_seq = READ_ONCE(lrugen->min_seq[file]);
> > > > > + return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> > > > > +}
> > > >
> > > > Nit: not refactoring actually looks cleaner to me -- there are only a
> > > > few lines of duplicated code and you can get rid of 4 parameters
> > > > including the unused workingset in the next patch.
> > >
> > > (resending this because I forgot to forward to the rest of the
> > > group!)
> > >
> > > Thanks for the comment, Yu!
> > >
> > > Personally, I prefer refactoring this piece of logic - I do think that
> > > it is cleaner than copying the logic into the syscall implementation.
> >
> > Let me clarify.
> >
> > You can add
> > lru_gen_test_recent(void *shadow, bool file)
> > without refactoring the existing
> > lru_gen_refault().
> >
> > Set the boilerplate aside, you only repeat one line of code, which is
> > the last line in the new function.
> >
> > (The boilerplate code is repeated in many places, and that's why it's
> > called boilerplate.)
>
> Oh maybe I've misunderstood your original comment. Your
> suggestion is to repeat the eviction recency check in workingset.c
> (i.e keep the *_gen_refault unchanged - just copy that logic to
> *_test_recent)?
Correct.
On Mon, Dec 05, 2022 at 09:51:38AM -0800, Nhat Pham wrote:
> +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
This line needs more than one tab indent. I tend to use two tabs,
others try to line up the parameters with the opening '('. But
one tab is too visually similar to the body of the function.
> rcu_read_lock();
>
> memcg = folio_memcg_rcu(folio);
> +
> + if (!lru_gen_test_recent(shadow, type, &memcg_id, &pgdat, &token,
> + &workingset))
Similarly here. &workingset looks like it's part of the body of the if,
not part of the function call.
> + goto unlock;
> +
> if (memcg_id != mem_cgroup_id(memcg))
> goto unlock;
>
On Tue, Dec 6, 2022 at 7:22 AM Matthew Wilcox <[email protected]> wrote:
>
> On Mon, Dec 05, 2022 at 09:51:38AM -0800, Nhat Pham wrote:
> > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
>
> This line needs more than one tab indent. I tend to use two tabs,
> others try to line up the parameters with the opening '('. But
> one tab is too visually similar to the body of the function.
>
> > rcu_read_lock();
> >
> > memcg = folio_memcg_rcu(folio);
> > +
> > + if (!lru_gen_test_recent(shadow, type, &memcg_id, &pgdat, &token,
> > + &workingset))
>
> Similarly here. &workingset looks like it's part of the body of the if,
> not part of the function call.
>
> > + goto unlock;
> > +
> > if (memcg_id != mem_cgroup_id(memcg))
> > goto unlock;
> >
That's fair - I'll add another tab to each of these lines! Let me
know if you have any other concerns.
On Mon, Dec 5, 2022 at 6:26 PM Yu Zhao <[email protected]> wrote:
>
> On Mon, Dec 5, 2022 at 7:22 PM Nhat Pham <[email protected]> wrote:
> >
> > On Mon, Dec 5, 2022 at 5:29 PM Yu Zhao <[email protected]> wrote:
> > >
> > > On Mon, Dec 5, 2022 at 6:19 PM Nhat Pham <[email protected]> wrote:
> > > >
> > > > On Mon, Dec 5, 2022 at 3:49 PM Yu Zhao <[email protected]> wrote:
> > > > >
> > > > > On Mon, Dec 5, 2022 at 10:51 AM Nhat Pham <[email protected]> wrote:
> > > > > >
> > > > > > In preparation for computing recently evicted pages in cachestat,
> > > > > > refactor workingset_refault and lru_gen_refault to expose a helper
> > > > > > function that would test if an evicted page is recently evicted.
> > > > > >
> > > > > > Signed-off-by: Nhat Pham <[email protected]>
> > > > > > ---
> > > > > > include/linux/swap.h | 1 +
> > > > > > mm/workingset.c | 143 +++++++++++++++++++++++++++++--------------
> > > > > > 2 files changed, 99 insertions(+), 45 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > > > > index a18cf4b7c724..dae6f6f955eb 100644
> > > > > > --- a/include/linux/swap.h
> > > > > > +++ b/include/linux/swap.h
> > > > > > @@ -361,6 +361,7 @@ static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> > > > > > }
> > > > > >
> > > > > > /* linux/mm/workingset.c */
> > > > > > +bool workingset_test_recent(void *shadow, bool file, bool *workingset);
> > > > > > void workingset_age_nonresident(struct lruvec *lruvec, unsigned long nr_pages);
> > > > > > void *workingset_eviction(struct folio *folio, struct mem_cgroup *target_memcg);
> > > > > > void workingset_refault(struct folio *folio, void *shadow);
> > > > > > diff --git a/mm/workingset.c b/mm/workingset.c
> > > > > > index 79585d55c45d..44b331ce3040 100644
> > > > > > --- a/mm/workingset.c
> > > > > > +++ b/mm/workingset.c
> > > > > > @@ -244,6 +244,30 @@ static void *lru_gen_eviction(struct folio *folio)
> > > > > > return pack_shadow(mem_cgroup_id(memcg), pgdat, token, refs);
> > > > > > }
> > > > > >
> > > > > > +/*
> > > > > > + * Test if the folio is recently evicted.
> > > > > > + *
> > > > > > + * As a side effect, also populates the references with
> > > > > > + * values unpacked from the shadow of the evicted folio.
> > > > > > + */
> > > > > > +static bool lru_gen_test_recent(void *shadow, bool file, int *memcgid,
> > > > > > + struct pglist_data **pgdat, unsigned long *token, bool *workingset)
> > > > > > +{
> > > > > > + struct mem_cgroup *eviction_memcg;
> > > > > > + struct lruvec *lruvec;
> > > > > > + struct lru_gen_struct *lrugen;
> > > > > > + unsigned long min_seq;
> > > > > > +
> > > > > > + unpack_shadow(shadow, memcgid, pgdat, token, workingset);
> > > > > > + eviction_memcg = mem_cgroup_from_id(*memcgid);
> > > > > > +
> > > > > > + lruvec = mem_cgroup_lruvec(eviction_memcg, *pgdat);
> > > > > > + lrugen = &lruvec->lrugen;
> > > > > > +
> > > > > > + min_seq = READ_ONCE(lrugen->min_seq[file]);
> > > > > > + return !((*token >> LRU_REFS_WIDTH) != (min_seq & (EVICTION_MASK >> LRU_REFS_WIDTH)));
> > > > > > +}
> > > > >
> > > > > Nit: not refactoring actually looks cleaner to me -- there are only a
> > > > > few lines of duplicated code and you can get rid of 4 parameters
> > > > > including the unused workingset in the next patch.
> > > >
> > > > (resending this because I forgot to forward to the rest of the
> > > > group!)
> > > >
> > > > Thanks for the comment, Yu!
> > > >
> > > > Personally, I prefer refactoring this piece of logic - I do think that
> > > > it is cleaner than copying the logic into the syscall implementation.
> > >
> > > Let me clarify.
> > >
> > > You can add
> > > lru_gen_test_recent(void *shadow, bool file)
> > > without refactoring the existing
> > > lru_gen_refault().
> > >
> > > Set the boilerplate aside, you only repeat one line of code, which is
> > > the last line in the new function.
> > >
> > > (The boilerplate code is repeated in many places, and that's why it's
> > > called boilerplate.)
> >
> > Oh maybe I've misunderstood your original comment. Your
> > suggestion is to repeat the eviction recency check in workingset.c
> > (i.e keep the *_gen_refault unchanged - just copy that logic to
> > *_test_recent)?
>
> Correct.
Thanks for the suggestion, Yu! I'll update if there are complications with
this, but otherwise the change will be in v3 of this patch series.