Re: [PATCH v5 02/18] zram: permit preemption with active compression stream

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Wed, Feb 12, 2025 at 03:27:00PM +0900, Sergey Senozhatsky wrote:
> Currently, per-CPU stream access is done from a non-preemptible
> (atomic) section, which imposes the same atomicity requirements on
> compression backends as entry spin-lock, and makes it impossible
> to use algorithms that can schedule/wait/sleep during compression
> and decompression.
> 
> Switch to preemptible per-CPU model, similar to the one used
> in zswap.  Instead of a per-CPU local lock, each stream carries
> a mutex which is locked throughout entire time zram uses it
> for compression or decompression, so that cpu-dead event waits
> for zram to stop using a particular per-CPU stream and release
> it.
> 
> Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/block/zram/zcomp.c    | 36 +++++++++++++++++++++++++----------
>  drivers/block/zram/zcomp.h    |  6 +++---
>  drivers/block/zram/zram_drv.c | 20 +++++++++----------
>  3 files changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index bb514403e305..e83dd9a80a81 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -7,6 +7,7 @@
>  #include <linux/wait.h>
>  #include <linux/sched.h>
>  #include <linux/cpu.h>
> +#include <linux/cpuhotplug.h>

What code changes prompt this?

>  #include <linux/crypto.h>
>  #include <linux/vmalloc.h>
>  
> @@ -54,6 +55,7 @@ static int zcomp_strm_init(struct zcomp *comp, struct zcomp_strm *zstrm)
>  {
>  	int ret;
>  
> +	mutex_init(&zstrm->lock);

I don't think we can initialize the mutex in the hotplug callback. I
think the following scenario is possible:

CPU #1					CPU #2
zcomp_stream_get()
  zstrm = raw_cpu_ptr()
/* task migrated to CPU 2 */

CPU goes offline
  zcomp_cpu_dead()
    mutex_lock()
    ..
    mutex_unlock()
					/* migrated task continues */
					zcomp_stream_get()
					  mutex_lock()
CPU goes online
  mutex_init()
  					  mutex_unlock() /* problem */
					
In this case we'll end up initializing the mutex on CPU #1 while CPU #2
has it locked. When we unlocked it on CPU #2 we will corrupt it AFAICT.

This is why I moved the mutex initialization out of the hotplug callback
in zswap. I suspect to do something similar for zram we'd need to do it
in zcomp_init()?

>  	ret = comp->ops->create_ctx(comp->params, &zstrm->ctx);
>  	if (ret)
>  		return ret;
> @@ -109,13 +111,29 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
>  
>  struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
>  {
> -	local_lock(&comp->stream->lock);
> -	return this_cpu_ptr(comp->stream);
> +	for (;;) {
> +		struct zcomp_strm *zstrm = raw_cpu_ptr(comp->stream);
> +
> +		/*
> +		 * Inspired by zswap
> +		 *
> +		 * stream is returned with ->mutex locked which prevents
> +		 * cpu_dead() from releasing this stream under us, however
> +		 * there is still a race window between raw_cpu_ptr() and
> +		 * mutex_lock(), during which we could have been migrated
> +		 * to a CPU that has already destroyed its stream.  If so

"we could have been migrated from** a CPU that has already destroyed its
stream"? Right?

> +		 * then unlock and re-try on the current CPU.
> +		 */
> +		mutex_lock(&zstrm->lock);
> +		if (likely(zstrm->buffer))
> +			return zstrm;
> +		mutex_unlock(&zstrm->lock);
> +	}
>  }
>  
> -void zcomp_stream_put(struct zcomp *comp)
> +void zcomp_stream_put(struct zcomp_strm *zstrm)
>  {
> -	local_unlock(&comp->stream->lock);
> +	mutex_unlock(&zstrm->lock);
>  }
>  
>  int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> @@ -151,12 +169,9 @@ int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm,
>  int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node)
>  {
>  	struct zcomp *comp = hlist_entry(node, struct zcomp, node);
> -	struct zcomp_strm *zstrm;
> +	struct zcomp_strm *zstrm = per_cpu_ptr(comp->stream, cpu);
>  	int ret;
>  
> -	zstrm = per_cpu_ptr(comp->stream, cpu);
> -	local_lock_init(&zstrm->lock);
> -
>  	ret = zcomp_strm_init(comp, zstrm);
>  	if (ret)
>  		pr_err("Can't allocate a compression stream\n");
> @@ -166,10 +181,11 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node)
>  int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node)
>  {
>  	struct zcomp *comp = hlist_entry(node, struct zcomp, node);
> -	struct zcomp_strm *zstrm;
> +	struct zcomp_strm *zstrm = per_cpu_ptr(comp->stream, cpu);
>  
> -	zstrm = per_cpu_ptr(comp->stream, cpu);
> +	mutex_lock(&zstrm->lock);
>  	zcomp_strm_free(comp, zstrm);
> +	mutex_unlock(&zstrm->lock);
>  	return 0;
>  }
>  
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index ad5762813842..23b8236b9090 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -3,7 +3,7 @@
>  #ifndef _ZCOMP_H_
>  #define _ZCOMP_H_
>  
> -#include <linux/local_lock.h>
> +#include <linux/mutex.h>
>  
>  #define ZCOMP_PARAM_NO_LEVEL	INT_MIN
>  
> @@ -31,7 +31,7 @@ struct zcomp_ctx {
>  };
>  
>  struct zcomp_strm {
> -	local_lock_t lock;
> +	struct mutex lock;
>  	/* compression buffer */
>  	void *buffer;
>  	struct zcomp_ctx ctx;
> @@ -77,7 +77,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);
>  
>  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 3708436f1d1f..43f460a45e3e 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1608,7 +1608,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_unmap_object(zram->mem_pool, handle);
> -	zcomp_stream_put(zram->comps[prio]);
> +	zcomp_stream_put(zstrm);
>  
>  	return ret;
>  }
> @@ -1769,14 +1769,14 @@ 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);
>  		pr_err("Compression failed! err=%d\n", ret);
>  		zs_free(zram->mem_pool, handle);
>  		return ret;
>  	}
>  
>  	if (comp_len >= huge_class_size) {
> -		zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
> +		zcomp_stream_put(zstrm);
>  		return write_incompressible_page(zram, page, index);
>  	}
>  
> @@ -1800,7 +1800,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index)
>  				   __GFP_HIGHMEM |
>  				   __GFP_MOVABLE);
>  	if (IS_ERR_VALUE(handle)) {
> -		zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
> +		zcomp_stream_put(zstrm);
>  		atomic64_inc(&zram->stats.writestall);
>  		handle = zs_malloc(zram->mem_pool, comp_len,
>  				   GFP_NOIO | __GFP_HIGHMEM |
> @@ -1812,7 +1812,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index)
>  	}
>  
>  	if (!zram_can_store_page(zram)) {
> -		zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
> +		zcomp_stream_put(zstrm);
>  		zs_free(zram->mem_pool, handle);
>  		return -ENOMEM;
>  	}
> @@ -1820,7 +1820,7 @@ static int zram_write_page(struct zram *zram, struct page *page, u32 index)
>  	dst = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
>  
>  	memcpy(dst, zstrm->buffer, comp_len);
> -	zcomp_stream_put(zram->comps[ZRAM_PRIMARY_COMP]);
> +	zcomp_stream_put(zstrm);
>  	zs_unmap_object(zram->mem_pool, handle);
>  
>  	zram_slot_lock(zram, index);
> @@ -1979,7 +1979,7 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page,
>  		kunmap_local(src);
>  
>  		if (ret) {
> -			zcomp_stream_put(zram->comps[prio]);
> +			zcomp_stream_put(zstrm);
>  			return ret;
>  		}
>  
> @@ -1989,7 +1989,7 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page,
>  		/* Continue until we make progress */
>  		if (class_index_new >= class_index_old ||
>  		    (threshold && comp_len_new >= threshold)) {
> -			zcomp_stream_put(zram->comps[prio]);
> +			zcomp_stream_put(zstrm);
>  			continue;
>  		}
>  
> @@ -2047,13 +2047,13 @@ static int recompress_slot(struct zram *zram, u32 index, struct page *page,
>  			       __GFP_HIGHMEM |
>  			       __GFP_MOVABLE);
>  	if (IS_ERR_VALUE(handle_new)) {
> -		zcomp_stream_put(zram->comps[prio]);
> +		zcomp_stream_put(zstrm);
>  		return PTR_ERR((void *)handle_new);
>  	}
>  
>  	dst = zs_map_object(zram->mem_pool, handle_new, ZS_MM_WO);
>  	memcpy(dst, zstrm->buffer, comp_len_new);
> -	zcomp_stream_put(zram->comps[prio]);
> +	zcomp_stream_put(zstrm);
>  
>  	zs_unmap_object(zram->mem_pool, handle_new);
>  
> -- 
> 2.48.1.502.g6dc24dfdaf-goog
>