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