Re: [PATCH v5 12/13] zram: borrow idle decompression streams in exclusive mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] zram: raw_smp_processor_id() re-evaluated in zcomp_dstrm_excl_tryget() loop
--

commit a4463f4c7972dc5e8129cff84e8c92b3498b7a73
Author: Sergey Senozhatsky <senozhatsky@chromium.org>

zram: borrow idle decompression streams in exclusive mode

This commit allows exclusive decompression stream requests to scan online
CPUs and borrow an idle per-CPU decompression stream before falling back
to sleeping on a per-zcomp waitqueue.

> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 4fda13aac8965..c6d2fd23c6f66 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -185,38 +185,67 @@ bool zcomp_shared_decomp(struct zcomp *comp)
>  	return !comp->ops->create_dctx;
>  }
>  
> +static bool zcomp_dstrm_excl_trylock(struct zcomp_dstrm *zstrm)
> +{
> +	/* Cheap pre-check: don't touch a busy stream's cachelines */
> +	if (mutex_is_locked(&zstrm->excl_lock))
> +		return false;
> +	/*
> +	 * Pin the stream.  The write side of ->lock is only taken by
> +	 * zcomp_cpu_dead(), which also sets ->local_copy to NULL.
> +	 */
> +	if (!down_read_trylock(&zstrm->lock))
> +		return false;
> +	if (zstrm->local_copy && mutex_trylock(&zstrm->excl_lock))
> +		return true;
> +	up_read(&zstrm->lock);
> +	return false;
> +}
> +
> +static struct zcomp_dstrm *zcomp_dstrm_excl_tryget(struct zcomp *comp)
> +{
> +	struct zcomp_dstrm *zstrm;
> +	unsigned int cpu;
> +
> +	/* Best-effort start CPU hint; migration during the scan is fine */
> +	for_each_online_cpu_wrap(cpu, raw_smp_processor_id()) {
> +		zstrm = per_cpu_ptr(comp->dstream, cpu);
> +		if (zcomp_dstrm_excl_trylock(zstrm))
> +			return zstrm;
> +	}
> +	return NULL;
> +}

[Severity: High]
Does passing raw_smp_processor_id() directly as the start argument to
for_each_online_cpu_wrap() in zcomp_dstrm_excl_tryget() break the loop
termination logic when the task migrates during the scan?

for_each_online_cpu_wrap() expands via for_each_cpu_wrap() to
for_each_set_bit_wrap() in include/linux/find.h:

#define for_each_set_bit_wrap(bit, addr, size, start) \
	for ((bit) = find_next_bit_wrap((addr), (size), (start));		\
	     (bit) < (size);							\
	     (bit) = __for_each_wrap((addr), (size), (start), (bit) + 1))

Because start is expanded inside __for_each_wrap() on every step,
raw_smp_processor_id() is re-evaluated on each iteration rather than
once at the beginning of the loop.

__for_each_wrap() relies on start remaining constant across the loop to
know whether the scan has wrapped around:

	/* If not wrapped around */
	if (n > start) {
		/* and have a bit, just return it. */
		bit = find_next_bit(bitmap, size, n);
		if (bit < size)
			return bit;

		/* Otherwise, wrap around and ... */
		n = 0;
	}

	/* Search the other part. */
	bit = find_next_bit(bitmap, start, n);
	return bit < start ? bit : size;

Since zcomp_dstrm_get() calls zcomp_dstrm_excl_tryget() in preemptible
context with migration enabled, if a task fails zcomp_dstrm_excl_trylock()
on CPU k and migrates to CPU m >= k + 1 before the step expression runs,
__for_each_wrap() sees n = k + 1 <= start (where start is now m). It then
treats the scan as having already wrapped and only searches [k + 1, m)
(returning size immediately if m == k + 1), skipping CPU m and the
remaining online CPUs.

When that happens, zcomp_dstrm_excl_tryget() returns NULL and
zcomp_dstrm_get() sleeps in wait_event() on comp->dstrm_waitq even when
idle streams are available on the skipped CPUs. In the case where CPU k
is concurrently taken offline (so zcomp_cpu_dead() sets local_copy to NULL
without waking comp->dstrm_waitq and the task is migrated off CPU k) while
all other online streams are idle, no task holds excl_lock to wake
comp->dstrm_waitq and the caller can sleep in TASK_UNINTERRUPTIBLE
indefinitely.

Should raw_smp_processor_id() be stored in a local variable before passing
it to for_each_online_cpu_wrap()?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018035125.3120704-1-senozhatsky@chromium.org?part=12