Re: [PATCH] zram: Introduce an aged idle interface

[Brian Geffon <bgeffon@google.com>]

On Mon, Sep 20, 2021 at 5:15 PM Minchan Kim <minchan@kernel.org> wrote:
>
> On Fri, Sep 17, 2021 at 02:06:40PM -0700, Brian Geffon wrote:
> > This change introduces a new sysfs file for the zram idle interface.
> > The current idle interface has only a single mode "all." This is
> > severely limiting in that it requires that you have the foresight
> > to know that you'll want to have pages idle at some point in the future
> > and forces the user to mark everything as idle.
> >
> > This new proposed file idle_aged takes a single integer argument which
> > represents the age of the pages (in seconds since access) to mark as idle.
> > Because it requires accessed tracking it is only enabled when
> > CONFIG_ZRAM_MEMORY_TRACKING is enabled. There are a few
> > reasons why  this is being proposed as a new file rather than extending
> > the existing file. The first reason is that it wouldn't allow more code
> > sharing as this change already refactors the existing code to do so.
> > Secondly, having a standalone file allows a caller to quickly check if this
> > idle_aged interface is supported. Finally, it simplifies the parsing
> > logic because now you only need to parse an int rather than more complex
> > logic which would need to parse things like "aged 50."
>
> I am not convinced with create new sysfs node to support the aged time.
> The reason I used the "all" was to have the room to support the aged
> time as you suggested for the future. IOW, If the value is not "all" but
> "only numeric", we could take it as "aged" value.
>
> And then, we can write up in the doc "The age-based idle" supported by
> kernel version 5.XX with CONFIG_ZRAM_MEMORY_TRACKING.
> I understand the separate interface will make code simple but not sure
> it's worth to create another sysfs knob.
>
> Any thoughts?

Sure, Sergey had similar feedback on that. I'll mail a new patch that
uses the existing knob but attempts to parse the input as an integer.

>
> >
> > Signed-off-by: Brian Geffon <bgeffon@google.com>
> > ---
> >  Documentation/admin-guide/blockdev/zram.rst | 11 ++-
> >  drivers/block/zram/zram_drv.c               | 75 +++++++++++++++++----
> >  2 files changed, 70 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/blockdev/zram.rst b/Documentation/admin-guide/blockdev/zram.rst
> > index 700329d25f57..ecd1c4916a1c 100644
> > --- a/Documentation/admin-guide/blockdev/zram.rst
> > +++ b/Documentation/admin-guide/blockdev/zram.rst
> > @@ -209,6 +209,8 @@ compact                   WO      trigger memory compaction
> >  debug_stat           RO      this file is used for zram debugging purposes
> >  backing_dev          RW      set up backend storage for zram to write out
> >  idle                 WO      mark allocated slot as idle
> > +idle_aged              WO      mark allocated slot older than 'age' seconds
> > +                                as idle (see later)
> >  ======================  ======  ===============================================
> >
> >
> > @@ -325,8 +327,13 @@ as idle::
> >
> >       echo all > /sys/block/zramX/idle
> >
> > -From now on, any pages on zram are idle pages. The idle mark
> > -will be removed until someone requests access of the block.
> > +Alternatively if the config option CONFIG_ZRAM_MEMORY_TRACKING is enabled
> > +the idle_aged interface can be used to mark only pages older than 'age'
> > +seconds as idle::
> > +
> > +        echo 86400 > /sys/block/zramX/idle_aged
> > +
> > +The idle mark will be removed until someone requests access of the block.
> >  IOW, unless there is access request, those pages are still idle pages.
> >
> >  Admin can request writeback of those idle pages at right timing via::
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index fcaf2750f68f..a371dc0edf9d 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -291,22 +291,16 @@ static ssize_t mem_used_max_store(struct device *dev,
> >       return len;
> >  }
> >
> > -static ssize_t idle_store(struct device *dev,
> > -             struct device_attribute *attr, const char *buf, size_t len)
> > +/*
> > + * Mark all pages which are older than or equal to cutoff as IDLE.
> > + * Callers should hold the zram init lock in read mode
> > + **/
> > +static void mark_idle(struct zram *zram, ktime_t cutoff)
> >  {
> > -     struct zram *zram = dev_to_zram(dev);
> > +     int is_idle = 1;
> >       unsigned long nr_pages = zram->disksize >> PAGE_SHIFT;
> >       int index;
> >
> > -     if (!sysfs_streq(buf, "all"))
> > -             return -EINVAL;
> > -
> > -     down_read(&zram->init_lock);
> > -     if (!init_done(zram)) {
> > -             up_read(&zram->init_lock);
> > -             return -EINVAL;
> > -     }
> > -
> >       for (index = 0; index < nr_pages; index++) {
> >               /*
> >                * Do not mark ZRAM_UNDER_WB slot as ZRAM_IDLE to close race.
> > @@ -314,16 +308,63 @@ static ssize_t idle_store(struct device *dev,
> >                */
> >               zram_slot_lock(zram, index);
> >               if (zram_allocated(zram, index) &&
> > -                             !zram_test_flag(zram, index, ZRAM_UNDER_WB))
> > -                     zram_set_flag(zram, index, ZRAM_IDLE);
> > +                             !zram_test_flag(zram, index, ZRAM_UNDER_WB)) {
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +                     is_idle = (!cutoff || cutoff >= zram->table[index].ac_time);
>
> I am not sure it's a good idea to compare those ktime_t directly
> without using the ktime API(e.g., ktime_after or ktime_before).
> Maybe, we can get age_sec ktime_t directly as a argument and then
> use the ktime API with ktime_get_boottime to compare them.

I will make this change in the next version of the patch.

>
> > +#endif
> > +                     if (is_idle)
> > +                             zram_set_flag(zram, index, ZRAM_IDLE);
> > +             }
> >               zram_slot_unlock(zram, index);
> >       }
> > +}
> > +
> > +static ssize_t idle_store(struct device *dev,
> > +             struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > +     struct zram *zram = dev_to_zram(dev);
> > +
> > +     if (!sysfs_streq(buf, "all"))
> > +             return -EINVAL;
> > +
> > +     down_read(&zram->init_lock);
> > +     if (!init_done(zram)) {
> > +             up_read(&zram->init_lock);
> > +             return -EINVAL;
> > +     }
> >
> > +     /* Mark everything as idle */
> > +     mark_idle(zram, 0);
> >       up_read(&zram->init_lock);
> >
> >       return len;
> >  }
> >
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +static ssize_t idle_aged_store(struct device *dev,
> > +             struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > +     struct zram *zram = dev_to_zram(dev);
> > +     ktime_t cutoff_time;
> > +     u64 age_sec;
> > +     ssize_t rv = -EINVAL;
> > +
> > +     down_read(&zram->init_lock);
> > +     if (!init_done(zram))
> > +             goto out;
> > +
> > +     if (kstrtoull(buf, 10, &age_sec))
> > +             goto out;
> > +
> > +     rv = len;
> > +     cutoff_time = ktime_sub(ktime_get_boottime(), ns_to_ktime(age_sec * NSEC_PER_SEC));
> > +     mark_idle(zram, cutoff_time);
> > +out:
> > +     up_read(&zram->init_lock);
> > +     return rv;
> > +}
> > +#endif
> > +
> >  #ifdef CONFIG_ZRAM_WRITEBACK
> >  static ssize_t writeback_limit_enable_store(struct device *dev,
> >               struct device_attribute *attr, const char *buf, size_t len)
> > @@ -1840,6 +1881,9 @@ static DEVICE_ATTR_WO(reset);
> >  static DEVICE_ATTR_WO(mem_limit);
> >  static DEVICE_ATTR_WO(mem_used_max);
> >  static DEVICE_ATTR_WO(idle);
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +static DEVICE_ATTR_WO(idle_aged);
> > +#endif
> >  static DEVICE_ATTR_RW(max_comp_streams);
> >  static DEVICE_ATTR_RW(comp_algorithm);
> >  #ifdef CONFIG_ZRAM_WRITEBACK
> > @@ -1857,6 +1901,9 @@ static struct attribute *zram_disk_attrs[] = {
> >       &dev_attr_mem_limit.attr,
> >       &dev_attr_mem_used_max.attr,
> >       &dev_attr_idle.attr,
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +     &dev_attr_idle_aged.attr,
> > +#endif
> >       &dev_attr_max_comp_streams.attr,
> >       &dev_attr_comp_algorithm.attr,
> >  #ifdef CONFIG_ZRAM_WRITEBACK
> > --
> > 2.33.0.464.g1972c5931b-goog
> >

Thank you for looking,
Brian