Re: [PATCHv1 3/6] zsmalloc: make zspage lock preemptible

[Uros Bizjak]

On 29. 01. 25 07:43, Sergey Senozhatsky wrote:

> +/*
> + * zspage lock permits preemption on the reader-side (there can be multiple
> + * readers).  Writers (exclusive zspage ownership), on the other hand, are
> + * always run in atomic context and cannot spin waiting for a (potentially
> + * preempted) reader to unlock zspage.  This, basically, means that writers
> + * can only call write-try-lock and must bail out if it didn't succeed.
> + *
> + * At the same time, writers cannot reschedule under zspage write-lock,
> + * so readers can spin waiting for the writer to unlock zspage.
> + */
> +static void zspage_read_lock(struct zspage *zspage)
> +{
> +	atomic_t *lock = &zspage->lock;
> +	int old;
> +
> +	while (1) {
> +		old = atomic_read(lock);
> +		if (old == ZS_PAGE_WRLOCKED) {
> +			cpu_relax();
> +			continue;
> +		}
> +
> +		if (atomic_try_cmpxchg(lock, &old, old + 1))
> +			return;
> +
> +		cpu_relax();
> +	}
> +}

Please note that atomic_try_cmpxchg updates old variable on failure, so 
the whole loop can be rewritten as:

{
        atomic_t *lock = &zspage->lock;
        int old = atomic_read(lock);

        while (1) {
                if (old == ZS_PAGE_WRLOCKED) {
                        cpu_relax();
                        old = atomic_read(lock);
                        continue;
                }

                if (atomic_try_cmpxchg(lock, &old, old + 1))
                        return;

                cpu_relax();
        }
}

Please note that cpu_relax() in the cmpxchg() loop is actually harmful 
[1] because:

--q--
On the x86-64 architecture even a failing cmpxchg grants exclusive
access to the cacheline, making it preferable to retry the failed op
immediately instead of stalling with the pause instruction.
--/q--

[1]
https://lore.kernel.org/all/20230113184447.1707316-1-mjguzik@xxxxxxxxx/

Based on the above, cpu_relax() should be removed from the loop, which 
becomes:

{
        atomic_t *lock = &zspage->lock;
        int old = atomic_read(lock);

        do {
                if (old == ZS_PAGE_WRLOCKED) {
                        cpu_relax();
                        old = atomic_read(lock);
                        continue;
                }

        } while (!atomic_try_cmpxchg(lock, &old, old + 1));
 }

> +static int zspage_try_write_lock(struct zspage *zspage)

This function can be declared as bool, returning true/false.

Uros.