Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Mon, 20 Sep 2021 14:14:54 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     Brian Geffon <bgeffon@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Nitin Gupta <ngupta@vflare.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Jonathan Corbet <corbet@lwn.net>, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-block@vger.kernel.org,
        Suleiman Souhlal <suleiman@google.com>,
        Jesse Barnes <jsbarnes@google.com>
Subject: Re: [PATCH] zram: Introduce an aged idle interface
Message-ID: <YUj5zkRN+N404nbe@google.com>
References: <20210917210640.214211-1-bgeffon@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210917210640.214211-1-bgeffon@google.com>
Precedence: bulk

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?

> 
> 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. 

> +#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
>