On Wed, Aug 07, 2024 at 04:55:53PM +0200, Thomas Gleixner wrote: > 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? FWIW with peter's 'ugly' patch applied, fstests didn't cough up any static key complaints overnight. > >> +/* > >> + * 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! > > :) It probably goes without saying that if either of you send a cleaned up patch with all these changes baked in, I will test it for you all. :) --D > > Thanks, > > tglx >