Re: Are jump labels broken on 6.11-rc1?

[Thomas Gleixner <tglx@linutronix.de>]

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