On Fri, 2013-06-28 at 22:30 +0000, jba...@akamai.com wrote:
> As pointed out by Andi Kleen, some static key users can be racy because they
> check the value of the key->enabled, and then subsequently update the branch
> direction. A number of call sites have 'higher' level locking that avoids this
> race, but the usage in the scheduler features does not. See:
> http://lkml.indiana.edu/hypermail/linux/kernel/1304.2/01655.html
>
> Thus, introduce a new API that does the check and set under the
> 'jump_label_mutex'. This should also allow to simplify call sites a bit.
>
> Users of static keys should use either the inc/dec or the set_true/set_false
> API.
>
> Signed-off-by: Jason Baron <jba...@akamai.com>
> ---
> Documentation/static-keys.txt | 8 ++++++++
> include/linux/jump_label.h | 30 ++++++++++++++++++++++++++++++
> kernel/jump_label.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+)
>
> diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt
> index 9f5263d..4cca941 100644
> --- a/Documentation/static-keys.txt
> +++ b/Documentation/static-keys.txt
> @@ -134,6 +134,14 @@ An example usage in the kernel is the implementation of
> tracepoints:
> TP_CONDITION(cond)); \
> }
>
> +Keys can also be updated with the following calls:
I would explain this a bit more. Something like:
When dealing with a simple on/off switch, where there's not a usage
counter involved with the static_key, the
static_key_slow_set_true/false() methods should be used.
> +
> + static_key_slow_set_true(struct static_key *key);
> + static_key_slow_set_false(struct static_key *key);
> +
> +Users should of the API should not mix the inc/dec with usages
> +of set_true/set_false. That is, users should choose one or the other.
Fix the above comment.
-- Steve
> +
> Tracepoints are disabled by default, and can be placed in performance
> critical
> pieces of the kernel. Thus, by using a static key, the tracepoints can have
> absolutely minimal impact when not in use.
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 0976fc4..787ab73 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -67,6 +67,11 @@ struct static_key_deferred {
> struct delayed_work work;
> };
>
> +/* for use with set_true/set_false API */
> +struct static_key_boolean {
> + struct static_key key;
> +};
> +
> # include <asm/jump_label.h>
> # define HAVE_JUMP_LABEL
> #endif /* CC_HAVE_ASM_GOTO && CONFIG_JUMP_LABEL */
> @@ -120,6 +125,8 @@ extern int jump_label_text_reserved(void *start, void
> *end);
> extern void static_key_slow_inc(struct static_key *key);
> extern void static_key_slow_dec(struct static_key *key);
> extern void static_key_slow_dec_deferred(struct static_key_deferred *key);
> +extern void static_key_slow_set_true(struct static_key_boolean *key);
> +extern void static_key_slow_set_false(struct static_key_boolean *key);
> extern void jump_label_apply_nops(struct module *mod);
> extern void
> jump_label_rate_limit(struct static_key_deferred *key, unsigned long rl);
> @@ -128,6 +135,10 @@ jump_label_rate_limit(struct static_key_deferred *key,
> unsigned long rl);
> { .enabled = ATOMIC_INIT(1), .entries = (void *)1 })
> #define STATIC_KEY_INIT_FALSE ((struct static_key) \
> { .enabled = ATOMIC_INIT(0), .entries = (void *)0 })
> +#define STATIC_KEY_BOOLEAN_INIT_TRUE ((struct static_key_boolean) \
> + { .key.enabled = ATOMIC_INIT(1), .key.entries = (void *)1 })
> +#define STATIC_KEY_BOOLEAN_INIT_FALSE ((struct static_key_boolean) \
> + { .key.enabled = ATOMIC_INIT(0), .key.entries = (void *)0 })
>
> #else /* !HAVE_JUMP_LABEL */
>
> @@ -137,6 +148,11 @@ struct static_key {
> atomic_t enabled;
> };
>
> +/* for use with set_true/set_false API */
> +struct static_key_boolean {
> + struct static_key key;
> +};
> +
> static __always_inline void jump_label_init(void)
> {
> }
> @@ -174,6 +190,16 @@ static inline void static_key_slow_dec_deferred(struct
> static_key_deferred *key)
> static_key_slow_dec(&key->key);
> }
>
> +static inline void static_key_slow_set_true(struct static_key_boolean *key)
> +{
> + atomic_set(&key->key.enabled, 1);
> +}
> +
> +static inline void static_key_slow_set_false(struct static_key_boolean *key)
> +{
> + atomic_set(&key->key.enabled, 0);
> +}
> +
> static inline int jump_label_text_reserved(void *start, void *end)
> {
> return 0;
> @@ -197,6 +223,10 @@ jump_label_rate_limit(struct static_key_deferred *key,
> { .enabled = ATOMIC_INIT(1) })
> #define STATIC_KEY_INIT_FALSE ((struct static_key) \
> { .enabled = ATOMIC_INIT(0) })
> +#define STATIC_KEY_BOOLEAN_INIT_TRUE ((struct static_key_boolean) \
> + { .key.enabled = ATOMIC_INIT(1) })
> +#define STATIC_KEY_BOOLEAN_INIT_FALSE ((struct static_key_boolean) \
> + { .key.enabled = ATOMIC_INIT(0) })
>
> #endif /* HAVE_JUMP_LABEL */
>
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 60f48fa..2234a4c 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -120,6 +120,46 @@ void jump_label_rate_limit(struct static_key_deferred
> *key,
> }
> EXPORT_SYMBOL_GPL(jump_label_rate_limit);
>
> +void static_key_slow_set_true(struct static_key_boolean *key_boolean)
> +{
> + struct static_key *key = (struct static_key *)key_boolean;
> + int enabled;
> +
> + jump_label_lock();
> + enabled = atomic_read(&key->enabled);
> + if (enabled == 1)
> + goto out;
> + WARN(enabled != 0, "%s: key->enabled: %d\n", __func__, enabled);
> + if (!jump_label_get_branch_default(key))
> + jump_label_update(key, JUMP_LABEL_ENABLE);
> + else
> + jump_label_update(key, JUMP_LABEL_DISABLE);
> + atomic_set(&key->enabled, 1);
> +out:
> + jump_label_unlock();
> +}
> +EXPORT_SYMBOL_GPL(static_key_slow_set_true);
> +
> +void static_key_slow_set_false(struct static_key_boolean *key_boolean)
> +{
> + struct static_key *key = (struct static_key *)key_boolean;
> + int enabled;
> +
> + jump_label_lock();
> + enabled = atomic_read(&key->enabled);
> + if (enabled == 0)
> + goto out;
> + WARN(enabled != 1, "%s: key->enabled: %d\n", __func__, enabled);
> + if (!jump_label_get_branch_default(key))
> + jump_label_update(key, JUMP_LABEL_DISABLE);
> + else
> + jump_label_update(key, JUMP_LABEL_ENABLE);
> + atomic_set(&key->enabled, 0);
> +out:
> + jump_label_unlock();
> +}
> +EXPORT_SYMBOL_GPL(static_key_slow_set_false);
> +
> static int addr_conflict(struct jump_entry *entry, void *start, void *end)
> {
> if (entry->code <= (unsigned long)end &&
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
