| Message ID | 20241119153502.41361-7-vschneid@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | context_tracking,x86: Defer some IPIs until a user->kernel transition | expand |
On Tue, Nov 19, 2024 at 04:34:53PM +0100, Valentin Schneider wrote: > Later commits will cause objtool to warn about non __ro_after_init static > keys being used in .noinstr sections in order to safely defer instruction > patching IPIs targeted at NOHZ_FULL CPUs. Don't we need similar checking for static calls? > Two such keys currently exist: mds_idle_clear and __sched_clock_stable, > which can both be modified at runtime. Not sure if feasible, but it sure would be a lot simpler to just make "no noinstr patching" a hard rule and then convert the above keys (or at least their noinstr-specific usage) to regular branches. Then "no noinstr patching" could be unilaterally enforced in text_poke_bp(). > diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h > index f5a2727ca4a9a..93e729545b941 100644 > --- a/include/linux/jump_label.h > +++ b/include/linux/jump_label.h > @@ -200,7 +200,8 @@ struct module; > #define JUMP_TYPE_FALSE 0UL > #define JUMP_TYPE_TRUE 1UL > #define JUMP_TYPE_LINKED 2UL > -#define JUMP_TYPE_MASK 3UL > +#define JUMP_TYPE_FORCEFUL 4UL JUMP_TYPE_NOINSTR_ALLOWED ?
On Tue, Nov 19, 2024 at 04:34:53PM +0100, Valentin Schneider wrote: > +++ b/include/linux/jump_label.h > @@ -200,7 +200,8 @@ struct module; > #define JUMP_TYPE_FALSE 0UL > #define JUMP_TYPE_TRUE 1UL > #define JUMP_TYPE_LINKED 2UL > -#define JUMP_TYPE_MASK 3UL > +#define JUMP_TYPE_FORCEFUL 4UL > +#define JUMP_TYPE_MASK 7UL Hm, I don't think we can (ab)use this pointer bit on 32-bit arches, as the address could be 4 byte aligned?
On Tue, Nov 19, 2024 at 04:05:32PM -0800, Josh Poimboeuf wrote: > On Tue, Nov 19, 2024 at 04:34:53PM +0100, Valentin Schneider wrote: > > +++ b/include/linux/jump_label.h > > @@ -200,7 +200,8 @@ struct module; > > #define JUMP_TYPE_FALSE 0UL > > #define JUMP_TYPE_TRUE 1UL > > #define JUMP_TYPE_LINKED 2UL > > -#define JUMP_TYPE_MASK 3UL > > +#define JUMP_TYPE_FORCEFUL 4UL > > +#define JUMP_TYPE_MASK 7UL > > Hm, I don't think we can (ab)use this pointer bit on 32-bit arches, as > the address could be 4 byte aligned? Right, you can force the alignment of the thing, workqueues do similar hacks to get more bits.
On Tue, Nov 19, 2024 at 03:39:02PM -0800, Josh Poimboeuf wrote: > On Tue, Nov 19, 2024 at 04:34:53PM +0100, Valentin Schneider wrote: > > Later commits will cause objtool to warn about non __ro_after_init static > > keys being used in .noinstr sections in order to safely defer instruction > > patching IPIs targeted at NOHZ_FULL CPUs. > > Don't we need similar checking for static calls? > > > Two such keys currently exist: mds_idle_clear and __sched_clock_stable, > > which can both be modified at runtime. > > Not sure if feasible, but it sure would be a lot simpler to just make > "no noinstr patching" a hard rule and then convert the above keys (or at > least their noinstr-specific usage) to regular branches. That'll be a bit of a mess. Also, then we're adding overhead/cost for all people for the benefit of this fringe case (NOHZ_FULL). Not a desirable trade-off IMO. So I do think the proposed solution (+- naming, I like your naming proposal better) is the better one. But I think we can make the fall-back safer, we can simply force the IPI when we poke at noinstr code -- then NOHZ_FULL gets to keep the pieces, but at least we don't violate any correctness constraints.
On Wed, Nov 20, 2024 at 03:56:49PM +0100, Peter Zijlstra wrote: > But I think we can make the fall-back safer, we can simply force the IPI > when we poke at noinstr code -- then NOHZ_FULL gets to keep the pieces, > but at least we don't violate any correctness constraints. I should have read more; that's what is being proposed.
On 19/11/24 15:39, Josh Poimboeuf wrote: > On Tue, Nov 19, 2024 at 04:34:53PM +0100, Valentin Schneider wrote: >> Later commits will cause objtool to warn about non __ro_after_init static >> keys being used in .noinstr sections in order to safely defer instruction >> patching IPIs targeted at NOHZ_FULL CPUs. > > Don't we need similar checking for static calls? > /sifts through my notes throwing paper all around Huh, I thought I had something, but no... Per the results they don't seem to be flipped around as much as static keys, but they also end up in text_poke_bp(), so yeah, we do. Welp, I'll add that to the list. >> Two such keys currently exist: mds_idle_clear and __sched_clock_stable, >> which can both be modified at runtime. > > Not sure if feasible, but it sure would be a lot simpler to just make > "no noinstr patching" a hard rule and then convert the above keys (or at > least their noinstr-specific usage) to regular branches. > > Then "no noinstr patching" could be unilaterally enforced in > text_poke_bp(). > >> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h >> index f5a2727ca4a9a..93e729545b941 100644 >> --- a/include/linux/jump_label.h >> +++ b/include/linux/jump_label.h >> @@ -200,7 +200,8 @@ struct module; >> #define JUMP_TYPE_FALSE 0UL >> #define JUMP_TYPE_TRUE 1UL >> #define JUMP_TYPE_LINKED 2UL >> -#define JUMP_TYPE_MASK 3UL >> +#define JUMP_TYPE_FORCEFUL 4UL > > JUMP_TYPE_NOINSTR_ALLOWED ? > That's better, I'll take it. Thanks! > -- > Josh
On Wed, Nov 20, 2024 at 03:57:46PM +0100, Peter Zijlstra wrote: > On Wed, Nov 20, 2024 at 03:56:49PM +0100, Peter Zijlstra wrote: > > > But I think we can make the fall-back safer, we can simply force the IPI > > when we poke at noinstr code -- then NOHZ_FULL gets to keep the pieces, > > but at least we don't violate any correctness constraints. > > I should have read more; that's what is being proposed. Hm, now I'm wondering what you read, as I only see the text poke IPIs being forced when the caller sets force_ipi, rather than the text poke code itself detecting a write to .noinstr.
On Wed, Nov 20, 2024 at 08:55:15AM -0800, Josh Poimboeuf wrote: > On Wed, Nov 20, 2024 at 03:57:46PM +0100, Peter Zijlstra wrote: > > On Wed, Nov 20, 2024 at 03:56:49PM +0100, Peter Zijlstra wrote: > > > > > But I think we can make the fall-back safer, we can simply force the IPI > > > when we poke at noinstr code -- then NOHZ_FULL gets to keep the pieces, > > > but at least we don't violate any correctness constraints. > > > > I should have read more; that's what is being proposed. > > Hm, now I'm wondering what you read, as I only see the text poke IPIs > being forced when the caller sets force_ipi, rather than the text poke > code itself detecting a write to .noinstr. Right, so I had much confusion and my initial thought was that it would do something dangerous. Then upon reading more I see it forces the IPI for these special keys -- with that force_ipi thing. Now, there's only two keys marked special, and both have a noinstr presence -- the entire reason they get marked. So effectively we force the IPI when patching noinstr, no? But yeah, this is not quite the same as not marking anything and simply forcing the IPI when the target address is noinstr. And having written all that; perhaps that is the better solution, it sticks the logic in text_poke and ensure it automagically work for all its users, obviating the need for special marking. Is that what you were thinking?
On Thu, Nov 21, 2024 at 12:00:20PM +0100, Peter Zijlstra wrote: > But yeah, this is not quite the same as not marking anything and simply > forcing the IPI when the target address is noinstr. > > And having written all that; perhaps that is the better solution, it > sticks the logic in text_poke and ensure it automagically work for all > its users, obviating the need for special marking. > > Is that what you were thinking? Yes, though I can't take credit for the idea as that's what I thought you were suggesting! That seems simpler and more bulletproof.
On 21/11/24 12:00, Peter Zijlstra wrote: > On Wed, Nov 20, 2024 at 08:55:15AM -0800, Josh Poimboeuf wrote: >> On Wed, Nov 20, 2024 at 03:57:46PM +0100, Peter Zijlstra wrote: >> > On Wed, Nov 20, 2024 at 03:56:49PM +0100, Peter Zijlstra wrote: >> > >> > > But I think we can make the fall-back safer, we can simply force the IPI >> > > when we poke at noinstr code -- then NOHZ_FULL gets to keep the pieces, >> > > but at least we don't violate any correctness constraints. >> > >> > I should have read more; that's what is being proposed. >> >> Hm, now I'm wondering what you read, as I only see the text poke IPIs >> being forced when the caller sets force_ipi, rather than the text poke >> code itself detecting a write to .noinstr. > > Right, so I had much confusion and my initial thought was that it would > do something dangerous. Then upon reading more I see it forces the IPI > for these special keys -- with that force_ipi thing. > > Now, there's only two keys marked special, and both have a noinstr > presence -- the entire reason they get marked. > > So effectively we force the IPI when patching noinstr, no? > > But yeah, this is not quite the same as not marking anything and simply > forcing the IPI when the target address is noinstr. > > And having written all that; perhaps that is the better solution, it > sticks the logic in text_poke and ensure it automagically work for all > its users, obviating the need for special marking. > Okay so forcing the IPI for .noinstr patching lets us get rid of all the force_ipi faff; however I would still want the special marking to tell objtool "yep we're okay with this one", and still get warnings when a new .noinstr key gets added so we double think about it.
On Thu, Nov 21, 2024 at 04:51:09PM +0100, Valentin Schneider wrote: > Okay so forcing the IPI for .noinstr patching lets us get rid of all the > force_ipi faff; however I would still want the special marking to tell > objtool "yep we're okay with this one", and still get warnings when a new > .noinstr key gets added so we double think about it. Yeah. Though, instead of DECLARE_STATIC_KEY_FALSE_NOINSTR adding a new jump label type, it could just add an objtool annotation pointing to the key. If that's the way we're going I could whip up a patch if that would help.
On 21/11/24 12:21, Josh Poimboeuf wrote: > On Thu, Nov 21, 2024 at 04:51:09PM +0100, Valentin Schneider wrote: >> Okay so forcing the IPI for .noinstr patching lets us get rid of all the >> force_ipi faff; however I would still want the special marking to tell >> objtool "yep we're okay with this one", and still get warnings when a new >> .noinstr key gets added so we double think about it. > > Yeah. Though, instead of DECLARE_STATIC_KEY_FALSE_NOINSTR adding a new > jump label type, it could just add an objtool annotation pointing to the > key. If that's the way we're going I could whip up a patch if that > would help. > Well I'm down for the approach and I'd appreciate help for the objtool side :-) > -- > Josh
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index f5a2727ca4a9a..93e729545b941 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -200,7 +200,8 @@ struct module; #define JUMP_TYPE_FALSE 0UL #define JUMP_TYPE_TRUE 1UL #define JUMP_TYPE_LINKED 2UL -#define JUMP_TYPE_MASK 3UL +#define JUMP_TYPE_FORCEFUL 4UL +#define JUMP_TYPE_MASK 7UL static __always_inline bool static_key_false(struct static_key *key) { @@ -244,12 +245,15 @@ extern enum jump_label_type jump_label_init_type(struct jump_entry *entry); * raw value, but have added a BUILD_BUG_ON() to catch any issues in * jump_label_init() see: kernel/jump_label.c. */ -#define STATIC_KEY_INIT_TRUE \ - { .enabled = { 1 }, \ - { .type = JUMP_TYPE_TRUE } } -#define STATIC_KEY_INIT_FALSE \ - { .enabled = { 0 }, \ - { .type = JUMP_TYPE_FALSE } } +#define __STATIC_KEY_INIT(_true, force) \ + { .enabled = { _true }, \ + { .type = (_true ? JUMP_TYPE_TRUE : JUMP_TYPE_FALSE) | \ + (force ? JUMP_TYPE_FORCEFUL : 0UL)} } + +#define STATIC_KEY_INIT_TRUE __STATIC_KEY_INIT(true, false) +#define STATIC_KEY_INIT_FALSE __STATIC_KEY_INIT(false, false) +#define STATIC_KEY_INIT_TRUE_FORCE __STATIC_KEY_INIT(true, true) +#define STATIC_KEY_INIT_FALSE_FORCE __STATIC_KEY_INIT(false, true) #else /* !CONFIG_JUMP_LABEL */ @@ -369,6 +373,8 @@ struct static_key_false { #define STATIC_KEY_TRUE_INIT (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE, } #define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, } +#define STATIC_KEY_TRUE_FORCE_INIT (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE_FORCE, } +#define STATIC_KEY_FALSE_FORCE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE_FORCE, } #define DEFINE_STATIC_KEY_TRUE(name) \ struct static_key_true name = STATIC_KEY_TRUE_INIT @@ -376,6 +382,9 @@ struct static_key_false { #define DEFINE_STATIC_KEY_TRUE_RO(name) \ struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT +#define DEFINE_STATIC_KEY_TRUE_FORCE(name) \ + struct static_key_true name = STATIC_KEY_TRUE_FORCE_INIT + #define DECLARE_STATIC_KEY_TRUE(name) \ extern struct static_key_true name @@ -385,6 +394,9 @@ struct static_key_false { #define DEFINE_STATIC_KEY_FALSE_RO(name) \ struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT +#define DEFINE_STATIC_KEY_FALSE_FORCE(name) \ + struct static_key_false name = STATIC_KEY_FALSE_FORCE_INIT + #define DECLARE_STATIC_KEY_FALSE(name) \ extern struct static_key_false name
Later commits will cause objtool to warn about non __ro_after_init static keys being used in .noinstr sections in order to safely defer instruction patching IPIs targeted at NOHZ_FULL CPUs. Two such keys currently exist: mds_idle_clear and __sched_clock_stable, which can both be modified at runtime. As discussed at LPC 2024 during the realtime micro-conference, modifying these specific keys incurs additional interference (SMT hotplug) or can even be downright incompatible with NOHZ_FULL (unstable TSC). Suppressing the IPI associated with modifying such keys is thus a minor concern wrt NOHZ_FULL interference, so add a jump type that will be leveraged by both the kernel (to know not to defer the IPI) and objtool (to know not to generate the aforementioned warning). Signed-off-by: Valentin Schneider <vschneid@redhat.com> --- include/linux/jump_label.h | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)