Re: [RFC PATCH] zram: avoid preemption with CPU-based compression backends
From: Barry Song
Date: Tue Aug 04 2026 - 22:32:00 EST
On Wed, Aug 5, 2026 at 10:19 AM Bo Zhang <zhangbo0325@xxxxxxxxx> wrote:
>
> On Wed, Aug 05, 2026 at 08:55:45AM +0800, Barry Song (Xiaomi) wrote:
> > Since commit 2efa9e9eb4db ("zram: permit preemption with active
> > compression stream"), a major Android regression has been reported.
> >
> > We add an async flag (currently false for almost all backends) to
> > indicate whether a backend is asynchronous. For synchronous
> > backends, we use preempt_disable() in the !PREEMPT_RT case. A
> > zram_zs_malloc() wrapper is provided to support a two-stage
> > zs_malloc() path, allowing allocation to transition from a
> > non-sleepable context to a sleepable context.
>
> Hi Barry,
>
> Thanks for working on this. The priority inversion issue on Android is
> real and we've hit it too.
>
> However, I think there's a bug in the zcomp_stream_put() change.
>
> The new zcomp_stream_put() uses raw_cpu_ptr() to find the stream:
>
> void zcomp_stream_put(struct zcomp *comp)
> {
> struct zcomp_strm *zstrm = raw_cpu_ptr(comp->stream);
> if (!comp->ops->async && !IS_ENABLED(CONFIG_PREEMPT_RT))
> preempt_enable();
> mutex_unlock(&zstrm->lock);
> }
>
> It relies on the thread still being on the same CPU where
> zcomp_stream_get() locked the stream. In the normal path it works
> fine, because preempt is disabled the entire time.
>
> But zram_zs_malloc() breaks this assumption in the fallback path:
>
> static unsigned long zram_zs_malloc(...)
> {
> if (!async && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> handle = zs_malloc(..., __GFP_KSWAPD_RECLAIM | ...);
> if (!IS_ERR_VALUE(handle))
> return handle;
> preempt_enable(); // preempt is now enabled
> }
>
> handle = zs_malloc(..., GFP_NOIO | ...); // may sleep, may migrate
> if (!async && !IS_ENABLED(CONFIG_PREEMPT_RT))
> preempt_disable(); // Preempt disabled, but it may work on another CPU
> return handle;
> }
>
> If the first zs_malloc() fails, preempt is enabled and the second
> zs_malloc(GFP_NOIO) can sleep. During this window the task can migrate
> to another CPU. After preempt_disable(), we continue on the new CPU.
>
> When zcomp_stream_put() is later called, raw_cpu_ptr() returns the new
> CPU's stream, which is not the one that was originally locked. Which may
> cause a mutex_unlock() on the wrong CPU.
>
> The original code didn't have this problem because zcomp_stream_put()
> took the zstrm pointer directly:
>
> void zcomp_stream_put(struct zcomp_strm *zstrm)
> {
> mutex_unlock(&zstrm->lock);
> }
>
> The caller always passed the saved pointer from zcomp_stream_get(),
> so regardless of CPU migration, the correct mutex was always unlocked.
>
> I think the fix is to keep passing the zstrm pointer:
>
> void zcomp_stream_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> {
> if (!comp->ops->async && !IS_ENABLED(CONFIG_PREEMPT_RT))
> preempt_enable();
> mutex_unlock(&zstrm->lock);
> }
>
> All callers already have zstrm available. Then It works as expected.
Good catch. You're absolutely right. We need the following
(will fix in v2):
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index dd4db4a8af73..b7f5127c05c8 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -136,10 +136,8 @@ struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
}
}
-void zcomp_stream_put(struct zcomp *comp)
+void zcomp_stream_put(struct zcomp_strm *zstrm, struct zcomp *comp)
{
- struct zcomp_strm *zstrm = raw_cpu_ptr(comp->stream);
-
if (!comp->ops->async && !IS_ENABLED(CONFIG_PREEMPT_RT))
preempt_enable();
mutex_unlock(&zstrm->lock);
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 5293d638da8f..aa2fd3646018 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -87,7 +87,7 @@ struct zcomp *zcomp_create(const char *alg, struct
zcomp_params *params);
void zcomp_destroy(struct zcomp *comp);
struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
-void zcomp_stream_put(struct zcomp *comp);
+void zcomp_stream_put(struct zcomp_strm *zstrm, struct zcomp *comp);
int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
const void *src, unsigned int *dst_len);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 52b9fbe3e0d1..8f36ca122e1c 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1364,7 +1364,7 @@ static int decompress_bdev_page(struct zram
*zram, struct page *page, u32 index)
if (!ret)
copy_page(src, zstrm->local_copy);
kunmap_local(src);
- zcomp_stream_put(zram->comps[prio]);
+ zcomp_stream_put(zstrm, zram->comps[prio]);
slot_unlock(zram, index);
return ret;
@@ -2098,7 +2098,7 @@ static int read_compressed_page(struct zram
*zram, struct page *page, u32 index)
ret = zcomp_decompress(zram->comps[prio], zstrm, src, size, dst);
kunmap_local(dst);
zs_obj_read_end(zram->mem_pool, handle, size, src);
- zcomp_stream_put(zram->comps[prio]);
+ zcomp_stream_put(zstrm, zram->comps[prio]);
return ret;
}
@@ -2124,7 +2124,7 @@ static int read_from_zspool_raw(struct zram
*zram, struct page *page, u32 index)
zstrm->local_copy);
memcpy_to_page(page, 0, src, size);
zs_obj_read_end(zram->mem_pool, handle, size, src);
- zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
+ zcomp_stream_put(zstrm, zram->comps[ZRAM_PRIMARY_COMP]);
memzero_page(page, size, PAGE_SIZE - size);
@@ -2304,30 +2304,30 @@ static int zram_write_page(struct zram *zram,
struct page *page, u32 index)
kunmap_local(mem);
if (unlikely(ret)) {
- zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
+ zcomp_stream_put(zstrm, zram->comps[ZRAM_PRIMARY_COMP]);
pr_err("Compression failed! err=%d\n", ret);
return ret;
}
if (comp_len >= huge_class_size) {
- zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
+ zcomp_stream_put(zstrm, zram->comps[ZRAM_PRIMARY_COMP]);
return write_incompressible_page(zram, page, index, async);
}
handle = zram_zs_malloc(zram, comp_len, async, page_to_nid(page));
if (IS_ERR_VALUE(handle)) {
- zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
+ zcomp_stream_put(zstrm, zram->comps[ZRAM_PRIMARY_COMP]);
return PTR_ERR((void *)handle);
}
if (!zram_can_store_page(zram)) {
- zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
+ zcomp_stream_put(zstrm, zram->comps[ZRAM_PRIMARY_COMP]);
zs_free(zram->mem_pool, handle);
return -ENOMEM;
}
zs_obj_write(zram->mem_pool, handle, zstrm->buffer, comp_len);
- zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
+ zcomp_stream_put(zstrm, zram->comps[ZRAM_PRIMARY_COMP]);
slot_lock(zram, index);
slot_free(zram, index);
@@ -2484,7 +2484,7 @@ static int recompress_slot(struct zram *zram,
u32 index, struct page *page,
*num_recomp_pages -= 1;
if (ret) {
- zcomp_stream_put(zram->comps[prio]);
+ zcomp_stream_put(zstrm, zram->comps[prio]);
return ret;
}
@@ -2493,7 +2493,7 @@ static int recompress_slot(struct zram *zram,
u32 index, struct page *page,
if (class_index_new >= class_index_old ||
(threshold && comp_len_new >= threshold)) {
- zcomp_stream_put(zram->comps[prio]);
+ zcomp_stream_put(zstrm, zram->comps[prio]);
/*
* Secondary algorithms failed to re-compress the page
@@ -2509,12 +2509,12 @@ static int recompress_slot(struct zram *zram,
u32 index, struct page *page,
handle_new = zram_zs_malloc(zram, comp_len_new, async,
page_to_nid(page));
if (IS_ERR_VALUE(handle_new)) {
- zcomp_stream_put(zram->comps[prio]);
+ zcomp_stream_put(zstrm, zram->comps[prio]);
return PTR_ERR((void *)handle_new);
}
zs_obj_write(zram->mem_pool, handle_new, zstrm->buffer, comp_len_new);
- zcomp_stream_put(zram->comps[prio]);
+ zcomp_stream_put(zstrm, zram->comps[prio]);
slot_free(zram, index);
set_slot_handle(zram, index, handle_new);