Messages in this thread | | | Date | Wed, 23 Nov 2022 14:11:59 +0000 | Subject | Re: [PATCH v5 1/5] jump_label: Prevent key->enabled int overflow | From | Dmitry Safonov <> |
| |
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
| |