* [PATCH 0/3] jump_labels: concurrency fixes
@ 2017-08-01 15:24 Paolo Bonzini
2017-08-01 15:24 ` [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Paolo Bonzini @ 2017-08-01 15:24 UTC (permalink / raw)
To: linux-kernel; +Cc: Peter Zijlstra
Any use of key->enabled (that is static_key_enabled and static_key_count)
outside jump_label_lock should handle its own serialization. Notably
among those that don't do so is static_key_enable/disable itself. Once
those are fixed (patch 1), they can be used to fix the actual offenders in
net/ (patch 2).
Patch 3 is not fixing anything, but the function cannot be used correctly
from any place other than cpuset.c itself so it is moved there.
Based on discussion with Peter Zijlstra.
Paolo
Paolo Bonzini (3):
jump_labels: fix concurrent static_key_enable/disable()
jump_labels: do not use unserialized static_key_enabled
cpuset: make nr_cpusets private
Documentation/static-keys.txt | 5 ++++
include/linux/cpuset.h | 6 -----
include/linux/jump_label.h | 22 ++++++++--------
kernel/cgroup/cpuset.c | 7 +++++
kernel/jump_label.c | 59 +++++++++++++++++++++++++++----------------
net/ipv4/udp.c | 3 +--
net/ipv6/udp.c | 3 +--
7 files changed, 63 insertions(+), 42 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() 2017-08-01 15:24 [PATCH 0/3] jump_labels: concurrency fixes Paolo Bonzini @ 2017-08-01 15:24 ` Paolo Bonzini 2017-08-01 16:45 ` Peter Zijlstra 2017-08-10 12:12 ` [tip:locking/core] jump_label: Fix " tip-bot for Paolo Bonzini 2017-08-01 15:24 ` [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled Paolo Bonzini 2017-08-01 15:24 ` [PATCH 3/3] cpuset: make nr_cpusets private Paolo Bonzini 2 siblings, 2 replies; 12+ messages in thread From: Paolo Bonzini @ 2017-08-01 15:24 UTC (permalink / raw) To: linux-kernel; +Cc: Peter Zijlstra static_key_enable/disable are trying to cap the static key count to 0/1. However, their use of key->enabled is outside jump_label_lock so they do not really ensure that. Rewrite them to do a quick check for an already enabled (respectively, already disabled), and then recheck under the jump label lock. Unlike static_key_slow_inc/dec, a failed check under the jump label lock does not modify key->enabled. Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- include/linux/jump_label.h | 22 +++++++++-------- kernel/jump_label.c | 59 +++++++++++++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index 2afd74b9d844..740a42ea7f7f 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -234,22 +234,24 @@ static inline int jump_label_apply_nops(struct module *mod) static inline void static_key_enable(struct static_key *key) { - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); + STATIC_KEY_CHECK_USE(); - if (!count) - static_key_slow_inc(key); + if (atomic_read(&key->enabled) != 0) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 1); + return; + } + atomic_set(&key->enabled, 1); } static inline void static_key_disable(struct static_key *key) { - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); + STATIC_KEY_CHECK_USE(); - if (count) - static_key_slow_dec(key); + if (atomic_read(&key->enabled) != 1) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 0); + return; + } + atomic_set(&key->enabled, 0); } #define STATIC_KEY_INIT_TRUE { .enabled = ATOMIC_INIT(1) } diff --git a/kernel/jump_label.c b/kernel/jump_label.c index 6c9cb208ac48..04a574fd5844 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -78,28 +78,6 @@ int static_key_count(struct static_key *key) } EXPORT_SYMBOL_GPL(static_key_count); -void static_key_enable(struct static_key *key) -{ - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); - - if (!count) - static_key_slow_inc(key); -} -EXPORT_SYMBOL_GPL(static_key_enable); - -void static_key_disable(struct static_key *key) -{ - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); - - if (count) - static_key_slow_dec(key); -} -EXPORT_SYMBOL_GPL(static_key_disable); - void static_key_slow_inc(struct static_key *key) { int v, v1; @@ -136,6 +114,43 @@ void static_key_slow_inc(struct static_key *key) } EXPORT_SYMBOL_GPL(static_key_slow_inc); +void static_key_enable(struct static_key *key) +{ + STATIC_KEY_CHECK_USE(); + if (atomic_read(&key->enabled) > 0) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 1); + return; + } + + cpus_read_lock(); + jump_label_lock(); + if (atomic_read(&key->enabled) == 0) { + atomic_set(&key->enabled, -1); + jump_label_update(key); + atomic_set(&key->enabled, 1); + } + jump_label_unlock(); + cpus_read_unlock(); +} +EXPORT_SYMBOL_GPL(static_key_enable); + +void static_key_disable(struct static_key *key) +{ + STATIC_KEY_CHECK_USE(); + if (atomic_read(&key->enabled) != 1) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 0); + return; + } + + cpus_read_lock(); + jump_label_lock(); + if (atomic_cmpxchg(&key->enabled, 1, 0)) + jump_label_update(key); + jump_label_unlock(); + cpus_read_unlock(); +} +EXPORT_SYMBOL_GPL(static_key_disable); + static void __static_key_slow_dec(struct static_key *key, unsigned long rate_limit, struct delayed_work *work) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() 2017-08-01 15:24 ` [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() Paolo Bonzini @ 2017-08-01 16:45 ` Peter Zijlstra 2017-08-01 16:46 ` Paolo Bonzini 2017-08-10 12:12 ` [tip:locking/core] jump_label: Fix " tip-bot for Paolo Bonzini 1 sibling, 1 reply; 12+ messages in thread From: Peter Zijlstra @ 2017-08-01 16:45 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel Thanks for doing these patches, I hadn't come around to them yet. On Tue, Aug 01, 2017 at 05:24:04PM +0200, Paolo Bonzini wrote: > > +void static_key_enable(struct static_key *key) > +{ > + STATIC_KEY_CHECK_USE(); > + if (atomic_read(&key->enabled) > 0) { > + WARN_ON_ONCE(atomic_read(&key->enabled) != 1); > + return; > + } > + > + cpus_read_lock(); > + jump_label_lock(); > + if (atomic_read(&key->enabled) == 0) { > + atomic_set(&key->enabled, -1); > + jump_label_update(key); As per the previous discussion, should I do a patch adding barriers here (or using atomic_set_release()) such that we close the window where a concurrent inc/enable sees 1 but not all text changes? > + atomic_set(&key->enabled, 1); > + } > + jump_label_unlock(); > + cpus_read_unlock(); > +} > +EXPORT_SYMBOL_GPL(static_key_enable); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() 2017-08-01 16:45 ` Peter Zijlstra @ 2017-08-01 16:46 ` Paolo Bonzini 0 siblings, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2017-08-01 16:46 UTC (permalink / raw) To: Peter Zijlstra; +Cc: linux-kernel On 01/08/2017 18:45, Peter Zijlstra wrote: > > Thanks for doing these patches, I hadn't come around to them yet. > > On Tue, Aug 01, 2017 at 05:24:04PM +0200, Paolo Bonzini wrote: >> >> +void static_key_enable(struct static_key *key) >> +{ >> + STATIC_KEY_CHECK_USE(); >> + if (atomic_read(&key->enabled) > 0) { >> + WARN_ON_ONCE(atomic_read(&key->enabled) != 1); >> + return; >> + } >> + >> + cpus_read_lock(); >> + jump_label_lock(); >> + if (atomic_read(&key->enabled) == 0) { >> + atomic_set(&key->enabled, -1); >> + jump_label_update(key); > > As per the previous discussion, should I do a patch adding barriers here > (or using atomic_set_release()) such that we close the window where a > concurrent inc/enable sees 1 but not all text changes? Sure, and that applies to static_key_slow_inc as well. Paolo >> + atomic_set(&key->enabled, 1); >> + } >> + jump_label_unlock(); >> + cpus_read_unlock(); >> +} >> +EXPORT_SYMBOL_GPL(static_key_enable); ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:locking/core] jump_label: Fix concurrent static_key_enable/disable() 2017-08-01 15:24 ` [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() Paolo Bonzini 2017-08-01 16:45 ` Peter Zijlstra @ 2017-08-10 12:12 ` tip-bot for Paolo Bonzini 1 sibling, 0 replies; 12+ messages in thread From: tip-bot for Paolo Bonzini @ 2017-08-10 12:12 UTC (permalink / raw) To: linux-tip-commits Cc: jbaron, tglx, pbonzini, hpa, peterz, torvalds, linux-kernel, mingo, eric.dumazet Commit-ID: 1dbb6704de91b169a58d0c8221624afd6a95cfc7 Gitweb: http://git.kernel.org/tip/1dbb6704de91b169a58d0c8221624afd6a95cfc7 Author: Paolo Bonzini <pbonzini@redhat.com> AuthorDate: Tue, 1 Aug 2017 17:24:04 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Thu, 10 Aug 2017 12:28:56 +0200 jump_label: Fix concurrent static_key_enable/disable() static_key_enable/disable are trying to cap the static key count to 0/1. However, their use of key->enabled is outside jump_label_lock so they do not really ensure that. Rewrite them to do a quick check for an already enabled (respectively, already disabled), and then recheck under the jump label lock. Unlike static_key_slow_inc/dec, a failed check under the jump label lock does not modify key->enabled. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Jason Baron <jbaron@akamai.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1501601046-35683-2-git-send-email-pbonzini@redhat.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/jump_label.h | 22 +++++++++-------- kernel/jump_label.c | 59 +++++++++++++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index 2afd74b..740a42e 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -234,22 +234,24 @@ static inline int jump_label_apply_nops(struct module *mod) static inline void static_key_enable(struct static_key *key) { - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); + STATIC_KEY_CHECK_USE(); - if (!count) - static_key_slow_inc(key); + if (atomic_read(&key->enabled) != 0) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 1); + return; + } + atomic_set(&key->enabled, 1); } static inline void static_key_disable(struct static_key *key) { - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); + STATIC_KEY_CHECK_USE(); - if (count) - static_key_slow_dec(key); + if (atomic_read(&key->enabled) != 1) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 0); + return; + } + atomic_set(&key->enabled, 0); } #define STATIC_KEY_INIT_TRUE { .enabled = ATOMIC_INIT(1) } diff --git a/kernel/jump_label.c b/kernel/jump_label.c index d11c506..833eeca 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -79,28 +79,6 @@ int static_key_count(struct static_key *key) } EXPORT_SYMBOL_GPL(static_key_count); -void static_key_enable(struct static_key *key) -{ - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); - - if (!count) - static_key_slow_inc(key); -} -EXPORT_SYMBOL_GPL(static_key_enable); - -void static_key_disable(struct static_key *key) -{ - int count = static_key_count(key); - - WARN_ON_ONCE(count < 0 || count > 1); - - if (count) - static_key_slow_dec(key); -} -EXPORT_SYMBOL_GPL(static_key_disable); - void static_key_slow_inc(struct static_key *key) { int v, v1; @@ -139,6 +117,43 @@ void static_key_slow_inc(struct static_key *key) } EXPORT_SYMBOL_GPL(static_key_slow_inc); +void static_key_enable(struct static_key *key) +{ + STATIC_KEY_CHECK_USE(); + if (atomic_read(&key->enabled) > 0) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 1); + return; + } + + cpus_read_lock(); + jump_label_lock(); + if (atomic_read(&key->enabled) == 0) { + atomic_set(&key->enabled, -1); + jump_label_update(key); + atomic_set(&key->enabled, 1); + } + jump_label_unlock(); + cpus_read_unlock(); +} +EXPORT_SYMBOL_GPL(static_key_enable); + +void static_key_disable(struct static_key *key) +{ + STATIC_KEY_CHECK_USE(); + if (atomic_read(&key->enabled) != 1) { + WARN_ON_ONCE(atomic_read(&key->enabled) != 0); + return; + } + + cpus_read_lock(); + jump_label_lock(); + if (atomic_cmpxchg(&key->enabled, 1, 0)) + jump_label_update(key); + jump_label_unlock(); + cpus_read_unlock(); +} +EXPORT_SYMBOL_GPL(static_key_disable); + static void __static_key_slow_dec(struct static_key *key, unsigned long rate_limit, struct delayed_work *work) { ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled 2017-08-01 15:24 [PATCH 0/3] jump_labels: concurrency fixes Paolo Bonzini 2017-08-01 15:24 ` [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() Paolo Bonzini @ 2017-08-01 15:24 ` Paolo Bonzini 2017-08-01 20:52 ` Eric Dumazet 2017-08-10 12:12 ` [tip:locking/core] jump_label: Do not use unserialized static_key_enabled() tip-bot for Paolo Bonzini 2017-08-01 15:24 ` [PATCH 3/3] cpuset: make nr_cpusets private Paolo Bonzini 2 siblings, 2 replies; 12+ messages in thread From: Paolo Bonzini @ 2017-08-01 15:24 UTC (permalink / raw) To: linux-kernel; +Cc: Peter Zijlstra, Eric Dumazet Any use of key->enabled (that is static_key_enabled and static_key_count) outside jump_label_lock should handle its own serialization. The only two that are not doing so are the UDP encapsulation static keys. Change them to use static_key_enable, which now correctly tests key->enabled under the jump label lock. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- Documentation/static-keys.txt | 5 +++++ net/ipv4/udp.c | 3 +-- net/ipv6/udp.c | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt index ef419fd0897f..181998852961 100644 --- a/Documentation/static-keys.txt +++ b/Documentation/static-keys.txt @@ -142,6 +142,11 @@ static_branch_inc(), will change the branch back to true. Likewise, if the key is initialized false, a 'static_branch_inc()', will change the branch to true. And then a 'static_branch_dec()', will again make the branch false. +The state and the reference count can be retrieved with 'static_key_enabled()' +and 'static_key_count()'. In general, if you use these functions, they +should be protected with the same mutex used around the enable/disable +or increment/decrement function. + Where an array of keys is required, it can be defined as: DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count); diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 1d6219bf2d6b..74b7810df9fc 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1644,8 +1644,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) static struct static_key udp_encap_needed __read_mostly; void udp_encap_enable(void) { - if (!static_key_enabled(&udp_encap_needed)) - static_key_slow_inc(&udp_encap_needed); + static_key_enable(&udp_encap_needed); } EXPORT_SYMBOL(udp_encap_enable); diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 75703fda23e7..1a96233622b7 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -564,8 +564,7 @@ static __inline__ void udpv6_err(struct sk_buff *skb, static struct static_key udpv6_encap_needed __read_mostly; void udpv6_encap_enable(void) { - if (!static_key_enabled(&udpv6_encap_needed)) - static_key_slow_inc(&udpv6_encap_needed); + static_key_enable(&udpv6_encap_needed); } EXPORT_SYMBOL(udpv6_encap_enable); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled 2017-08-01 15:24 ` [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled Paolo Bonzini @ 2017-08-01 20:52 ` Eric Dumazet 2017-08-02 5:45 ` Paolo Bonzini 2017-08-10 12:12 ` [tip:locking/core] jump_label: Do not use unserialized static_key_enabled() tip-bot for Paolo Bonzini 1 sibling, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2017-08-01 20:52 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, Peter Zijlstra On Tue, 2017-08-01 at 17:24 +0200, Paolo Bonzini wrote: > Any use of key->enabled (that is static_key_enabled and static_key_count) > outside jump_label_lock should handle its own serialization. The only > two that are not doing so are the UDP encapsulation static keys. Change > them to use static_key_enable, which now correctly tests key->enabled under > the jump label lock. > > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > Documentation/static-keys.txt | 5 +++++ > net/ipv4/udp.c | 3 +-- > net/ipv6/udp.c | 3 +-- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt > index ef419fd0897f..181998852961 100644 > --- a/Documentation/static-keys.txt > +++ b/Documentation/static-keys.txt > @@ -142,6 +142,11 @@ static_branch_inc(), will change the branch back to true. Likewise, if the > key is initialized false, a 'static_branch_inc()', will change the branch to > true. And then a 'static_branch_dec()', will again make the branch false. > > +The state and the reference count can be retrieved with 'static_key_enabled()' > +and 'static_key_count()'. In general, if you use these functions, they > +should be protected with the same mutex used around the enable/disable > +or increment/decrement function. > + > Where an array of keys is required, it can be defined as: > > DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count); > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index 1d6219bf2d6b..74b7810df9fc 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -1644,8 +1644,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > static struct static_key udp_encap_needed __read_mostly; > void udp_encap_enable(void) > { > - if (!static_key_enabled(&udp_encap_needed)) > - static_key_slow_inc(&udp_encap_needed); > + static_key_enable(&udp_encap_needed); > } Looks good to me, but static_key_enable() is not serialized either ? I suspect you should have CCed me on patch 1/3 :) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled 2017-08-01 20:52 ` Eric Dumazet @ 2017-08-02 5:45 ` Paolo Bonzini 0 siblings, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2017-08-02 5:45 UTC (permalink / raw) To: Eric Dumazet; +Cc: linux-kernel, Peter Zijlstra ----- Original Message ----- > From: "Eric Dumazet" <eric.dumazet@gmail.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: linux-kernel@vger.kernel.org, "Peter Zijlstra" <peterz@infradead.org> > Sent: Tuesday, August 1, 2017 10:52:14 PM > Subject: Re: [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled > > On Tue, 2017-08-01 at 17:24 +0200, Paolo Bonzini wrote: > > Any use of key->enabled (that is static_key_enabled and static_key_count) > > outside jump_label_lock should handle its own serialization. The only > > two that are not doing so are the UDP encapsulation static keys. Change > > them to use static_key_enable, which now correctly tests key->enabled under > > the jump label lock. > > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > > index 1d6219bf2d6b..74b7810df9fc 100644 > > --- a/net/ipv4/udp.c > > +++ b/net/ipv4/udp.c > > @@ -1644,8 +1644,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, > > struct sk_buff *skb) > > static struct static_key udp_encap_needed __read_mostly; > > void udp_encap_enable(void) > > { > > - if (!static_key_enabled(&udp_encap_needed)) > > - static_key_slow_inc(&udp_encap_needed); > > + static_key_enable(&udp_encap_needed); > > } > > Looks good to me, but static_key_enable() is not serialized either ? > > I suspect you should have CCed me on patch 1/3 :) Yes, indeed... That's the "now correctly test" in the commit message. What a difference one word makes. :) You can find 1/3 at https://patchwork.kernel.org/patch/9874845/. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:locking/core] jump_label: Do not use unserialized static_key_enabled() 2017-08-01 15:24 ` [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled Paolo Bonzini 2017-08-01 20:52 ` Eric Dumazet @ 2017-08-10 12:12 ` tip-bot for Paolo Bonzini 1 sibling, 0 replies; 12+ messages in thread From: tip-bot for Paolo Bonzini @ 2017-08-10 12:12 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, pbonzini, linux-kernel, jbaron, mingo, eric.dumazet, torvalds, peterz, hpa Commit-ID: 7a34bcb8b272b1300f0125c93a54f0c98812acdd Gitweb: http://git.kernel.org/tip/7a34bcb8b272b1300f0125c93a54f0c98812acdd Author: Paolo Bonzini <pbonzini@redhat.com> AuthorDate: Tue, 1 Aug 2017 17:24:05 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Thu, 10 Aug 2017 12:28:56 +0200 jump_label: Do not use unserialized static_key_enabled() Any use of key->enabled (that is static_key_enabled and static_key_count) outside jump_label_lock should handle its own serialization. The only two that are not doing so are the UDP encapsulation static keys. Change them to use static_key_enable, which now correctly tests key->enabled under the jump label lock. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Jason Baron <jbaron@akamai.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1501601046-35683-3-git-send-email-pbonzini@redhat.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- Documentation/static-keys.txt | 5 +++++ net/ipv4/udp.c | 3 +-- net/ipv6/udp.c | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt index b83dfa1..870b4be 100644 --- a/Documentation/static-keys.txt +++ b/Documentation/static-keys.txt @@ -149,6 +149,11 @@ static_branch_inc(), will change the branch back to true. Likewise, if the key is initialized false, a 'static_branch_inc()', will change the branch to true. And then a 'static_branch_dec()', will again make the branch false. +The state and the reference count can be retrieved with 'static_key_enabled()' +and 'static_key_count()'. In general, if you use these functions, they +should be protected with the same mutex used around the enable/disable +or increment/decrement function. + Where an array of keys is required, it can be defined as:: DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count); diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index e6276fa..3037339 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1809,8 +1809,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) static struct static_key udp_encap_needed __read_mostly; void udp_encap_enable(void) { - if (!static_key_enabled(&udp_encap_needed)) - static_key_slow_inc(&udp_encap_needed); + static_key_enable(&udp_encap_needed); } EXPORT_SYMBOL(udp_encap_enable); diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 578142b..96d2407 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -574,8 +574,7 @@ static __inline__ void udpv6_err(struct sk_buff *skb, static struct static_key udpv6_encap_needed __read_mostly; void udpv6_encap_enable(void) { - if (!static_key_enabled(&udpv6_encap_needed)) - static_key_slow_inc(&udpv6_encap_needed); + static_key_enable(&udpv6_encap_needed); } EXPORT_SYMBOL(udpv6_encap_enable); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] cpuset: make nr_cpusets private 2017-08-01 15:24 [PATCH 0/3] jump_labels: concurrency fixes Paolo Bonzini 2017-08-01 15:24 ` [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() Paolo Bonzini 2017-08-01 15:24 ` [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled Paolo Bonzini @ 2017-08-01 15:24 ` Paolo Bonzini 2017-08-02 1:08 ` Zefan Li 2017-08-10 12:13 ` [tip:locking/core] cpuset: Make " tip-bot for Paolo Bonzini 2 siblings, 2 replies; 12+ messages in thread From: Paolo Bonzini @ 2017-08-01 15:24 UTC (permalink / raw) To: linux-kernel; +Cc: Peter Zijlstra, Zefan Li Any use of key->enabled (that is static_key_enabled and static_key_count) outside jump_label_lock should handle its own serialization. In the case of cpusets_enabled_key, the key is always incremented/decremented under cpuset_mutex, and hence the same rule applies to nr_cpusets. The rule *is* respected currently, but the mutex is static so nr_cpusets should be static too. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Zefan Li <lizefan@huawei.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- include/linux/cpuset.h | 6 ------ kernel/cgroup/cpuset.c | 7 +++++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h index 119a3f9604b0..cedcc910b7a7 100644 --- a/include/linux/cpuset.h +++ b/include/linux/cpuset.h @@ -24,12 +24,6 @@ static inline bool cpusets_enabled(void) return static_branch_unlikely(&cpusets_enabled_key); } -static inline int nr_cpusets(void) -{ - /* jump label reference count + the top-level cpuset */ - return static_key_count(&cpusets_enabled_key.key) + 1; -} - static inline void cpuset_inc(void) { static_branch_inc(&cpusets_enabled_key); diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index ae643412948a..f65db17e9e23 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -576,6 +576,13 @@ static void update_domain_attr_tree(struct sched_domain_attr *dattr, rcu_read_unlock(); } +/* Must be called with cpuset_mutex held. */ +static inline int nr_cpusets(void) +{ + /* jump label reference count + the top-level cpuset */ + return static_key_count(&cpusets_enabled_key.key) + 1; +} + /* * generate_sched_domains() * -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] cpuset: make nr_cpusets private 2017-08-01 15:24 ` [PATCH 3/3] cpuset: make nr_cpusets private Paolo Bonzini @ 2017-08-02 1:08 ` Zefan Li 2017-08-10 12:13 ` [tip:locking/core] cpuset: Make " tip-bot for Paolo Bonzini 1 sibling, 0 replies; 12+ messages in thread From: Zefan Li @ 2017-08-02 1:08 UTC (permalink / raw) To: Paolo Bonzini, linux-kernel; +Cc: Peter Zijlstra On 2017/8/1 23:24, Paolo Bonzini wrote: > Any use of key->enabled (that is static_key_enabled and static_key_count) > outside jump_label_lock should handle its own serialization. In the case > of cpusets_enabled_key, the key is always incremented/decremented under > cpuset_mutex, and hence the same rule applies to nr_cpusets. The rule > *is* respected currently, but the mutex is static so nr_cpusets should > be static too. > > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Zefan Li <lizefan@huawei.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Acked-by: Zefan Li <lizefan@huawei.com> > --- > include/linux/cpuset.h | 6 ------ > kernel/cgroup/cpuset.c | 7 +++++++ > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h > index 119a3f9604b0..cedcc910b7a7 100644 > --- a/include/linux/cpuset.h > +++ b/include/linux/cpuset.h > @@ -24,12 +24,6 @@ static inline bool cpusets_enabled(void) > return static_branch_unlikely(&cpusets_enabled_key); > } > > -static inline int nr_cpusets(void) > -{ > - /* jump label reference count + the top-level cpuset */ > - return static_key_count(&cpusets_enabled_key.key) + 1; > -} > - > static inline void cpuset_inc(void) > { > static_branch_inc(&cpusets_enabled_key); > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c > index ae643412948a..f65db17e9e23 100644 > --- a/kernel/cgroup/cpuset.c > +++ b/kernel/cgroup/cpuset.c > @@ -576,6 +576,13 @@ static void update_domain_attr_tree(struct sched_domain_attr *dattr, > rcu_read_unlock(); > } > > +/* Must be called with cpuset_mutex held. */ > +static inline int nr_cpusets(void) > +{ > + /* jump label reference count + the top-level cpuset */ > + return static_key_count(&cpusets_enabled_key.key) + 1; > +} > + > /* > * generate_sched_domains() > * > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:locking/core] cpuset: Make nr_cpusets private 2017-08-01 15:24 ` [PATCH 3/3] cpuset: make nr_cpusets private Paolo Bonzini 2017-08-02 1:08 ` Zefan Li @ 2017-08-10 12:13 ` tip-bot for Paolo Bonzini 1 sibling, 0 replies; 12+ messages in thread From: tip-bot for Paolo Bonzini @ 2017-08-10 12:13 UTC (permalink / raw) To: linux-tip-commits Cc: pbonzini, linux-kernel, torvalds, lizefan, mingo, peterz, hpa, tglx Commit-ID: be040bea9085a9c2b1700c9e60888777baeb96d5 Gitweb: http://git.kernel.org/tip/be040bea9085a9c2b1700c9e60888777baeb96d5 Author: Paolo Bonzini <pbonzini@redhat.com> AuthorDate: Tue, 1 Aug 2017 17:24:06 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Thu, 10 Aug 2017 12:28:57 +0200 cpuset: Make nr_cpusets private Any use of key->enabled (that is static_key_enabled and static_key_count) outside jump_label_lock should handle its own serialization. In the case of cpusets_enabled_key, the key is always incremented/decremented under cpuset_mutex, and hence the same rule applies to nr_cpusets. The rule *is* respected currently, but the mutex is static so nr_cpusets should be static too. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Acked-by: Zefan Li <lizefan@huawei.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1501601046-35683-4-git-send-email-pbonzini@redhat.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/cpuset.h | 6 ------ kernel/cgroup/cpuset.c | 7 +++++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h index 898cfe2..e74655d 100644 --- a/include/linux/cpuset.h +++ b/include/linux/cpuset.h @@ -37,12 +37,6 @@ static inline bool cpusets_enabled(void) return static_branch_unlikely(&cpusets_enabled_key); } -static inline int nr_cpusets(void) -{ - /* jump label reference count + the top-level cpuset */ - return static_key_count(&cpusets_enabled_key.key) + 1; -} - static inline void cpuset_inc(void) { static_branch_inc(&cpusets_pre_enable_key); diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index 8d51516..9ed6a05 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -577,6 +577,13 @@ static void update_domain_attr_tree(struct sched_domain_attr *dattr, rcu_read_unlock(); } +/* Must be called with cpuset_mutex held. */ +static inline int nr_cpusets(void) +{ + /* jump label reference count + the top-level cpuset */ + return static_key_count(&cpusets_enabled_key.key) + 1; +} + /* * generate_sched_domains() * ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-08-10 12:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-01 15:24 [PATCH 0/3] jump_labels: concurrency fixes Paolo Bonzini 2017-08-01 15:24 ` [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable() Paolo Bonzini 2017-08-01 16:45 ` Peter Zijlstra 2017-08-01 16:46 ` Paolo Bonzini 2017-08-10 12:12 ` [tip:locking/core] jump_label: Fix " tip-bot for Paolo Bonzini 2017-08-01 15:24 ` [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled Paolo Bonzini 2017-08-01 20:52 ` Eric Dumazet 2017-08-02 5:45 ` Paolo Bonzini 2017-08-10 12:12 ` [tip:locking/core] jump_label: Do not use unserialized static_key_enabled() tip-bot for Paolo Bonzini 2017-08-01 15:24 ` [PATCH 3/3] cpuset: make nr_cpusets private Paolo Bonzini 2017-08-02 1:08 ` Zefan Li 2017-08-10 12:13 ` [tip:locking/core] cpuset: Make " tip-bot for Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).