| Message ID | 1381987923-1524-4-git-send-email-hannes@stressinduktion.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 17 Oct 2013 07:31:57 +0200 Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > Based on a patch from Andi Kleen. I'm fine with the patch, but the change log needs a lot more work. Like, why is this needed? I know, but does anyone else? -- Steve > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Andi Kleen <andi@firstfloor.org> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > include/linux/jump_label.h | 10 ++++++++++ > include/linux/jump_label_ratelimit.h | 2 ++ > init/main.c | 7 +++++++ > kernel/jump_label.c | 5 +++++ > 4 files changed, 24 insertions(+) > > diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h > index a507907..e96be72 100644 > --- a/include/linux/jump_label.h > +++ b/include/linux/jump_label.h > @@ -48,6 +48,13 @@ > > #include <linux/types.h> > #include <linux/compiler.h> > +#include <linux/bug.h> > + > +extern bool static_key_initialized; > + > +#define STATIC_KEY_CHECK_USE() WARN(!static_key_initialized, \ > + "%s used before call to jump_label_init", \ > + __func__) > > #if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL) > > @@ -128,6 +135,7 @@ struct static_key { > > static __always_inline void jump_label_init(void) > { > + static_key_initialized = true; > } > > static __always_inline bool static_key_false(struct static_key *key) > @@ -146,11 +154,13 @@ static __always_inline bool static_key_true(struct static_key *key) > > static inline void static_key_slow_inc(struct static_key *key) > { > + STATIC_KEY_CHECK_USE(); > atomic_inc(&key->enabled); > } > > static inline void static_key_slow_dec(struct static_key *key) > { > + STATIC_KEY_CHECK_USE(); > atomic_dec(&key->enabled); > } > > diff --git a/include/linux/jump_label_ratelimit.h b/include/linux/jump_label_ratelimit.h > index 1137883..089f70f 100644 > --- a/include/linux/jump_label_ratelimit.h > +++ b/include/linux/jump_label_ratelimit.h > @@ -23,12 +23,14 @@ struct static_key_deferred { > }; > static inline void static_key_slow_dec_deferred(struct static_key_deferred *key) > { > + STATIC_KEY_CHECK_USE(); > static_key_slow_dec(&key->key); > } > static inline void > jump_label_rate_limit(struct static_key_deferred *key, > unsigned long rl) > { > + STATIC_KEY_CHECK_USE(); > } > #endif /* HAVE_JUMP_LABEL */ > #endif /* _LINUX_JUMP_LABEL_RATELIMIT_H */ > diff --git a/init/main.c b/init/main.c > index af310af..27bbec1a 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -136,6 +136,13 @@ static char *execute_command; > static char *ramdisk_execute_command; > > /* > + * Used to generate warnings if static_key manipulation functions are used > + * before jump_label_init is called. > + */ > +bool static_key_initialized __read_mostly = false; > +EXPORT_SYMBOL_GPL(static_key_initialized); > + > +/* > * If set, this is an indication to the drivers that reset the underlying > * device before going ahead with the initialization otherwise driver might > * rely on the BIOS and skip the reset operation. > diff --git a/kernel/jump_label.c b/kernel/jump_label.c > index 297a924..9019f15 100644 > --- a/kernel/jump_label.c > +++ b/kernel/jump_label.c > @@ -58,6 +58,7 @@ static void jump_label_update(struct static_key *key, int enable); > > void static_key_slow_inc(struct static_key *key) > { > + STATIC_KEY_CHECK_USE(); > if (atomic_inc_not_zero(&key->enabled)) > return; > > @@ -103,12 +104,14 @@ static void jump_label_update_timeout(struct work_struct *work) > > void static_key_slow_dec(struct static_key *key) > { > + STATIC_KEY_CHECK_USE(); > __static_key_slow_dec(key, 0, NULL); > } > EXPORT_SYMBOL_GPL(static_key_slow_dec); > > void static_key_slow_dec_deferred(struct static_key_deferred *key) > { > + STATIC_KEY_CHECK_USE(); > __static_key_slow_dec(&key->key, key->timeout, &key->work); > } > EXPORT_SYMBOL_GPL(static_key_slow_dec_deferred); > @@ -116,6 +119,7 @@ EXPORT_SYMBOL_GPL(static_key_slow_dec_deferred); > void jump_label_rate_limit(struct static_key_deferred *key, > unsigned long rl) > { > + STATIC_KEY_CHECK_USE(); > key->timeout = rl; > INIT_DELAYED_WORK(&key->work, jump_label_update_timeout); > } > @@ -212,6 +216,7 @@ void __init jump_label_init(void) > key->next = NULL; > #endif > } > + static_key_initialized = true; > jump_label_unlock(); > } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 17, 2013 at 05:19:40PM -0400, Steven Rostedt wrote: > On Thu, 17 Oct 2013 07:31:57 +0200 > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > > Based on a patch from Andi Kleen. > > I'm fine with the patch, but the change log needs a lot more work. > Like, why is this needed? I know, but does anyone else? Ok. :) I'll move the description from the changelog here and write something in a few days (hope to get more feedback on the other parts, especially net_get_random_once). Does that also mean you are in principle ok with the patch weakening the check in arch/x86/jump_label.c? Thanks for the review, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 17, 2013 at 05:19:40PM -0400, Steven Rostedt wrote: > On Thu, 17 Oct 2013 07:31:57 +0200 > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > > Based on a patch from Andi Kleen. > > I'm fine with the patch, but the change log needs a lot more work. > Like, why is this needed? I know, but does anyone else? Or just merge the orininal patch. While being less verbose in the warning it had a full changelog. -Andi -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 18, 2013 at 02:50:57AM +0200, Andi Kleen wrote: > On Thu, Oct 17, 2013 at 05:19:40PM -0400, Steven Rostedt wrote: > > On Thu, 17 Oct 2013 07:31:57 +0200 > > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > > > > Based on a patch from Andi Kleen. > > > > I'm fine with the patch, but the change log needs a lot more work. > > Like, why is this needed? I know, but does anyone else? > > Or just merge the orininal patch. While being less verbose > in the warning it had a full changelog. I don't care that much which patch gets merged but I just want to point out another difference: I added WARN_ONs on code paths without architecture support for static_keys to help developers on platforms without support for that. Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index a507907..e96be72 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -48,6 +48,13 @@ #include <linux/types.h> #include <linux/compiler.h> +#include <linux/bug.h> + +extern bool static_key_initialized; + +#define STATIC_KEY_CHECK_USE() WARN(!static_key_initialized, \ + "%s used before call to jump_label_init", \ + __func__) #if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL) @@ -128,6 +135,7 @@ struct static_key { static __always_inline void jump_label_init(void) { + static_key_initialized = true; } static __always_inline bool static_key_false(struct static_key *key) @@ -146,11 +154,13 @@ static __always_inline bool static_key_true(struct static_key *key) static inline void static_key_slow_inc(struct static_key *key) { + STATIC_KEY_CHECK_USE(); atomic_inc(&key->enabled); } static inline void static_key_slow_dec(struct static_key *key) { + STATIC_KEY_CHECK_USE(); atomic_dec(&key->enabled); } diff --git a/include/linux/jump_label_ratelimit.h b/include/linux/jump_label_ratelimit.h index 1137883..089f70f 100644 --- a/include/linux/jump_label_ratelimit.h +++ b/include/linux/jump_label_ratelimit.h @@ -23,12 +23,14 @@ struct static_key_deferred { }; static inline void static_key_slow_dec_deferred(struct static_key_deferred *key) { + STATIC_KEY_CHECK_USE(); static_key_slow_dec(&key->key); } static inline void jump_label_rate_limit(struct static_key_deferred *key, unsigned long rl) { + STATIC_KEY_CHECK_USE(); } #endif /* HAVE_JUMP_LABEL */ #endif /* _LINUX_JUMP_LABEL_RATELIMIT_H */ diff --git a/init/main.c b/init/main.c index af310af..27bbec1a 100644 --- a/init/main.c +++ b/init/main.c @@ -136,6 +136,13 @@ static char *execute_command; static char *ramdisk_execute_command; /* + * Used to generate warnings if static_key manipulation functions are used + * before jump_label_init is called. + */ +bool static_key_initialized __read_mostly = false; +EXPORT_SYMBOL_GPL(static_key_initialized); + +/* * If set, this is an indication to the drivers that reset the underlying * device before going ahead with the initialization otherwise driver might * rely on the BIOS and skip the reset operation. diff --git a/kernel/jump_label.c b/kernel/jump_label.c index 297a924..9019f15 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -58,6 +58,7 @@ static void jump_label_update(struct static_key *key, int enable); void static_key_slow_inc(struct static_key *key) { + STATIC_KEY_CHECK_USE(); if (atomic_inc_not_zero(&key->enabled)) return; @@ -103,12 +104,14 @@ static void jump_label_update_timeout(struct work_struct *work) void static_key_slow_dec(struct static_key *key) { + STATIC_KEY_CHECK_USE(); __static_key_slow_dec(key, 0, NULL); } EXPORT_SYMBOL_GPL(static_key_slow_dec); void static_key_slow_dec_deferred(struct static_key_deferred *key) { + STATIC_KEY_CHECK_USE(); __static_key_slow_dec(&key->key, key->timeout, &key->work); } EXPORT_SYMBOL_GPL(static_key_slow_dec_deferred); @@ -116,6 +119,7 @@ EXPORT_SYMBOL_GPL(static_key_slow_dec_deferred); void jump_label_rate_limit(struct static_key_deferred *key, unsigned long rl) { + STATIC_KEY_CHECK_USE(); key->timeout = rl; INIT_DELAYED_WORK(&key->work, jump_label_update_timeout); } @@ -212,6 +216,7 @@ void __init jump_label_init(void) key->next = NULL; #endif } + static_key_initialized = true; jump_label_unlock(); }
Based on a patch from Andi Kleen. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Andi Kleen <andi@firstfloor.org> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- include/linux/jump_label.h | 10 ++++++++++ include/linux/jump_label_ratelimit.h | 2 ++ init/main.c | 7 +++++++ kernel/jump_label.c | 5 +++++ 4 files changed, 24 insertions(+)