[PATCH v12 06/15] asm-generic: barrier: Add smp_cond_load_acquire_timeout()

[Ankur Arora]

sashiko-bot at kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ACQUIRE semantics violation in `smp_cond_load_acquire_timeout()` on weakly-ordered architectures.
> --
>
> commit 0c67207964772b92d28f5af7e4a97ea44a103e9b
> Author: Ankur Arora <ankur.a.arora at oracle.com>
>
> asm-generic: barrier: Add smp_cond_load_acquire_timeout()
>
> This commit adds the acquire variant of smp_cond_load_relaxed_timeout().
> It attempts to provide acquire ordering by reusing the relaxed variant and
> adding a control dependency combined with smp_acquire__after_ctrl_dep().
>
>> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
>> index c56df9513a084..0ab26e98842c0 100644
>> --- a/include/asm-generic/barrier.h
>> +++ b/include/asm-generic/barrier.h
>
> [ ... ]
>
>> +/**
>> + * smp_cond_load_acquire_timeout() - (Spin) wait for cond with ACQUIRE ordering
>> + * until a timeout expires.
>> + * @ptr: pointer to the variable to wait on.
>> + * @cond_expr: boolean expression to wait for.
>> + * @time_expr_ns: monotonic expression that evaluates to time in ns or,
>> + *  on failure, returns a negative value.
>> + * @timeout_ns: timeout value in ns
>> + * (Both of the above are assumed to be compatible with s64.)
>> + *
>> + * Equivalent to using smp_cond_load_acquire() on the condition variable with
>> + * a timeout.
>> + */
>> +#ifndef smp_cond_load_acquire_timeout
>> +#define smp_cond_load_acquire_timeout(ptr, cond_expr,			\
>> +				      time_expr_ns, timeout_ns)		\
>> +({									\
>> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
>> +	VAL = smp_cond_load_relaxed_timeout(ptr, cond_expr,		\
>> +					     time_expr_ns,		\
>> +					     timeout_ns);		\
>> +	if (cond_expr)							\
>> +		barrier();						\
>> +	smp_acquire__after_ctrl_dep();					\
>
> [Severity: High]
> Does this sequence correctly establish a control dependency for the caller's
> subsequent instructions?
>
> According to Documentation/memory-barriers.txt, control dependencies only
> apply to stores executed strictly within the conditional clauses (the true
> or false branches), not to code following the if statement. Since the
> caller's subsequent stores reside outside this conditional block, they
> appear unconditional to the CPU.

Yes, this is a problem. One way to fix this would be to add a full
acquire barrier in the failure path:

	VAL = smp_cond_load_relaxed_timeout(ptr, cond_expr,
					     time_expr_ns,
					     timeout_ns);
	if (!cond_expr)
		VAL = smp_load_acquire();
	else
                smp_acquire__after_ctrl_dep();
        (typeof(*(ptr)))VAL;

However, this seems like overkill since there are likely few situations
where we need an acquire barrier on the failure path.

AFAICT rqspinlock doesn't seem to need one.

Unless someone has an objection that's what I plan to do.

Ankur

> Furthermore, smp_acquire__after_ctrl_dep() typically falls back to
> smp_rmb(). On weakly-ordered architectures like RISC-V, where smp_rmb() is
> implemented as "fence r, r" and only enforces load-to-load ordering, could
> the CPU speculatively execute subsequent stores before the final load of VAL
> completes?
>
> Could this violate the expected acquire semantics if the caller relies on
> the barrier in the timeout or failure path?
>
>> +	(typeof(*(ptr)))VAL;						\
>> +})
>> +#endif


--
ankur