linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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

* [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 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

* 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 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

* 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: 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

* [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

* [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).