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

[Peter Zijlstra]

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.

> +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 )

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() ?

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



Otherwise, things look good.

Thanks!