| Message ID | 20241119153502.41361-7-vschneid@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v3,01/15] objtool: Make validate_call() recognize indirect calls to pv_ops[] | expand |
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 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: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(-)