Re: [PATCH 5/5] locking/ww_mutex/test: Time an ABBA with both owners running

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] locking/ww_mutex/test: missing stage ordering in
  test_abba_spin()
--

commit a75841edff4ce8ecc91baad12d6e7f770d7d72f7
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

locking/ww_mutex/test: Time an ABBA with both owners running

This commit adds a wound-wait test case to test-ww_mutex where two
SCHED_FIFO threads on separate CPUs create an ABBA contention while both
owners remain running.

> diff --git a/kernel/locking/test-ww_mutex.c b/kernel/locking/test-ww_mutex.c
> index 838d631544ed2..1bf3a91adfec5 100644
> --- a/kernel/locking/test-ww_mutex.c
> +++ b/kernel/locking/test-ww_mutex.c
[ ... ]
> @@ -279,6 +281,213 @@ static int test_abba(struct ww_class *class, bool trylock, bool resolve)
>  	return ret;
>  }
>  
[ ... ]
> +static bool abba_spin_until(struct test_abba_spin *s, int stage)
> +{
> +	u64 end = ktime_get_ns() + 5ULL * NSEC_PER_SEC;
> +
> +	while (atomic_read(&s->stage) < stage) {
> +		if (ktime_get_ns() > end)
> +			return false;
> +		cpu_relax();
> +	}
> +	return true;
> +}
[ ... ]
> +static int abba_spin_old(void *arg)
> +{
> +	struct test_abba_spin *s = arg;
> +	struct ww_acquire_ctx ctx;
> +	u64 t, end;
> +	int err = -ETIMEDOUT;
> +
> +	sched_set_fifo_low(current);
> +	wait_for_completion(&s->start);
> +	ww_acquire_init_noinject(&ctx, s->class);
> +	atomic_set(&s->stage, 1);
[ ... ]
> +static int abba_spin_young(void *arg)
> +{
> +	struct test_abba_spin *s = arg;
> +	struct ww_acquire_ctx ctx;
> +	int err = -ETIMEDOUT;
> +
> +	sched_set_fifo_low(current);
> +	wait_for_completion(&s->start);
> +	if (!abba_spin_until(s, 1))
> +		goto out;
> +	ww_acquire_init_noinject(&ctx, s->class);

[Severity: Low]
Can abba_spin_young() obtain a smaller ctx.stamp than abba_spin_old() on
weakly-ordered architectures?

ww_acquire_init() allocates ctx->stamp with
atomic_long_inc_return_relaxed(&ww_class->stamp), and s->stage is updated
and polled with relaxed atomic_set() and atomic_read():

CPU0 (abba_spin_old)              CPU1 (abba_spin_young)
ww_acquire_init_noinject()
  atomic_long_inc_return_relaxed(&s->class->stamp) [reordered]
atomic_set(&s->stage, 1)
                                  abba_spin_until(s, 1)
                                    atomic_read(&s->stage) == 1
                                  ww_acquire_init_noinject()
                                    atomic_long_inc_return_relaxed(...)
  [stamp increment executes here]  <-- young gets the older stamp

If abba_spin_young() receives the smaller stamp, it wounds
abba_spin_old() when blocking on s->a_mutex, causing abba_spin_old()'s
ww_mutex_lock(&s->b_mutex, &ctx) to fail with -EDEADLK.  Using
atomic_set_release() and atomic_read_acquire() on s->stage would order
the stamp allocations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106235931.1478800-1-mathieu.desnoyers@efficios.com?part=5