Messages in this thread | | | Date | Wed, 23 Nov 2022 10:55:28 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v5 1/5] jump_label: Prevent key->enabled int overflow |
| |
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!
| |