[RFC] zswap: add writeback_time_threshold interface to shrink zswap pool

  zswap does not have a suitable method to select objects that have not
been accessed for a long time, and just shrink the pool when the limit
is hit. There is a high probability of wasting memory in zswap if the
limit is too high.

This patch add a new interface writeback_time_threshold to shrink zswap
pool proactively based on the time threshold in second, e.g.::

echo 600 > /sys/module/zswap/parameters/writeback_time_threshold

If zswap_entrys have not been accessed for more than 600 seconds, they
will be swapout to swap. if set to 0, all of them will be swapout.

Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
---
 Documentation/admin-guide/mm/zswap.rst |  9 +++
 mm/zswap.c                             | 76 ++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)
  

On Tue, Oct 10, 2023 at 10:11 PM Zhongkun He
<hezhongkun.hzk@bytedance.com> wrote:
>
> zswap does not have a suitable method to select objects that have not
> been accessed for a long time, and just shrink the pool when the limit
> is hit. There is a high probability of wasting memory in zswap if the
> limit is too high.

We're currently trying to solve this exact problem. Our approach is to
add a shrinker that automatically shrinks the size of the zswap pool:

https://lore.kernel.org/lkml/20230919171447.2712746-1-nphamcs@gmail.com/

It is triggered on memory-pressure, and can perform reclaim in a
workload-specific manner.

I'm currently working on v3 of this patch series, but in the meantime,
could you take a look and see if it will address your issues as well?

Comments and suggestions are always welcome, of course :)

>
> This patch add a new interface writeback_time_threshold to shrink zswap
> pool proactively based on the time threshold in second, e.g.::
>
> echo 600 > /sys/module/zswap/parameters/writeback_time_threshold

My concern with this approach is that this value seems rather arbitrary.
I imagine that it is workload- and memory access pattern- dependent,
and will have to be tuned. Other than a couple of big users, no one
will have the resources to do this.

And since this is a one-off knob, there's another parameter users
will have to decide - frequency, i.e how often should the userspace
agent trigger this reclaim action. This is again very hard to determine
a priori, and most likely has to be tuned as well.

>
> If zswap_entrys have not been accessed for more than 600 seconds, they
> will be swapout to swap. if set to 0, all of them will be swapout.
>
> Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
> ---
>  Documentation/admin-guide/mm/zswap.rst |  9 +++
>  mm/zswap.c                             | 76 ++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> index 45b98390e938..9ffaed26c3c0 100644
> --- a/Documentation/admin-guide/mm/zswap.rst
> +++ b/Documentation/admin-guide/mm/zswap.rst
> @@ -153,6 +153,15 @@ attribute, e. g.::
>
>  Setting this parameter to 100 will disable the hysteresis.
>
> +When there is a lot of cold memory according to the store time in the zswap,
> +it can be swapout and save memory in userspace proactively. User can write
> +writeback time threshold in second to enable it, e.g.::
> +
> +  echo 600 > /sys/module/zswap/parameters/writeback_time_threshold
> +
> +If zswap_entrys have not been accessed for more than 600 seconds, they will be
> +swapout. if set to 0, all of them will be swapout.
> +
>  A debugfs interface is provided for various statistic about pool size, number
>  of pages stored, same-value filled pages and various counters for the reasons
>  pages are rejected.
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 083c693602b8..c3a19b56a29b 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -141,6 +141,16 @@ static bool zswap_exclusive_loads_enabled = IS_ENABLED(
>                 CONFIG_ZSWAP_EXCLUSIVE_LOADS_DEFAULT_ON);
>  module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644);
>
> +/* zswap writeback time threshold in second */
> +static unsigned int  zswap_writeback_time_thr;
> +static int zswap_writeback_time_thr_param_set(const char *, const struct kernel_param *);
> +static const struct kernel_param_ops zswap_writeback_param_ops = {
> +       .set =          zswap_writeback_time_thr_param_set,
> +       .get =          param_get_uint,
> +};
> +module_param_cb(writeback_time_threshold, &zswap_writeback_param_ops,
> +                       &zswap_writeback_time_thr, 0644);
> +
>  /* Number of zpools in zswap_pool (empirically determined for scalability) */
>  #define ZSWAP_NR_ZPOOLS 32
>
> @@ -197,6 +207,7 @@ struct zswap_pool {
>   * value - value of the same-value filled pages which have same content
>   * objcg - the obj_cgroup that the compressed memory is charged to
>   * lru - handle to the pool's lru used to evict pages.
> + * sto_time - the store time of zswap_entry.
>   */
>  struct zswap_entry {
>         struct rb_node rbnode;
> @@ -210,6 +221,7 @@ struct zswap_entry {
>         };
>         struct obj_cgroup *objcg;
>         struct list_head lru;
> +       ktime_t sto_time;
>  };
>
>  /*
> @@ -288,6 +300,31 @@ static void zswap_update_total_size(void)
>         zswap_pool_total_size = total;
>  }
>
> +static void zswap_reclaim_entry_by_timethr(void);
> +
> +static bool zswap_reach_timethr(struct zswap_pool *pool)
> +{
> +       struct zswap_entry *entry;
> +       ktime_t expire_time = 0;
> +       bool ret = false;
> +
> +       spin_lock(&pool->lru_lock);
> +
> +       if (list_empty(&pool->lru))
> +               goto out;
> +
> +       entry = list_last_entry(&pool->lru, struct zswap_entry, lru);
> +       expire_time = ktime_add(entry->sto_time,
> +                       ns_to_ktime(zswap_writeback_time_thr * NSEC_PER_SEC));
> +
> +       if (ktime_after(ktime_get_boottime(), expire_time))
> +               ret = true;
> +out:
> +       spin_unlock(&pool->lru_lock);
> +       return ret;
> +}
> +
> +
>  /*********************************
>  * zswap entry functions
>  **********************************/
> @@ -395,6 +432,7 @@ static void zswap_free_entry(struct zswap_entry *entry)
>         else {
>                 spin_lock(&entry->pool->lru_lock);
>                 list_del(&entry->lru);
> +               entry->sto_time = 0;
>                 spin_unlock(&entry->pool->lru_lock);
>                 zpool_free(zswap_find_zpool(entry), entry->handle);
>                 zswap_pool_put(entry->pool);
> @@ -709,6 +747,28 @@ static void shrink_worker(struct work_struct *w)
>         zswap_pool_put(pool);
>  }
>
> +static void zswap_reclaim_entry_by_timethr(void)
> +{
> +       struct zswap_pool *pool = zswap_pool_current_get();
> +       int ret, failures = 0;
> +
> +       if (!pool)
> +               return;
> +
> +       while (zswap_reach_timethr(pool)) {
> +               ret = zswap_reclaim_entry(pool);
> +               if (ret) {
> +                       zswap_reject_reclaim_fail++;
> +                       if (ret != -EAGAIN)
> +                               break;
> +                       if (++failures == MAX_RECLAIM_RETRIES)
> +                               break;
> +               }
> +               cond_resched();
> +       }
> +       zswap_pool_put(pool);
> +}
> +
>  static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  {
>         int i;
> @@ -1037,6 +1097,21 @@ static int zswap_enabled_param_set(const char *val,
>         return ret;
>  }
>
> +static int zswap_writeback_time_thr_param_set(const char *val,
> +                               const struct kernel_param *kp)
> +{
> +       int ret = -ENODEV;
> +
> +       /* if this is load-time (pre-init) param setting, just return. */
> +       if (system_state != SYSTEM_RUNNING)
> +               return ret;
> +
> +       ret = param_set_uint(val, kp);
> +       if (!ret)
> +               zswap_reclaim_entry_by_timethr();
> +       return ret;
> +}
> +
>  /*********************************
>  * writeback code
>  **********************************/
> @@ -1360,6 +1435,7 @@ bool zswap_store(struct folio *folio)
>         if (entry->length) {
>                 spin_lock(&entry->pool->lru_lock);
>                 list_add(&entry->lru, &entry->pool->lru);
> +               entry->sto_time = ktime_get_boottime();
>                 spin_unlock(&entry->pool->lru_lock);
>         }

I think there might be some issues with just storing the store time here
as well. IIUC, there might be cases where the zswap entry
is accessed and brought into memory, but that entry (with the associated
compressed memory) still hangs around. For e.g and more context,
see this patch that enables exclusive loads:

https://lore.kernel.org/lkml/20230607195143.1473802-1-yosryahmed@google.com/

If that happens, this sto_time field does not tell the full story, right?
For instance, if an object is stored a long time ago, but has been
accessed since, it shouldn't be considered a cold object that should be
a candidate for reclaim. But the old sto_time would indicate otherwise.

>         spin_unlock(&tree->lock);
> --
> 2.25.1
>
  

Hi Nhat, thanks for your detailed reply.

> We're currently trying to solve this exact problem. Our approach is to
> add a shrinker that automatically shrinks the size of the zswap pool:
>
> https://lore.kernel.org/lkml/20230919171447.2712746-1-nphamcs@gmail.com/
>
> It is triggered on memory-pressure, and can perform reclaim in a
> workload-specific manner.
>
> I'm currently working on v3 of this patch series, but in the meantime,
> could you take a look and see if it will address your issues as well?
>
> Comments and suggestions are always welcome, of course :)
>

Thanks, I've seen both patches. But we hope to be able to reclaim memory
in advance, regardless of memory pressure, like memory.reclaim in memcg,
so we can offload memory in different tiers.

>
> My concern with this approach is that this value seems rather arbitrary.
> I imagine that it is workload- and memory access pattern- dependent,
> and will have to be tuned. Other than a couple of big users, no one
> will have the resources to do this.
>
> And since this is a one-off knob, there's another parameter users
> will have to decide - frequency, i.e how often should the userspace
> agent trigger this reclaim action. This is again very hard to determine
> a priori, and most likely has to be tuned as well.
>

I totally agree with you, this is the key point of this approach.It depends
on how we define cold pages, which are usually measured in time,
such as not being accessed for 600 seconds, etc. So the frequency
should be greater than 600 seconds.

> I think there might be some issues with just storing the store time here
> as well. IIUC, there might be cases where the zswap entry
> is accessed and brought into memory, but that entry (with the associated
> compressed memory) still hangs around. For e.g and more context,
> see this patch that enables exclusive loads:
>
> https://lore.kernel.org/lkml/20230607195143.1473802-1-yosryahmed@google.com/
>
> If that happens, this sto_time field does not tell the full story, right?
> For instance, if an object is stored a long time ago, but has been
> accessed since, it shouldn't be considered a cold object that should be
> a candidate for reclaim. But the old sto_time would indicate otherwise.
>

Thanks for your review，we should update the store time when it was loaded.
But it confused me, there are two copies of the same page in memory
(compressed and uncompressed) after faulting in a page from zswap if
'zswap_exclusive_loads_enabled' was disabled. I didn't notice any difference
when turning that option on or off because the frontswap_ops has been removed
and there is no frontswap_map anymore. Sorry, am I missing something?
  

On Thu, Oct 12, 2023 at 10:13:16PM +0800, 贺中坤 wrote:
> Hi Nhat, thanks for your detailed reply.
> 
> > We're currently trying to solve this exact problem. Our approach is to
> > add a shrinker that automatically shrinks the size of the zswap pool:
> >
> > https://lore.kernel.org/lkml/20230919171447.2712746-1-nphamcs@gmail.com/
> >
> > It is triggered on memory-pressure, and can perform reclaim in a
> > workload-specific manner.
> >
> > I'm currently working on v3 of this patch series, but in the meantime,
> > could you take a look and see if it will address your issues as well?
> >
> > Comments and suggestions are always welcome, of course :)
> >
> 
> Thanks, I've seen both patches. But we hope to be able to reclaim memory
> in advance, regardless of memory pressure, like memory.reclaim in memcg,
> so we can offload memory in different tiers.

Can you use memory.reclaim itself for that? With Nhat's shrinker, it
should move the whole pipeline (LRU -> zswap -> swap).

> Thanks for your review，we should update the store time when it was loaded.
> But it confused me, there are two copies of the same page in memory
> (compressed and uncompressed) after faulting in a page from zswap if
> 'zswap_exclusive_loads_enabled' was disabled. I didn't notice any difference
> when turning that option on or off because the frontswap_ops has been removed
> and there is no frontswap_map anymore. Sorry, am I missing something?

In many instances, swapins already free the swap slot through the
generic swap code (see should_try_to_free_swap()). It matters for
shared pages, or for swapcaching read-only data when swap isn't full -
it could be that isn't the case in your tests.
  

On Thu, Oct 12, 2023 at 7:13 AM 贺中坤 <hezhongkun.hzk@bytedance.com> wrote:
>
> Hi Nhat, thanks for your detailed reply.
>
> > We're currently trying to solve this exact problem. Our approach is to
> > add a shrinker that automatically shrinks the size of the zswap pool:
> >
> > https://lore.kernel.org/lkml/20230919171447.2712746-1-nphamcs@gmail.com/
> >
> > It is triggered on memory-pressure, and can perform reclaim in a
> > workload-specific manner.
> >
> > I'm currently working on v3 of this patch series, but in the meantime,
> > could you take a look and see if it will address your issues as well?
> >
> > Comments and suggestions are always welcome, of course :)
> >
>
> Thanks, I've seen both patches. But we hope to be able to reclaim memory
> in advance, regardless of memory pressure, like memory.reclaim in memcg,
> so we can offload memory in different tiers.

As Johannes pointed out, with a zswap shrinker, we can just push on
the memory.reclaim knob, and it'll automatically get pushed down the
pipeline:

memory -> swap -> zswap

That seems to be a bit more natural and user-friendly to me than
making the users manually decide to push zswap out to swap.

My ideal vision of how all of this should go is that users provide an
abstract declaration of requirement, and the specific decision of what
to be done is left to the kernel to perform, as transparently to the user
as possible. This philosophy extends to multi-tier memory management
in general, not just the above 3-tier model.

>
> >
> > My concern with this approach is that this value seems rather arbitrary.
> > I imagine that it is workload- and memory access pattern- dependent,
> > and will have to be tuned. Other than a couple of big users, no one
> > will have the resources to do this.
> >
> > And since this is a one-off knob, there's another parameter users
> > will have to decide - frequency, i.e how often should the userspace
> > agent trigger this reclaim action. This is again very hard to determine
> > a priori, and most likely has to be tuned as well.
> >
>
> I totally agree with you, this is the key point of this approach.It depends
> on how we define cold pages, which are usually measured in time,
> such as not being accessed for 600 seconds, etc. So the frequency
> should be greater than 600 seconds.

I guess my main concern here is - how do you determine the value
600 seconds in the first place?

And yes, the frequency should be greater than the oldness cutoff,
but how much greater?

We can run experiments to decide what cutoff will hurt performance
the least (or improve the performance the most), but that value will
be specific to our workload and memory access patterns. Other
users might need a different value entirely, and they might not have
the resources to find out.

If it's just a binary decision (on or off), then at least it could be
one A/B experiment (per workload/service). But the range here
could vary wildly.

Is there at least a default value that works decently well across
workload/service, in your experience?

>
> > I think there might be some issues with just storing the store time here
> > as well. IIUC, there might be cases where the zswap entry
> > is accessed and brought into memory, but that entry (with the associated
> > compressed memory) still hangs around. For e.g and more context,
> > see this patch that enables exclusive loads:
> >
> > https://lore.kernel.org/lkml/20230607195143.1473802-1-yosryahmed@google.com/
> >
> > If that happens, this sto_time field does not tell the full story, right?
> > For instance, if an object is stored a long time ago, but has been
> > accessed since, it shouldn't be considered a cold object that should be
> > a candidate for reclaim. But the old sto_time would indicate otherwise.
> >
>
> Thanks for your review，we should update the store time when it was loaded.
> But it confused me, there are two copies of the same page in memory
> (compressed and uncompressed) after faulting in a page from zswap if
> 'zswap_exclusive_loads_enabled' was disabled. I didn't notice any difference
> when turning that option on or off because the frontswap_ops has been removed
> and there is no frontswap_map anymore. Sorry, am I missing something?

I believe Johannes has explained the case where this could happen.
But yeah, this should be fixable with by updating the stored time
field on access (maybe rename it to something a bit more fitting as
well - last_accessed_time?)

Regardless, it is incredibly validating to see that other parties share the
same problems as us :) It's not a super invasive change as well.
I just don't think it solves the issue that well for every zswap user.
  

On Tue, Oct 10, 2023 at 10:11 PM Zhongkun He
<hezhongkun.hzk@bytedance.com> wrote:
>
> zswap does not have a suitable method to select objects that have not
> been accessed for a long time, and just shrink the pool when the limit
> is hit. There is a high probability of wasting memory in zswap if the
> limit is too high.
>
> This patch add a new interface writeback_time_threshold to shrink zswap
> pool proactively based on the time threshold in second, e.g.::
>
> echo 600 > /sys/module/zswap/parameters/writeback_time_threshold
>
> If zswap_entrys have not been accessed for more than 600 seconds, they
> will be swapout to swap. if set to 0, all of them will be swapout.
>
> Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>

I prefer if this can be done through memory.reclaim when the zswap
shrinker is in place, as others have suggested. I understand that this
provides more control by specifying the time at which to start writing
pages out, which is similar to zram writeback AFAICT, but it is also
difficult to determine the right value to write here.

I am also not sure how you decide that it is better to writeback cold
pages in zswap or compress cold pages in the LRUs. The pages in zswap
are obviously colder, but accessing them after they are written back
is much more expensive, to the point that it could be better to
compress more cold memory from the LRUs. This is obviously not
straightforward and requires a fair amount of tuning to do more good
than harm.

That being said, if we decide to move forward with this I have a
couple of comments:

- I think you should check out how zram implements idle writeback and
try to make things consistent. Zswap and zram don't really see eye to
eye, but some consistency would be nice. If you looked at zram's
implementation you would realize that you also need to update the
access time when a page is read (unless the load is exclusive).

- This should be behind a config option. Every word that we add to
struct zswap_entry reduces the zswap savings by roughly 0.2%. Maybe
this doesn't sound like much but it adds up. Let's not opt everyone in
unless they ask for it.

> ---
>  Documentation/admin-guide/mm/zswap.rst |  9 +++
>  mm/zswap.c                             | 76 ++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> index 45b98390e938..9ffaed26c3c0 100644
> --- a/Documentation/admin-guide/mm/zswap.rst
> +++ b/Documentation/admin-guide/mm/zswap.rst
> @@ -153,6 +153,15 @@ attribute, e. g.::
>
>  Setting this parameter to 100 will disable the hysteresis.
>
> +When there is a lot of cold memory according to the store time in the zswap,
> +it can be swapout and save memory in userspace proactively. User can write
> +writeback time threshold in second to enable it, e.g.::
> +
> +  echo 600 > /sys/module/zswap/parameters/writeback_time_threshold
> +
> +If zswap_entrys have not been accessed for more than 600 seconds, they will be
> +swapout. if set to 0, all of them will be swapout.
> +
>  A debugfs interface is provided for various statistic about pool size, number
>  of pages stored, same-value filled pages and various counters for the reasons
>  pages are rejected.
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 083c693602b8..c3a19b56a29b 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -141,6 +141,16 @@ static bool zswap_exclusive_loads_enabled = IS_ENABLED(
>                 CONFIG_ZSWAP_EXCLUSIVE_LOADS_DEFAULT_ON);
>  module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644);
>
> +/* zswap writeback time threshold in second */
> +static unsigned int  zswap_writeback_time_thr;
> +static int zswap_writeback_time_thr_param_set(const char *, const struct kernel_param *);
> +static const struct kernel_param_ops zswap_writeback_param_ops = {
> +       .set =          zswap_writeback_time_thr_param_set,
> +       .get =          param_get_uint,
> +};
> +module_param_cb(writeback_time_threshold, &zswap_writeback_param_ops,
> +                       &zswap_writeback_time_thr, 0644);
> +
>  /* Number of zpools in zswap_pool (empirically determined for scalability) */
>  #define ZSWAP_NR_ZPOOLS 32
>
> @@ -197,6 +207,7 @@ struct zswap_pool {
>   * value - value of the same-value filled pages which have same content
>   * objcg - the obj_cgroup that the compressed memory is charged to
>   * lru - handle to the pool's lru used to evict pages.
> + * sto_time - the store time of zswap_entry.
>   */
>  struct zswap_entry {
>         struct rb_node rbnode;
> @@ -210,6 +221,7 @@ struct zswap_entry {
>         };
>         struct obj_cgroup *objcg;
>         struct list_head lru;
> +       ktime_t sto_time;
>  };
>
>  /*
> @@ -288,6 +300,31 @@ static void zswap_update_total_size(void)
>         zswap_pool_total_size = total;
>  }
>
> +static void zswap_reclaim_entry_by_timethr(void);
> +
> +static bool zswap_reach_timethr(struct zswap_pool *pool)
> +{
> +       struct zswap_entry *entry;
> +       ktime_t expire_time = 0;
> +       bool ret = false;
> +
> +       spin_lock(&pool->lru_lock);
> +
> +       if (list_empty(&pool->lru))
> +               goto out;
> +
> +       entry = list_last_entry(&pool->lru, struct zswap_entry, lru);
> +       expire_time = ktime_add(entry->sto_time,
> +                       ns_to_ktime(zswap_writeback_time_thr * NSEC_PER_SEC));
> +
> +       if (ktime_after(ktime_get_boottime(), expire_time))
> +               ret = true;
> +out:
> +       spin_unlock(&pool->lru_lock);
> +       return ret;
> +}
> +
> +
>  /*********************************
>  * zswap entry functions
>  **********************************/
> @@ -395,6 +432,7 @@ static void zswap_free_entry(struct zswap_entry *entry)
>         else {
>                 spin_lock(&entry->pool->lru_lock);
>                 list_del(&entry->lru);
> +               entry->sto_time = 0;
>                 spin_unlock(&entry->pool->lru_lock);
>                 zpool_free(zswap_find_zpool(entry), entry->handle);
>                 zswap_pool_put(entry->pool);
> @@ -709,6 +747,28 @@ static void shrink_worker(struct work_struct *w)
>         zswap_pool_put(pool);
>  }
>
> +static void zswap_reclaim_entry_by_timethr(void)
> +{
> +       struct zswap_pool *pool = zswap_pool_current_get();
> +       int ret, failures = 0;
> +
> +       if (!pool)
> +               return;
> +
> +       while (zswap_reach_timethr(pool)) {
> +               ret = zswap_reclaim_entry(pool);
> +               if (ret) {
> +                       zswap_reject_reclaim_fail++;
> +                       if (ret != -EAGAIN)
> +                               break;
> +                       if (++failures == MAX_RECLAIM_RETRIES)
> +                               break;
> +               }
> +               cond_resched();
> +       }
> +       zswap_pool_put(pool);
> +}
> +
>  static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  {
>         int i;
> @@ -1037,6 +1097,21 @@ static int zswap_enabled_param_set(const char *val,
>         return ret;
>  }
>
> +static int zswap_writeback_time_thr_param_set(const char *val,
> +                               const struct kernel_param *kp)
> +{
> +       int ret = -ENODEV;
> +
> +       /* if this is load-time (pre-init) param setting, just return. */
> +       if (system_state != SYSTEM_RUNNING)
> +               return ret;
> +
> +       ret = param_set_uint(val, kp);
> +       if (!ret)
> +               zswap_reclaim_entry_by_timethr();
> +       return ret;
> +}
> +
>  /*********************************
>  * writeback code
>  **********************************/
> @@ -1360,6 +1435,7 @@ bool zswap_store(struct folio *folio)
>         if (entry->length) {
>                 spin_lock(&entry->pool->lru_lock);
>                 list_add(&entry->lru, &entry->pool->lru);
> +               entry->sto_time = ktime_get_boottime();
>                 spin_unlock(&entry->pool->lru_lock);
>         }
>         spin_unlock(&tree->lock);
> --
> 2.25.1
>
  

>
> Can you use memory.reclaim itself for that? With Nhat's shrinker, it
> should move the whole pipeline (LRU -> zswap -> swap).
>

Thanks,  I will backport it and have a try.

> In many instances, swapins already free the swap slot through the
> generic swap code (see should_try_to_free_swap()). It matters for
> shared pages, or for swapcaching read-only data when swap isn't full -
> it could be that isn't the case in your tests.

Got it. Thanks for your reply.
  

>
> As Johannes pointed out, with a zswap shrinker, we can just push on
> the memory.reclaim knob, and it'll automatically get pushed down the
> pipeline:
>
> memory -> swap -> zswap
>
> That seems to be a bit more natural and user-friendly to me than
> making the users manually decide to push zswap out to swap.
>
> My ideal vision of how all of this should go is that users provide an
> abstract declaration of requirement, and the specific decision of what
> to be done is left to the kernel to perform, as transparently to the user
> as possible. This philosophy extends to multi-tier memory management
> in general, not just the above 3-tier model.
>

That sounds great，i will backport it and  have a try.

>
> I guess my main concern here is - how do you determine the value
> 600 seconds in the first place?
>

I will test based on different applications and corresponding memory
access models. Usually we run similar programs on the same machine.
First, we can use memory.reclaim to swap out pages to zswap, and with
this patch , I would find the distribution of times the page resides in
zswap, and then choose the appropriate time.

> And yes, the frequency should be greater than the oldness cutoff,
> but how much greater?
>
This depends on the user's memory needs. If you want to reclaim
memory faster, you can set it to 1.5 times the threshold. On the contrary,
you can set it to 1 hour, two hours, etc.

> We can run experiments to decide what cutoff will hurt performance
> the least (or improve the performance the most), but that value will
> be specific to our workload and memory access patterns. Other
> users might need a different value entirely, and they might not have
> the resources to find out.
>
> If it's just a binary decision (on or off), then at least it could be
> one A/B experiment (per workload/service). But the range here
> could vary wildly.
>
> Is there at least a default value that works decently well across
> workload/service, in your experience?
>

Yes I agree, it's difficult to set a perfect value, but it's actually beneficial
to just have a normal value, such as 600 seconds by default. This
means that the zswap value stores values that have not been accessed
within 600 seconds and then unloads them to swap.

> I believe Johannes has explained the case where this could happen.
> But yeah, this should be fixable with by updating the stored time
> field on access (maybe rename it to something a bit more fitting as
> well - last_accessed_time?)

Thanks, I agree.

>
> Regardless, it is incredibly validating to see that other parties share the
> same problems as us :) It's not a super invasive change as well.
> I just don't think it solves the issue that well for every zswap user.

I've noticed this problem before and thought about some solutions,but only
saw your patch recently. I can also try it and discuss it together.At the
same time, I will think about how to improve this patch.
  

Thanks for your reply.
> I prefer if this can be done through memory.reclaim when the zswap
> shrinker is in place, as others have suggested. I understand that this
> provides more control by specifying the time at which to start writing
> pages out, which is similar to zram writeback AFAICT, but it is also
> difficult to determine the right value to write here.
>
> I am also not sure how you decide that it is better to writeback cold
> pages in zswap or compress cold pages in the LRUs. The pages in zswap
> are obviously colder, but accessing them after they are written back
> is much more expensive, to the point that it could be better to
> compress more cold memory from the LRUs. This is obviously not
> straightforward and requires a fair amount of tuning to do more good
> than harm.

I do agree.  For some applications, a common value will work,
such as 600s. Besides, this patch provides a more flexible way
to offload compress pages.

>
> That being said, if we decide to move forward with this I have a
> couple of comments:
>
> - I think you should check out how zram implements idle writeback and
> try to make things consistent. Zswap and zram don't really see eye to
> eye, but some consistency would be nice. If you looked at zram's
> implementation you would realize that you also need to update the
> access time when a page is read (unless the load is exclusive).

Thanks for your suggestion，i will fix it and check it again.

>
> - This should be behind a config option. Every word that we add to
> struct zswap_entry reduces the zswap savings by roughly 0.2%. Maybe
> this doesn't sound like much but it adds up. Let's not opt everyone in
> unless they ask for it.
>

Good idea， Thanks.