From: Brian Geffon <bgeffon@google.com>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Minchan Kim <minchan@kernel.org>,
David Stevens <stevensd@google.com>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
linux-block@vger.kernel.org
Subject: Re: [PATCH 1/3] zram: use u32 for entry ac_time tracking
Date: Mon, 15 Dec 2025 17:31:12 -0500 [thread overview]
Message-ID: <CADyq12zu_o1fEz2B0nrZUFhbnEgiLPVkJv4ku5mrXYNBBNQ-Dg@mail.gmail.com> (raw)
In-Reply-To: <d7c0b48450c70eeb5fd8acd6ecd23593f30dbf1f.1765775954.git.senozhatsky@chromium.org>
On Mon, Dec 15, 2025 at 12:47 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> We can reduce sizeof(zram_table_entry) on 64-bit systems
> by converting flags and ac_time to u32. Entry flags fit
> into u32, and for ac_time u32 gives us over a century of
> entry lifespan (approx 136 years) which is plenty (zram
> uses system boot time (seconds)).
Makes sense.
>
> In struct zram_table_entry we use bytes aliasing, because
> bit-wait API (for slot lock) requires a whole unsigned
> long word.
>
> Suggested-by: David Stevens <stevensd@google.com>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Brian Geffon <bgeffon@google.com>
> ---
> drivers/block/zram/zram_drv.c | 60 +++++++++++++++++------------------
> drivers/block/zram/zram_drv.h | 9 ++++--
> 2 files changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 67a9e7c005c3..65f99ff3e2e5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -81,7 +81,7 @@ static void zram_slot_lock_init(struct zram *zram, u32 index)
> */
> static __must_check bool zram_slot_trylock(struct zram *zram, u32 index)
> {
> - unsigned long *lock = &zram->table[index].flags;
> + unsigned long *lock = &zram->table[index].__lock;
>
> if (!test_and_set_bit_lock(ZRAM_ENTRY_LOCK, lock)) {
> mutex_acquire(slot_dep_map(zram, index), 0, 1, _RET_IP_);
> @@ -94,7 +94,7 @@ static __must_check bool zram_slot_trylock(struct zram *zram, u32 index)
>
> static void zram_slot_lock(struct zram *zram, u32 index)
> {
> - unsigned long *lock = &zram->table[index].flags;
> + unsigned long *lock = &zram->table[index].__lock;
>
> mutex_acquire(slot_dep_map(zram, index), 0, 0, _RET_IP_);
> wait_on_bit_lock(lock, ZRAM_ENTRY_LOCK, TASK_UNINTERRUPTIBLE);
> @@ -103,7 +103,7 @@ static void zram_slot_lock(struct zram *zram, u32 index)
>
> static void zram_slot_unlock(struct zram *zram, u32 index)
> {
> - unsigned long *lock = &zram->table[index].flags;
> + unsigned long *lock = &zram->table[index].__lock;
>
> mutex_release(slot_dep_map(zram, index), _RET_IP_);
> clear_and_wake_up_bit(ZRAM_ENTRY_LOCK, lock);
> @@ -130,34 +130,33 @@ static void zram_set_handle(struct zram *zram, u32 index, unsigned long handle)
> }
>
> static bool zram_test_flag(struct zram *zram, u32 index,
> - enum zram_pageflags flag)
> + enum zram_pageflags flag)
> {
> - return zram->table[index].flags & BIT(flag);
> + return zram->table[index].attr.flags & BIT(flag);
> }
>
> static void zram_set_flag(struct zram *zram, u32 index,
> - enum zram_pageflags flag)
> + enum zram_pageflags flag)
> {
> - zram->table[index].flags |= BIT(flag);
> + zram->table[index].attr.flags |= BIT(flag);
> }
>
> static void zram_clear_flag(struct zram *zram, u32 index,
> - enum zram_pageflags flag)
> + enum zram_pageflags flag)
> {
> - zram->table[index].flags &= ~BIT(flag);
> + zram->table[index].attr.flags &= ~BIT(flag);
> }
>
> static size_t zram_get_obj_size(struct zram *zram, u32 index)
> {
> - return zram->table[index].flags & (BIT(ZRAM_FLAG_SHIFT) - 1);
> + return zram->table[index].attr.flags & (BIT(ZRAM_FLAG_SHIFT) - 1);
> }
>
> -static void zram_set_obj_size(struct zram *zram,
> - u32 index, size_t size)
> +static void zram_set_obj_size(struct zram *zram, u32 index, size_t size)
> {
> - unsigned long flags = zram->table[index].flags >> ZRAM_FLAG_SHIFT;
> + unsigned long flags = zram->table[index].attr.flags >> ZRAM_FLAG_SHIFT;
>
> - zram->table[index].flags = (flags << ZRAM_FLAG_SHIFT) | size;
> + zram->table[index].attr.flags = (flags << ZRAM_FLAG_SHIFT) | size;
> }
>
> static inline bool zram_allocated(struct zram *zram, u32 index)
> @@ -208,14 +207,14 @@ static inline void zram_set_priority(struct zram *zram, u32 index, u32 prio)
> * Clear previous priority value first, in case if we recompress
> * further an already recompressed page
> */
> - zram->table[index].flags &= ~(ZRAM_COMP_PRIORITY_MASK <<
> - ZRAM_COMP_PRIORITY_BIT1);
> - zram->table[index].flags |= (prio << ZRAM_COMP_PRIORITY_BIT1);
> + zram->table[index].attr.flags &= ~(ZRAM_COMP_PRIORITY_MASK <<
> + ZRAM_COMP_PRIORITY_BIT1);
> + zram->table[index].attr.flags |= (prio << ZRAM_COMP_PRIORITY_BIT1);
> }
>
> static inline u32 zram_get_priority(struct zram *zram, u32 index)
> {
> - u32 prio = zram->table[index].flags >> ZRAM_COMP_PRIORITY_BIT1;
> + u32 prio = zram->table[index].attr.flags >> ZRAM_COMP_PRIORITY_BIT1;
>
> return prio & ZRAM_COMP_PRIORITY_MASK;
> }
> @@ -225,7 +224,7 @@ static void zram_accessed(struct zram *zram, u32 index)
> zram_clear_flag(zram, index, ZRAM_IDLE);
> zram_clear_flag(zram, index, ZRAM_PP_SLOT);
> #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
> - zram->table[index].ac_time = ktime_get_boottime();
> + zram->table[index].attr.ac_time = (u32)ktime_get_boottime_seconds();
> #endif
> }
>
> @@ -447,7 +446,7 @@ static void mark_idle(struct zram *zram, ktime_t cutoff)
>
> #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
> is_idle = !cutoff ||
> - ktime_after(cutoff, zram->table[index].ac_time);
> + ktime_after(cutoff, zram->table[index].attr.ac_time);
> #endif
> if (is_idle)
> zram_set_flag(zram, index, ZRAM_IDLE);
> @@ -461,18 +460,19 @@ 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);
> - ktime_t cutoff_time = 0;
> + ktime_t cutoff = 0;
>
> if (!sysfs_streq(buf, "all")) {
> /*
> * If it did not parse as 'all' try to treat it as an integer
> * when we have memory tracking enabled.
> */
> - u64 age_sec;
> + u32 age_sec;
>
> - if (IS_ENABLED(CONFIG_ZRAM_TRACK_ENTRY_ACTIME) && !kstrtoull(buf, 0, &age_sec))
> - cutoff_time = ktime_sub(ktime_get_boottime(),
> - ns_to_ktime(age_sec * NSEC_PER_SEC));
> + if (IS_ENABLED(CONFIG_ZRAM_TRACK_ENTRY_ACTIME) &&
> + !kstrtouint(buf, 0, &age_sec))
> + cutoff = ktime_sub((u32)ktime_get_boottime_seconds(),
> + age_sec);
> else
> return -EINVAL;
> }
> @@ -482,10 +482,10 @@ static ssize_t idle_store(struct device *dev, struct device_attribute *attr,
> return -EINVAL;
>
> /*
> - * A cutoff_time of 0 marks everything as idle, this is the
> + * A cutoff of 0 marks everything as idle, this is the
> * "all" behavior.
> */
> - mark_idle(zram, cutoff_time);
> + mark_idle(zram, cutoff);
> return len;
> }
>
> @@ -1588,7 +1588,7 @@ static ssize_t read_block_state(struct file *file, char __user *buf,
> if (!zram_allocated(zram, index))
> goto next;
>
> - ts = ktime_to_timespec64(zram->table[index].ac_time);
> + ts = ktime_to_timespec64(zram->table[index].attr.ac_time);
> copied = snprintf(kbuf + written, count,
> "%12zd %12lld.%06lu %c%c%c%c%c%c\n",
> index, (s64)ts.tv_sec,
> @@ -2013,7 +2013,7 @@ static void zram_slot_free(struct zram *zram, u32 index)
> unsigned long handle;
>
> #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
> - zram->table[index].ac_time = 0;
> + zram->table[index].attr.ac_time = 0;
> #endif
>
> zram_clear_flag(zram, index, ZRAM_IDLE);
> @@ -3286,7 +3286,7 @@ static int __init zram_init(void)
> struct zram_table_entry zram_te;
> int ret;
>
> - BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > sizeof(zram_te.flags) * 8);
> + BUILD_BUG_ON(__NR_ZRAM_PAGEFLAGS > sizeof(zram_te.attr.flags) * 8);
>
> ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
> zcomp_cpu_up_prepare, zcomp_cpu_dead);
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 72fdf66c78ab..48d6861c6647 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -65,10 +65,15 @@ enum zram_pageflags {
> */
> struct zram_table_entry {
> unsigned long handle;
> - unsigned long flags;
> + union {
> + unsigned long __lock;
> + struct attr {
> + u32 flags;
> #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
> - ktime_t ac_time;
> + u32 ac_time;
> #endif
Why not just always enable CONFIG_ZRAM_TRACK_ENTRY_ACTIME now that it
doesn't consume any additional space? Also, why can't we do this with
a single unsigned long flags as before and have a simple method that
isolates and casts the lower 32bits as a u32?
static u32 *zram_get_actime_for_slot(struct zram *zram, u32 index) {
return ((u32*)&zram->table[index].flags) + 1;
}
>
> --
> 2.52.0.239.gd5f0c6e74e-goog
>
next prev parent reply other threads:[~2025-12-15 22:31 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-15 5:47 Sergey Senozhatsky
2025-12-15 5:47 ` [PATCH 2/3] zram: rename internal slot API Sergey Senozhatsky
2025-12-15 5:47 ` [PATCH 3/3] zram: trivial fix of recompress_slot() coding styles Sergey Senozhatsky
2025-12-15 22:31 ` Brian Geffon [this message]
2025-12-16 0:59 ` [PATCH 1/3] zram: use u32 for entry ac_time tracking Sergey Senozhatsky
2025-12-16 1:17 ` Brian Geffon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADyq12zu_o1fEz2B0nrZUFhbnEgiLPVkJv4ku5mrXYNBBNQ-Dg@mail.gmail.com \
--to=bgeffon@google.com \
--cc=akpm@linux-foundation.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=minchan@kernel.org \
--cc=senozhatsky@chromium.org \
--cc=stevensd@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox