Re: [PATCH v5 1/5] jump_label: Prevent key->enabled int overflow

[Dmitry Safonov <dima@arista.com>]

On 11/23/22 09:55, Peter Zijlstra wrote:
> On Tue, Nov 22, 2022 at 06:55:30PM +0000, Dmitry Safonov wrote:
> 
>> +/***
>> + * static_key_fast_inc_not_negative - adds a user for a static key
>> + * @key: static key that must be already enabled
>> + *
>> + * The caller must make sure that the static key can't get disabled while
>> + * in this function. It doesn't patch jump labels, only adds a user to
>> + * an already enabled static key.
>> + *
>> + * Returns true if the increment was done.
>> + */
> 
> I don't normally do kerneldoc style comments, and this is the first in
> the whole file. The moment I get a docs person complaining about some
> markup issue I just take the ** off.

The only reason I used kerneldoc style is that otherwise usually someone
would come and complain. I'll convert it to a regular comment.

> One more thing; it might be useful to point out that unlike refcount_t
> this thing does not saturate but will fail to increment on overflow.

Will add it as well.

> 
>> +static bool static_key_fast_inc_not_negative(struct static_key *key)
>>  {
>> +	int v;
>> +
>>  	STATIC_KEY_CHECK_USE(key);
>> +	/*
>> +	 * Negative key->enabled has a special meaning: it sends
>> +	 * static_key_slow_inc() down the slow path, and it is non-zero
>> +	 * so it counts as "enabled" in jump_label_update().  Note that
>> +	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
>> +	 */
>> +	v = atomic_read(&key->enabled);
>> +	do {
>> +		if (v <= 0 || (v + 1) < 0)
>> +			return false;
>> +	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v + 1)));
>> +
>> +	return true;
>> +}
> 
> ( vexing how this function and the JUMP_LABEL=n static_key_slow_inc() are
>   only a single character different )

Yeah, also another reason for it was that when JUMP_LABEL=y jump_label.h
doesn't include <linux/atomic.h> and <linux/bug.h> because of the
inclusion hell:
commit 1f69bf9c6137 ("jump_label: remove bug.h, atomic.h dependencies
for HAVE_JUMP_LABEL")
and I can't move JUMP_LABEL=n version of static_key_slow_inc() to
jump_label.c as it is not being built without the config set.

So, in result I was looking into macro-define for both cases, but that
adds quite some ugliness and has no type checks for just reusing 10
lines, where 1 differs...

> So while strictly accurate, I dislike this name (and I see I was not
> quick enough responding to your earlier suggestion :/). The whole
> negative thing is an implementation detail that should not spread
> outside of jump_label.c.
> 
> Since you did not like the canonical _inc_not_zero(), how about
> inc_not_disabled() ?

Ok, that sounds good, I'll rename in v6.

> Also, perhaps expose this function in this patch, instead of hiding that
> in patch 3?

Will do.

> Otherwise, things look good.
> 
> Thanks!
Thanks again for the review,
          Dmitry