On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
>
>> > + if (static_key_dec(key, true)) // dec-not-one
>>
>> Eeew.
>
> :-) I knew you'd hate on that
So you added it just to make me grumpy enough to fix it for you, right?
>> +/*
>> + * Fastpath: Decrement if the reference count is greater than one
>> + *
>> + * Returns false, if the reference count is 1 or -1 to force the caller
>> + * into the slowpath.
>> + *
>> + * The -1 case is to handle a decrement during a concurrent first enable,
>> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
>> + * slow path is serialized the caller will observe 1 once it acquired the
>> + * jump_label_mutex, so the slow path can succeed.
>> + */
>> +static bool static_key_dec_not_one(struct static_key *key)
>> +{
>> + int v = static_key_dec(key, true);
>> +
>> + return v != 1 && v != -1;
>
> if (v < 0)
> return false;
Hmm. I think we should do:
#define KEY_ENABLE_IN_PROGRESS -1
or even a more distinct value like (INT_MIN / 2)
and replace all the magic -1 numbers with it. Then the check becomes
explicit:
if (v == KEY_ENABLE_IN_PROGRESS)
return false;
> /*
> * Notably, 0 (underflow) returns true such that it bails out
> * without doing anything.
> */
> return v != 1;
>
> Perhaps?
Sure.
>> +}
>> +
>> +/*
>> + * Slowpath: Decrement and test whether the refcount hit 0.
>> + *
>> + * Returns true if the refcount hit zero, i.e. the previous value was one.
>> + */
>> +static bool static_key_dec_and_test(struct static_key *key)
>> +{
>> + int v = static_key_dec(key, false);
>> +
>> + lockdep_assert_held(&jump_label_mutex);
>> + return v == 1;
>> }
>
> But yeah, this is nicer!
:)
Thanks,
tglx
