Re: [RFC PATCH 1/4] hazptr: Add initial implementation of hazard pointers

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2024-09-27 16:43, Mathieu Desnoyers wrote:
> On 2024-09-27 12:59, Mathieu Desnoyers wrote:
>> On 2024-09-27 06:28, Boqun Feng wrote:
> [...]
>>> I replaced ADDRESS_EQ(a, b) with ADDRESS_EQ(b, a), and the compile
>>> result shows it can prevent the issue:
>>
>> I see, yes. It prevents the issue by making the compiler create
>> a copy of the value "modified" by the asm before doing the equality
>> comparison.
>>
>> This means the compiler cannot derive the value for b from the first
>> load when b is used after after the equality comparison.
>>
>> The only downside of OPTIMIZER_HIDE_VAR() is that it adds an extra
>> "mov" instruction to move the content across registers. I don't think
>> it matters performance wise though, so that solution is appealing
>> because it is arch-agnostic.
>>
>> One small improvement over your proposed solution would be to apply
>> OPTIMIZER_HIDE_VAR() on both inputs. Because this is not a volatile
>> asm, it is simply optimized away if var1 or var2 is unused following
>> the equality comparison. It is more convenient to prevent replacement
>> of both addresses being compared by the other rather than providing
>> the guarantee only on a single parameter:
> 
> Actually, your approach is better (only preserving the address
> dependency on the first parameter), because it allows the second
> parameter to be a constant.
> 
> Here is a diff. Please let me know if I need to improve anything wrt
> comments or implementation:
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 2df665fa2964..52434eccd715 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -186,6 +186,32 @@ void ftrace_likely_update(struct ftrace_likely_data 
> *f, int val,
>       __asm__ ("" : "=r" (var) : "0" (var))
>   #endif
> 
> +/*
> + * Compare an address with an expression while preserving the address
> + * dependencies for later use of the address. It should be used when
> + * comparing an address returned by rcu_dereference() with another
> + * address (either constant or in registers).
> + *
> + * This is needed to prevent the compiler SSA GVN optimization pass from
> + * replacing the register holding @addr by @expr (either a constant or a

Actually, I've done a bit on testing on godbolt, and it appears that
disabling this specific optimization makes the problem disappear:
-fcse-follow-jumps (by use of -fno-cse-follow-jumps).

I will update this comment to state that both CSE and SSA GVN
optimizations can cause issues there.

Thanks,

Mathieu

> + * register) based on their equality, which does not preserve address
> + * dependencies and allows the following misordering speculations:
> + *
> + * - If @expr is a constant, the compiler can issue the loads which depend
> + *   on @addr before the load of @addr.
> + * - If @expr is a register populated by a prior load, weakly-ordered
> + *   CPUs can speculate loads which depend on @addr before the load of the
> + *   address they depend on.
> + */
> +#ifndef ADDRESS_EQ
> +#define ADDRESS_EQ(addr, expr)                    \
> +    ({                            \
> +        bool __res = (addr) == (expr);            \
> +        OPTIMIZER_HIDE_VAR(addr);            \
> +        __res;                        \
> +    })
> +#endif
> +
>   #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), 
> __COUNTER__)
> 
>   /**
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com