Re: [RFC PATCH] zram: avoid preemption with CPU-based compression backends

On Wed, Aug 5, 2026 at 10:19 AM Bo Zhang <[email protected]> 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);