Re: [PATCH v14 01/15] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Deferred time check causes massive timeout multiplication on architectures with precise waits.
> - [High] Unchecked negative initial timeout leads to an infinite hardware sleep.
> - [High] Generic local variable names in the macro cause dangerous variable shadowing.
> - [Medium] Missing parentheses around the `timeout_ns` macro argument.
> --
>
> --- Patch [1]: [PATCH v14 01/15] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() ---
> commit fd56d26a441207368c7c3704e10f44093698b198
> Author: Ankur Arora <[email protected]>
> Subject: asm-generic: barrier: Add smp_cond_load_relaxed_timeout()
>
> This patch introduces smp_cond_load_relaxed_timeout, extending the relaxed
> condition loader with a timeout mechanism. It implements a loop with
> cpu_poll_relax and defers time checks to a slowpath to minimize performance
> overhead.
>
>> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
>> index d4f581c1e21da..ecdaddbbd7b64 100644
>> --- a/include/asm-generic/barrier.h
>> +++ b/include/asm-generic/barrier.h
> [ ... ]
>> +#ifndef smp_cond_load_relaxed_timeout
>> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr,			\
>> +				      time_expr_ns, timeout_ns)		\
>> +({									\
>> +	typeof(ptr) __PTR = (ptr);					\
>> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
>> +	u32 __count = 0, __spin = SMP_TIMEOUT_POLL_COUNT;		\
>
> [Severity: High]
> Could these generic variable names cause variable shadowing issues?
>
> If a caller passes an expression for time_expr_ns or cond_expr that uses a
> variable named __count or __timeout, would it evaluate using the macro's
> internal variables instead of the caller's? For example, if the caller's
> timeout variable is named __timeout, might it initialize the macro's local
> variable with its own uninitialized value?

process/coding-style.rst does mention this case:

 5) namespace collisions when defining local variables in macros resembling
 functions:
   ...
  ret is a common name for a local variable - __foo_ret is less likely
  to collide with an existing variable.

This has an __ prefix, but I suppose a collision is possible. Will fix.

>> +	s64 __timeout = (s64)timeout_ns;				\
>
> [Severity: Medium]
> Is it possible for operator precedence to cause incorrect timeout calculations
> here?
>
> Since the timeout_ns macro argument is not enclosed in parentheses during the
> cast, if a caller passes an expression like a << b, the cast would apply only
> to the first operand instead of the entire expression.

Had missed this. Will fix.

>> +	s64 __time_now, __time_end = 0;					\
>> +									\
>> +	for (;;) {							\
>> +		VAL = READ_ONCE(*__PTR);				\
>> +		if (cond_expr)						\
>> +			break;						\
>> +		cpu_poll_relax(__PTR, VAL, (u64)__timeout);		\
>
> [Severity: High]
> What happens if the initial timeout_ns passed by the caller is zero or
> negative?
>
> Because the time validation is deferred for SMP_TIMEOUT_POLL_COUNT iterations,
> a negative initial timeout is cast to u64 on the very first loop iteration. On
> architectures that implement a precise hardware wait, could this extremely
> large unsigned value cause an infinite hardware sleep instead of returning
> immediately?

If an arch implements a precise hardware wait it doesn't make sense for it
to define SMP_TIMEOUT_POLL_COUNT to be anything but 1.

If the caller calls with timeout_ns=0, then in general it is architecture
defined if it results in an arbitray wait or immediate return.
(On arm64 it will result in an immediate return with WFET, and up to
100us delay with WFE.)

If the caller uses timeout_ns < 0, then this will result in a long sleep.

IMO something like this is best addressed at code review instead of adding
unnecessary checks in potentially fast path code.

However, in the typical case of constant value of timeout_ns, there's no
runtime cost to the check. So, I'll add a domain check at the top level.

That should remove a bunch of sashiko comments.

>> +		if (++__count < __spin)					\
>> +			continue;					\
>
> [Severity: High]
> Does this loop inadvertently multiply the wait time on architectures with
> precise waits?
>
> Since __timeout is repeatedly passed to cpu_poll_relax without being
> decremented inside the SMP_TIMEOUT_POLL_COUNT deferral loop, if the CPU wakes
> up spuriously, it appears it will sleep again for the full initial timeout
> duration up to 200 times. Could this cause significant latency spikes?

In theory, this could happen. However, as the comment above
cpu_poll_relax() says:

   /*
    * cpu_poll_relax() stitches up two kinds of primitives: ones that provide
    * a momentary blip in the pipeline (ex. cpu_relax() on x86).
    * The second support waiting for @ptr value to change, coupled with a
    * with a precise (or imprecise) timeout.
    *
    * cpu_poll_relax() keeps them together, because its utility is in minimizing
    * expensive operations while polling on @ptr waiting for it to change.
    * The arguments to cpu_poll_relax() are only needed for the waiting
    * primitives.
    * ... */

So, for cases where the arch implements a timeout it doesn't make sense
for it to define SMP_TIMEOUT_POLL_COUNT to be anything but 1.

That said, the deferral of the time-check (to ensure we don't pay a cost
in the fast path ex. the locking path in rqspinlock) will cause a delay
(potentially up to doubling the timeout.) That could be fixed with an
alternative like the one I posted in:
  https://lore.kernel.org/all/[email protected]/

However, after the discussion with David Laight I came to the view that
it just overcomplicates the implementation for no real gain.

A better fix is to just document that in the worst cae me might end up
waiting for double the timeout (this is documented in the commit message).

--
ankur