On Fri, Jun 24, 2016 at 01:59:06AM -0700, tip-bot for Paolo Bonzini wrote:
> +++ b/kernel/jump_label.c
> @@ -58,13 +58,36 @@ static void jump_label_update(struct static_key *key);
>
> void static_key_slow_inc(struct static_key *key)
> {
> + int v, v1;
> +
> STATIC_KEY_CHECK_USE();
> - if (atomic_inc_not_zero(&key->enabled))
> - return;
> +
> + /*
> + * Careful if we get concurrent static_key_slow_inc() calls;
> + * later calls must wait for the first one to _finish_ the
> + * jump_label_update() process. At the same time, however,
> + * the jump_label_update() call below wants to see
> + * static_key_enabled(&key) for jumps to be updated properly.
> + *
> + * So give a special meaning to negative key->enabled: it sends
> + * static_key_slow_inc() down the slow path, and it is non-zero
> + * so it counts as "enabled" in jump_label_update(). Note that
> + * atomic_inc_unless_negative() checks >= 0, so roll our own.
> + */
> + for (v = atomic_read(&key->enabled); v > 0; v = v1) {
> + v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> + if (likely(v1 == v))
> + return;
> + }
>
> jump_label_lock();
> - if (atomic_inc_return(&key->enabled) == 1)
> + if (atomic_read(&key->enabled) == 0) {
> + atomic_set(&key->enabled, -1);
> jump_label_update(key);
> + atomic_set(&key->enabled, 1);
> + } else {
> + atomic_inc(&key->enabled);
> + }
> jump_label_unlock();
> }
So I was recently looking at this again and got all paranoid. Do we want
something like so?
---
kernel/jump_label.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d11c506a6ac3..69d07e78e48b 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -103,7 +103,7 @@ EXPORT_SYMBOL_GPL(static_key_disable);
void static_key_slow_inc(struct static_key *key)
{
- int v, v1;
+ int v;
STATIC_KEY_CHECK_USE();
@@ -119,18 +119,28 @@ void static_key_slow_inc(struct static_key *key)
* so it counts as "enabled" in jump_label_update(). Note that
* atomic_inc_unless_negative() checks >= 0, so roll our own.
*/
- for (v = atomic_read(&key->enabled); v > 0; v = v1) {
- v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
- if (likely(v1 == v))
+ for (v = atomic_read(&key->enabled); v > 0;) {
+ if (atomic_try_cmpxchg(&key->enabled, &v, v+1))
return;
}
cpus_read_lock();
jump_label_lock();
- if (atomic_read(&key->enabled) == 0) {
- atomic_set(&key->enabled, -1);
+
+ if (atomic_try_cmpxchg(&key->enabled, 0, -1)) {
+ /*
+ * smp_mb implied, must have -1 before proceeding to change
+ * text.
+ */
jump_label_update(key);
- atomic_set(&key->enabled, 1);
+
+ /*
+ * smp_mb, such that we finish modifying text before enabling
+ * the fast path. Probably not needed because modifying text is
+ * likely to serialize everything. Be paranoid.
+ */
+ smp_mb__before_atomic();
+ atomic_add(2, &key->enabled); /* -1 -> 1 */
} else {
atomic_inc(&key->enabled);
}
On 31/07/2017 11:36, Peter Zijlstra wrote:
> On Fri, Jun 24, 2016 at 01:59:06AM -0700, tip-bot for Paolo Bonzini wrote:
>> +++ b/kernel/jump_label.c
>> @@ -58,13 +58,36 @@ static void jump_label_update(struct static_key *key);
>>
>> void static_key_slow_inc(struct static_key *key)
>> {
>> + int v, v1;
>> +
>> STATIC_KEY_CHECK_USE();
>> - if (atomic_inc_not_zero(&key->enabled))
>> - return;
>> +
>> + /*
>> + * Careful if we get concurrent static_key_slow_inc() calls;
>> + * later calls must wait for the first one to _finish_ the
>> + * jump_label_update() process. At the same time, however,
>> + * the jump_label_update() call below wants to see
>> + * static_key_enabled(&key) for jumps to be updated properly.
>> + *
>> + * So give a special meaning to negative key->enabled: it sends
>> + * static_key_slow_inc() down the slow path, and it is non-zero
>> + * so it counts as "enabled" in jump_label_update(). Note that
>> + * atomic_inc_unless_negative() checks >= 0, so roll our own.
>> + */
>> + for (v = atomic_read(&key->enabled); v > 0; v = v1) {
>> + v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
>> + if (likely(v1 == v))
>> + return;
>> + }
>>
>> jump_label_lock();
>> - if (atomic_inc_return(&key->enabled) == 1)
>> + if (atomic_read(&key->enabled) == 0) {
>> + atomic_set(&key->enabled, -1);
>> jump_label_update(key);
>> + atomic_set(&key->enabled, 1);
>> + } else {
>> + atomic_inc(&key->enabled);
>> + }
>> jump_label_unlock();
>> }
>
>
> So I was recently looking at this again and got all paranoid. Do we want
> something like so?
Though I agree with the paranoia sentiment, no:
- key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
For this reason the atomic_try_cmpxchg is unnecessary.
- the (implied) smp_mb before jump_label_update is not interesting, but
I don't think it is useful because: 1) during the jump_label_update
there is no correspondence between what static_key_enabled returns and
what the text look like; 2) what would it even be pairing with?
- the smp_mb (though it could be a smp_wmb or atomic_set_release)
initially triggered my paranoia indeed. But even then, I can't see why
you would need it because there's nothing it pairs with. Rather, it's
*any use of key->enabled outside jump_label_lock* (meaning: any use of
static_key_enabled and static_key_count outside the core jump_label.c
code) that should be handled with care.
And indeed, while there aren't many, I think two of them are wrong (and
not fixed by your patch):
- include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
It makes no sense to call it outside cpuset_mutex, and indeed that's
how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
<- rebuild_sched_domains_locked). The function should be moved inside
kernel/cgroup/cpuset.c since the mutex is static.
- kernel/cgroup/cgroup.c only enables/disables at init, so using
static_key_enabled should be fine.
- kernel/tracepoint.c only manipulates tracepoint static keys under
tracepoints_mutex, and uses static_key_enabled under the same mutex, so
it's fine.
- net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
increment of the static key. It's racy and maybe we should provide a
new API static_key_enable_forever:
void static_key_enable_forever(struct static_key *key)
{
STATIC_KEY_CHECK_USE();
if (atomic_read(&key->enabled) > 0)
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_forever);
I can prepare a patch if you agree.
Paolo
> ---
> kernel/jump_label.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index d11c506a6ac3..69d07e78e48b 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -103,7 +103,7 @@ EXPORT_SYMBOL_GPL(static_key_disable);
>
> void static_key_slow_inc(struct static_key *key)
> {
> - int v, v1;
> + int v;
>
> STATIC_KEY_CHECK_USE();
>
> @@ -119,18 +119,28 @@ void static_key_slow_inc(struct static_key *key)
> * so it counts as "enabled" in jump_label_update(). Note that
> * atomic_inc_unless_negative() checks >= 0, so roll our own.
> */
> - for (v = atomic_read(&key->enabled); v > 0; v = v1) {
> - v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> - if (likely(v1 == v))
> + for (v = atomic_read(&key->enabled); v > 0;) {
> + if (atomic_try_cmpxchg(&key->enabled, &v, v+1))
> return;
> }
>
> cpus_read_lock();
> jump_label_lock();
> - if (atomic_read(&key->enabled) == 0) {
> - atomic_set(&key->enabled, -1);
> +
> + if (atomic_try_cmpxchg(&key->enabled, 0, -1)) {
> + /*
> + * smp_mb implied, must have -1 before proceeding to change
> + * text.
> + */
> jump_label_update(key);
> - atomic_set(&key->enabled, 1);
> +
> + /*
> + * smp_mb, such that we finish modifying text before enabling
> + * the fast path. Probably not needed because modifying text is
> + * likely to serialize everything. Be paranoid.
> + */
> + smp_mb__before_atomic();
> + atomic_add(2, &key->enabled); /* -1 -> 1 */
> } else {
> atomic_inc(&key->enabled);
> }
>
On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
> - key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
> For this reason the atomic_try_cmpxchg is unnecessary.
Agreed, the only reason was the implied barrier.
> - the (implied) smp_mb before jump_label_update is not interesting, but
> I don't think it is useful because: 1) during the jump_label_update
> there is no correspondence between what static_key_enabled returns and
> what the text look like; 2) what would it even be pairing with?
Ah, indeed. So I was worried about the text changes escaping upwards,
but you're right in that there's no harm in that because there's nothing
that cares.
Another inc would see 0 and still serialize on the mutex.
> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
> initially triggered my paranoia indeed. But even then, I can't see why
> you would need it because there's nothing it pairs with.
So this one would pair with the mb implied by the cmpxchg loop for
inc-if-positive. The ordering being that if we see a positive v, we must
then also see all the text modifications.
So if jump_label_update() were to not fully serialize things, it would
be possible for the v=1 store to appear before the last text changes.
And in that case we'd allow the fast path to complete
static_key_slow_inc() before it was in fact done with changing all text.
Now, I suspect (but did not audit) that anything that changes text must
in fact serialize world, but I wasn't sure.
> Rather, it's *any use of key->enabled outside jump_label_lock*
> (meaning: any use of static_key_enabled and static_key_count outside
> the core jump_label.c code) that should be handled with care.
> And indeed, while there aren't many, I think two of them are wrong (and
> not fixed by your patch):
>
> - include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
> It makes no sense to call it outside cpuset_mutex, and indeed that's
> how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
> <- rebuild_sched_domains_locked). The function should be moved inside
> kernel/cgroup/cpuset.c since the mutex is static.
Dima was poking at that code.
> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
> increment of the static key. It's racy and maybe we should provide a
> new API static_key_enable_forever:
>
> void static_key_enable_forever(struct static_key *key)
> {
> STATIC_KEY_CHECK_USE();
> if (atomic_read(&key->enabled) > 0)
> 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_forever);
>
> I can prepare a patch if you agree.
Isn't that what we have static_key_enable() for? Which btw also uses
static_key_count() outside of the mutex.
On 31/07/2017 15:33, Peter Zijlstra wrote:
> On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
>> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
>> initially triggered my paranoia indeed. But even then, I can't see why
>> you would need it because there's nothing it pairs with.
>
> So this one would pair with the mb implied by the cmpxchg loop for
> inc-if-positive. The ordering being that if we see a positive v, we must
> then also see all the text modifications.
>
> So if jump_label_update() were to not fully serialize things, it would
> be possible for the v=1 store to appear before the last text changes.
> And in that case we'd allow the fast path to complete
> static_key_slow_inc() before it was in fact done with changing all text.
>
> Now, I suspect (but did not audit) that anything that changes text must
> in fact serialize world, but I wasn't sure.
I see. Then yes, I agree a smp_wmb would be nicer here.
>> Rather, it's *any use of key->enabled outside jump_label_lock*
>> (meaning: any use of static_key_enabled and static_key_count outside
>> the core jump_label.c code) that should be handled with care.
>
>> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
>> increment of the static key. It's racy and maybe we should provide a
>> new API static_key_enable_forever:
>>
>> void static_key_enable_forever(struct static_key *key)
>> {
>> STATIC_KEY_CHECK_USE();
>> if (atomic_read(&key->enabled) > 0)
>> 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_forever);
>>
>> I can prepare a patch if you agree.
>
> Isn't that what we have static_key_enable() for? Which btw also uses
> static_key_count() outside of the mutex.
Yes, they should be fixed and net/ can then use static_key_enable.
Paolo
On Mon, Jul 31, 2017 at 05:35:40PM +0200, Paolo Bonzini wrote:
> On 31/07/2017 15:33, Peter Zijlstra wrote:
> > On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
> >> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
> >> initially triggered my paranoia indeed. But even then, I can't see why
> >> you would need it because there's nothing it pairs with.
> >
> > So this one would pair with the mb implied by the cmpxchg loop for
> > inc-if-positive. The ordering being that if we see a positive v, we must
> > then also see all the text modifications.
> >
> > So if jump_label_update() were to not fully serialize things, it would
> > be possible for the v=1 store to appear before the last text changes.
> > And in that case we'd allow the fast path to complete
> > static_key_slow_inc() before it was in fact done with changing all text.
> >
> > Now, I suspect (but did not audit) that anything that changes text must
> > in fact serialize world, but I wasn't sure.
>
> I see. Then yes, I agree a smp_wmb would be nicer here.
I'll make it atomic_set_release() and a comment.
> >> Rather, it's *any use of key->enabled outside jump_label_lock*
> >> (meaning: any use of static_key_enabled and static_key_count outside
> >> the core jump_label.c code) that should be handled with care.
> >
> >> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
> >> increment of the static key. It's racy and maybe we should provide a
> >> new API static_key_enable_forever:
> >>
> >> void static_key_enable_forever(struct static_key *key)
> >> {
> >> STATIC_KEY_CHECK_USE();
> >> if (atomic_read(&key->enabled) > 0)
> >> 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_forever);
> >>
> >> I can prepare a patch if you agree.
> >
> > Isn't that what we have static_key_enable() for? Which btw also uses
> > static_key_count() outside of the mutex.
>
> Yes, they should be fixed and net/ can then use static_key_enable.
Right, let me try and fix _enable().
On Mon, Jul 31, 2017 at 6:33 AM, Peter Zijlstra <[email protected]> wrote:
> On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
>> - key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
>> For this reason the atomic_try_cmpxchg is unnecessary.
>
> Agreed, the only reason was the implied barrier.
>
>> - the (implied) smp_mb before jump_label_update is not interesting, but
>> I don't think it is useful because: 1) during the jump_label_update
>> there is no correspondence between what static_key_enabled returns and
>> what the text look like; 2) what would it even be pairing with?
>
> Ah, indeed. So I was worried about the text changes escaping upwards,
> but you're right in that there's no harm in that because there's nothing
> that cares.
>
> Another inc would see 0 and still serialize on the mutex.
>
>> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
>> initially triggered my paranoia indeed. But even then, I can't see why
>> you would need it because there's nothing it pairs with.
>
> So this one would pair with the mb implied by the cmpxchg loop for
> inc-if-positive. The ordering being that if we see a positive v, we must
> then also see all the text modifications.
>
> So if jump_label_update() were to not fully serialize things, it would
> be possible for the v=1 store to appear before the last text changes.
> And in that case we'd allow the fast path to complete
> static_key_slow_inc() before it was in fact done with changing all text.
>
> Now, I suspect (but did not audit) that anything that changes text must
> in fact serialize world, but I wasn't sure.
>
>> Rather, it's *any use of key->enabled outside jump_label_lock*
>> (meaning: any use of static_key_enabled and static_key_count outside
>> the core jump_label.c code) that should be handled with care.
>
>> And indeed, while there aren't many, I think two of them are wrong (and
>> not fixed by your patch):
>>
>> - include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
>> It makes no sense to call it outside cpuset_mutex, and indeed that's
>> how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
>> <- rebuild_sched_domains_locked). The function should be moved inside
>> kernel/cgroup/cpuset.c since the mutex is static.
>
> Dima was poking at that code.
As I understand it, in the case of read_mems_allowed, the value of the
key enabled count itself is not used. We just use the branch rewrite
to avoid seqcount lookups, so only the text changes actually matter
there. We'd still need my fix since the problem was ordering of the
text changes.
Thanks!
--Dima
>
>> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
>> increment of the static key. It's racy and maybe we should provide a
>> new API static_key_enable_forever:
>>
>> void static_key_enable_forever(struct static_key *key)
>> {
>> STATIC_KEY_CHECK_USE();
>> if (atomic_read(&key->enabled) > 0)
>> 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_forever);
>>
>> I can prepare a patch if you agree.
>
> Isn't that what we have static_key_enable() for? Which btw also uses
> static_key_count() outside of the mutex.
> > > Isn't that what we have static_key_enable() for? Which btw also uses
> > > static_key_count() outside of the mutex.
> >
> > Yes, they should be fixed and net/ can then use static_key_enable.
>
> Right, let me try and fix _enable().
Here is what I scribbled before leaving the office. (What was missing:
documentation for how to use static_key_enabled/count, testing).
>From c0bdcc89e26fb16cdc564485232bebcd1e0cc102 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Mon, 31 Jul 2017 16:46:04 +0200
Subject: [PATCH 1/3] jump_labels: 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) key, 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 <[email protected]>
---
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..8fc5649476ca 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 d11c506a6ac3..47a3e927b87e 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)
{
--
2.13.3
>From 89d89520915bb12fd330069ca8aed32a0c216ab6 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Mon, 31 Jul 2017 16:48:05 +0200
Subject: [PATCH 2/3] jump_labels: 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 <[email protected]>
---
net/ipv4/udp.c | 3 +--
net/ipv6/udp.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index b057653ceca9..9b305776fe14 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1811,8 +1811,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 4a3e65626e8b..27057c41d648 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -569,8 +569,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);
--
2.13.3
>From 7d8f5a373c0357663683197dfc64f20abf31a892 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Mon, 31 Jul 2017 16:52:07 +0200
Subject: [PATCH 3/3] 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 <[email protected]>
---
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 ca8376e5008c..6ddca2480276 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()
*
--
2.13.3