Kernel broken on processors without performance counters

Kernel broken on processors without performance counters

[Mikulas Patocka]

Hi

I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks 
the kernel on processors without performance counters, such as AMD K6-3. 
Reverting the patch fixes the problem.

The static key rdpmc_always_available somehow gets set (I couldn't really 
find out what is setting it, the function set_attr_rdpmc is not executed), 
cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot 
when attempting to execute init, because the proecssor doesn't support 
that bit in CR4.

Mikulas

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
> Hi
> 
> I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks 
> the kernel on processors without performance counters, such as AMD K6-3. 
> Reverting the patch fixes the problem.
> 
> The static key rdpmc_always_available somehow gets set (I couldn't really 
> find out what is setting it, the function set_attr_rdpmc is not executed), 
> cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot 
> when attempting to execute init, because the proecssor doesn't support 
> that bit in CR4.

Urgh, the static key trainwreck bites again.

One is not supposed to mix static_key_true() and STATIC_KEY_INIT_FALSE.

Does this make it go again?

---
 arch/x86/include/asm/mmu_context.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 5e8daee7c5c9..804a3a6030ca 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
 
 static inline void load_mm_cr4(struct mm_struct *mm)
 {
-	if (static_key_true(&rdpmc_always_available) ||
+	if (static_key_false(&rdpmc_always_available) ||
 	    atomic_read(&mm->context.perf_rdpmc_allowed))
 		cr4_set_bits(X86_CR4_PCE);
 	else

Re: Kernel broken on processors without performance counters

[Mikulas Patocka]

On Wed, 8 Jul 2015, Peter Zijlstra wrote:

> On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
> > Hi
> > 
> > I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks 
> > the kernel on processors without performance counters, such as AMD K6-3. 
> > Reverting the patch fixes the problem.
> > 
> > The static key rdpmc_always_available somehow gets set (I couldn't really 
> > find out what is setting it, the function set_attr_rdpmc is not executed), 
> > cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot 
> > when attempting to execute init, because the proecssor doesn't support 
> > that bit in CR4.
> 
> Urgh, the static key trainwreck bites again.
> 
> One is not supposed to mix static_key_true() and STATIC_KEY_INIT_FALSE.
> 
> Does this make it go again?

Yes, this patch fixes the problem.

Mikulas

> ---
>  arch/x86/include/asm/mmu_context.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index 5e8daee7c5c9..804a3a6030ca 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
>  
>  static inline void load_mm_cr4(struct mm_struct *mm)
>  {
> -	if (static_key_true(&rdpmc_always_available) ||
> +	if (static_key_false(&rdpmc_always_available) ||
>  	    atomic_read(&mm->context.perf_rdpmc_allowed))
>  		cr4_set_bits(X86_CR4_PCE);
>  	else
> 

[PATCH] x86: Fix static_key in load_mm_cr4()

[Peter Zijlstra]

Mikulas reported his K6-3 not booting. This is because the static_key
confusion struck and bit Andy, this wants to be static_key_false().

Fixes: a66734297f78 ("perf/x86: Add /sys/devices/cpu/rdpmc=2 to allow rdpmc for all tasks")
Cc: Andy Lutomirski <luto@amacapital.net>
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Tested-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/mmu_context.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 5e8daee7c5c9..804a3a6030ca 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
 
 static inline void load_mm_cr4(struct mm_struct *mm)
 {
-	if (static_key_true(&rdpmc_always_available) ||
+	if (static_key_false(&rdpmc_always_available) ||
 	    atomic_read(&mm->context.perf_rdpmc_allowed))
 		cr4_set_bits(X86_CR4_PCE);
 	else

Re: [PATCH] x86: Fix static_key in load_mm_cr4()

[Mikulas Patocka]

On Thu, 9 Jul 2015, Peter Zijlstra wrote:

> 
> Mikulas reported his K6-3 not booting. This is because the static_key
> confusion struck and bit Andy, this wants to be static_key_false().
> 
> Fixes: a66734297f78 ("perf/x86: Add /sys/devices/cpu/rdpmc=2 to allow rdpmc for all tasks")
> Cc: Andy Lutomirski <luto@amacapital.net>
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>


You should also add
Cc: stable@vger.kernel.org	# v4.0
so that it will be backported to 4.0 and 4.1.

> ---
>  arch/x86/include/asm/mmu_context.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index 5e8daee7c5c9..804a3a6030ca 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
>  
>  static inline void load_mm_cr4(struct mm_struct *mm)
>  {
> -	if (static_key_true(&rdpmc_always_available) ||
> +	if (static_key_false(&rdpmc_always_available) ||
>  	    atomic_read(&mm->context.perf_rdpmc_allowed))
>  		cr4_set_bits(X86_CR4_PCE);
>  	else
> 

[tip:perf/urgent] x86, perf: Fix static_key bug in load_mm_cr4()

[tip-bot for Peter Zijlstra]

Commit-ID:  a833581e372a4adae2319d8dc379493edbc444e9
Gitweb:     http://git.kernel.org/tip/a833581e372a4adae2319d8dc379493edbc444e9
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 9 Jul 2015 19:23:38 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 10 Jul 2015 10:24:38 +0200

x86, perf: Fix static_key bug in load_mm_cr4()

Mikulas reported his K6-3 not booting. This is because the
static_key API confusion struck and bit Andy, this wants to be
static_key_false().

Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Tested-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <stable@vger.kernel.org>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
Cc: Vince Weaver <vince@deater.net>
Cc: hillf.zj <hillf.zj@alibaba-inc.com>
Fixes: a66734297f78 ("perf/x86: Add /sys/devices/cpu/rdpmc=2 to allow rdpmc for all tasks")
Link: http://lkml.kernel.org/r/20150709172338.GC19282@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/mmu_context.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 5e8daee..804a3a6 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
 
 static inline void load_mm_cr4(struct mm_struct *mm)
 {
-	if (static_key_true(&rdpmc_always_available) ||
+	if (static_key_false(&rdpmc_always_available) ||
 	    atomic_read(&mm->context.perf_rdpmc_allowed))
 		cr4_set_bits(X86_CR4_PCE);
 	else

Re: Kernel broken on processors without performance counters

[Andy Lutomirski]

On Wed, Jul 8, 2015 at 9:07 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
>> Hi
>>
>> I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks
>> the kernel on processors without performance counters, such as AMD K6-3.
>> Reverting the patch fixes the problem.
>>
>> The static key rdpmc_always_available somehow gets set (I couldn't really
>> find out what is setting it, the function set_attr_rdpmc is not executed),
>> cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot
>> when attempting to execute init, because the proecssor doesn't support
>> that bit in CR4.
>
> Urgh, the static key trainwreck bites again.
>
> One is not supposed to mix static_key_true() and STATIC_KEY_INIT_FALSE.
>
> Does this make it go again?
>
> ---
>  arch/x86/include/asm/mmu_context.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index 5e8daee7c5c9..804a3a6030ca 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
>
>  static inline void load_mm_cr4(struct mm_struct *mm)
>  {
> -       if (static_key_true(&rdpmc_always_available) ||
> +       if (static_key_false(&rdpmc_always_available) ||

In what universe is "static_key_false" a reasonable name for a
function that returns true if a static key is true?

Can we rename that function?  And could we maybe make static keys type
safe?  I.e. there would be a type that starts out true and a type that
starts out false.

--Andy

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/08/2015 01:37 PM, Andy Lutomirski wrote:
> On Wed, Jul 8, 2015 at 9:07 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
>>> Hi
>>>
>>> I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks
>>> the kernel on processors without performance counters, such as AMD K6-3.
>>> Reverting the patch fixes the problem.
>>>
>>> The static key rdpmc_always_available somehow gets set (I couldn't really
>>> find out what is setting it, the function set_attr_rdpmc is not executed),
>>> cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot
>>> when attempting to execute init, because the proecssor doesn't support
>>> that bit in CR4.
>> Urgh, the static key trainwreck bites again.
>>
>> One is not supposed to mix static_key_true() and STATIC_KEY_INIT_FALSE.
>>
>> Does this make it go again?
>>
>> ---
>>  arch/x86/include/asm/mmu_context.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index 5e8daee7c5c9..804a3a6030ca 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
>>
>>  static inline void load_mm_cr4(struct mm_struct *mm)
>>  {
>> -       if (static_key_true(&rdpmc_always_available) ||
>> +       if (static_key_false(&rdpmc_always_available) ||
> In what universe is "static_key_false" a reasonable name for a
> function that returns true if a static key is true?
>
> Can we rename that function?  And could we maybe make static keys type
> safe?  I.e. there would be a type that starts out true and a type that
> starts out false.

So the 'static_key_false' is really branch is initially false. We had
a naming discussion before, but if ppl think its confusing,
'static_key_init_false', or 'static_key_default_false' might be better,
or other ideas.... I agree its confusing.

In terms of getting the type to match so we don't have these
mismatches, I think we could introduce 'struct static_key_false'
and 'struct static_key_true' with proper initializers. However,
'static_key_slow_inc()/dec()'  would also have to add the
true/false modifier. Or maybe we do:

struct static_key_false {
    struct static_key key;
} random_key;

and then the 'static_key_sloc_inc()/dec()' would just take
a &random_key.key....

If we were to change this, I don't think it would be too hard to
introduce the new API, convert subsystems over time and then
drop the old one.

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Andy Lutomirski]

On Wed, Jul 8, 2015 at 1:04 PM, Jason Baron <jasonbaron0@gmail.com> wrote:
> On 07/08/2015 01:37 PM, Andy Lutomirski wrote:
>> On Wed, Jul 8, 2015 at 9:07 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>> On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
>>>> Hi
>>>>
>>>> I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks
>>>> the kernel on processors without performance counters, such as AMD K6-3.
>>>> Reverting the patch fixes the problem.
>>>>
>>>> The static key rdpmc_always_available somehow gets set (I couldn't really
>>>> find out what is setting it, the function set_attr_rdpmc is not executed),
>>>> cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot
>>>> when attempting to execute init, because the proecssor doesn't support
>>>> that bit in CR4.
>>> Urgh, the static key trainwreck bites again.
>>>
>>> One is not supposed to mix static_key_true() and STATIC_KEY_INIT_FALSE.
>>>
>>> Does this make it go again?
>>>
>>> ---
>>>  arch/x86/include/asm/mmu_context.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>>> index 5e8daee7c5c9..804a3a6030ca 100644
>>> --- a/arch/x86/include/asm/mmu_context.h
>>> +++ b/arch/x86/include/asm/mmu_context.h
>>> @@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
>>>
>>>  static inline void load_mm_cr4(struct mm_struct *mm)
>>>  {
>>> -       if (static_key_true(&rdpmc_always_available) ||
>>> +       if (static_key_false(&rdpmc_always_available) ||
>> In what universe is "static_key_false" a reasonable name for a
>> function that returns true if a static key is true?
>>
>> Can we rename that function?  And could we maybe make static keys type
>> safe?  I.e. there would be a type that starts out true and a type that
>> starts out false.
>
> So the 'static_key_false' is really branch is initially false. We had
> a naming discussion before, but if ppl think its confusing,
> 'static_key_init_false', or 'static_key_default_false' might be better,
> or other ideas.... I agree its confusing.

I think the current naming is almost maximally bad.  The naming would
be less critical if it were typesafe, though.

>
> In terms of getting the type to match so we don't have these
> mismatches, I think we could introduce 'struct static_key_false'
> and 'struct static_key_true' with proper initializers. However,
> 'static_key_slow_inc()/dec()'  would also have to add the
> true/false modifier.

I think that would be okay.

> Or maybe we do:
>
> struct static_key_false {
>     struct static_key key;
> } random_key;
>
> and then the 'static_key_sloc_inc()/dec()' would just take
> a &random_key.key....

That would be okay, too.  Or it could be a macro that has the same effect.

>
> If we were to change this, I don't think it would be too hard to
> introduce the new API, convert subsystems over time and then
> drop the old one.

And we might discover a bug or three while doing it :)

--Andy

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Wed, Jul 08, 2015 at 05:36:43PM -0700, Andy Lutomirski wrote:
> >> In what universe is "static_key_false" a reasonable name for a
> >> function that returns true if a static key is true?

> I think the current naming is almost maximally bad.  The naming would
> be less critical if it were typesafe, though.

How about something like so on top? It will allow us to slowly migrate
existing and new users over to the hopefully saner interface?

---
 include/linux/jump_label.h | 67 +++++++++++++++++++++++++++++++++++++++++++++-
 kernel/sched/core.c        | 18 ++++++-------
 2 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index f4de473f226b..98ed3c2ec78d 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -213,6 +213,71 @@ static inline bool static_key_enabled(struct static_key *key)
 	return static_key_count(key) > 0;
 }
 
-#endif	/* _LINUX_JUMP_LABEL_H */
+static inline 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);
+}
+
+static inline 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);
+}
+
+/* -------------------------------------------------------------------------- */
+
+/*
+ * likely -- default enabled, puts the branch body in-line
+ */
+
+struct static_likely_key {
+	struct static_key key;
+};
+
+#define STATIC_LIKELY_KEY_INIT	(struct static_likely_key){ .key = STATIC_KEY_INIT_TRUE, }
+
+static inline bool static_likely_branch(struct static_likely_key *key)
+{
+	return static_key_true(&key->key);
+}
+
+/*
+ * unlikely -- default disabled, puts the branch body out-of-line
+ */
+
+struct static_unlikely_key {
+	struct static_key key;
+};
+
+#define STATIC_UNLIKELY_KEY_INIT (struct static_unlikely_key){ .key = STATIC_KEY_INIT_FALSE, }
+
+static inline bool static_unlikely_branch(struct static_unlikely_key *key)
+{
+	return static_key_false(&key->key);
+}
+
+/*
+ * Advanced usage; refcount, branch is enabled when: count != 0
+ */
+
+#define static_branch_inc(_k)	static_key_slow_inc(&(_k)->key)
+#define static_branch_dec(_k)	static_key_slow_dec(&(_k)->key)
+
+/*
+ * Normal usage; boolean enable/disable.
+ */
+
+#define static_branch_enable(_k)	static_key_enable(&(_k)->key)
+#define static_branch_disable(_k)	static_key_disable(&(_k)->key)
 
 #endif /* __ASSEMBLY__ */
+#endif	/* _LINUX_JUMP_LABEL_H */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 78b4bad10081..22ba92297375 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -164,14 +164,12 @@ struct static_key sched_feat_keys[__SCHED_FEAT_NR] = {
 
 static void sched_feat_disable(int i)
 {
-	if (static_key_enabled(&sched_feat_keys[i]))
-		static_key_slow_dec(&sched_feat_keys[i]);
+	static_key_enable(&sched_feat_keys[i]);
 }
 
 static void sched_feat_enable(int i)
 {
-	if (!static_key_enabled(&sched_feat_keys[i]))
-		static_key_slow_inc(&sched_feat_keys[i]);
+	static_key_disable(&sched_feat_keys[i]);
 }
 #else
 static void sched_feat_disable(int i) { };
@@ -2318,17 +2316,17 @@ void wake_up_new_task(struct task_struct *p)
 
 #ifdef CONFIG_PREEMPT_NOTIFIERS
 
-static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE;
+static struct static_unlikely_key preempt_notifier_key = STATIC_UNLIKELY_KEY_INIT;
 
 void preempt_notifier_inc(void)
 {
-	static_key_slow_inc(&preempt_notifier_key);
+	static_branch_inc(&preempt_notifier_key);
 }
 EXPORT_SYMBOL_GPL(preempt_notifier_inc);
 
 void preempt_notifier_dec(void)
 {
-	static_key_slow_dec(&preempt_notifier_key);
+	static_branch_dec(&preempt_notifier_key);
 }
 EXPORT_SYMBOL_GPL(preempt_notifier_dec);
 
@@ -2338,7 +2336,7 @@ EXPORT_SYMBOL_GPL(preempt_notifier_dec);
  */
 void preempt_notifier_register(struct preempt_notifier *notifier)
 {
-	if (!static_key_false(&preempt_notifier_key))
+	if (!static_unlikely_branch(&preempt_notifier_key))
 		WARN(1, "registering preempt_notifier while notifiers disabled\n");
 
 	hlist_add_head(&notifier->link, &current->preempt_notifiers);
@@ -2367,7 +2365,7 @@ static void __fire_sched_in_preempt_notifiers(struct task_struct *curr)
 
 static __always_inline void fire_sched_in_preempt_notifiers(struct task_struct *curr)
 {
-	if (static_key_false(&preempt_notifier_key))
+	if (static_unlikely_branch(&preempt_notifier_key))
 		__fire_sched_in_preempt_notifiers(curr);
 }
 
@@ -2385,7 +2383,7 @@ static __always_inline void
 fire_sched_out_preempt_notifiers(struct task_struct *curr,
 				 struct task_struct *next)
 {
-	if (static_key_false(&preempt_notifier_key))
+	if (static_unlikely_branch(&preempt_notifier_key))
 		__fire_sched_out_preempt_notifiers(curr, next);
 }
 

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/10/2015 10:13 AM, Peter Zijlstra wrote:
> On Wed, Jul 08, 2015 at 05:36:43PM -0700, Andy Lutomirski wrote:
>>>> In what universe is "static_key_false" a reasonable name for a
>>>> function that returns true if a static key is true?
>> I think the current naming is almost maximally bad.  The naming would
>> be less critical if it were typesafe, though.
> How about something like so on top? It will allow us to slowly migrate
> existing and new users over to the hopefully saner interface?
>
> ---
>  include/linux/jump_label.h | 67 +++++++++++++++++++++++++++++++++++++++++++++-
>  kernel/sched/core.c        | 18 ++++++-------
>  2 files changed, 74 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index f4de473f226b..98ed3c2ec78d 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -213,6 +213,71 @@ static inline bool static_key_enabled(struct static_key *key)
>  	return static_key_count(key) > 0;
>  }
>  
> -#endif	/* _LINUX_JUMP_LABEL_H */
> +static inline 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);
> +}
> +
> +static inline 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);
> +}

should those be __static_key_enable()/disable() to indicate that we don't
that we don't want ppl using these directly. Similarly for other 'internal'
functions.

> +
> +/* -------------------------------------------------------------------------- */
> +
> +/*
> + * likely -- default enabled, puts the branch body in-line
> + */
> +
> +struct static_likely_key {
> +	struct static_key key;
> +};
> +
> +#define STATIC_LIKELY_KEY_INIT	(struct static_likely_key){ .key = STATIC_KEY_INIT_TRUE, }
> +
> +static inline bool static_likely_branch(struct static_likely_key *key)
> +{
> +	return static_key_true(&key->key);
> +}
> +
> +/*
> + * unlikely -- default disabled, puts the branch body out-of-line
> + */
> +
> +struct static_unlikely_key {
> +	struct static_key key;
> +};
> +
> +#define STATIC_UNLIKELY_KEY_INIT (struct static_unlikely_key){ .key = STATIC_KEY_INIT_FALSE, }
> +
> +static inline bool static_unlikely_branch(struct static_unlikely_key *key)
> +{
> +	return static_key_false(&key->key);
> +}
> +
> +/*
> + * Advanced usage; refcount, branch is enabled when: count != 0
> + */
> +
> +#define static_branch_inc(_k)	static_key_slow_inc(&(_k)->key)
> +#define static_branch_dec(_k)	static_key_slow_dec(&(_k)->key)
> +

I think of these as operations on the 'static_key' so I still
like 'static_key_inc()/dec()' (removing the 'slow' makes them
different still).

> +/*
> + * Normal usage; boolean enable/disable.
> + */
> +
> +#define static_branch_enable(_k)	static_key_enable(&(_k)->key)
> +#define static_branch_disable(_k)	static_key_disable(&(_k)->key)
>  

Same here maybe: static_key_set_true()/false()?

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Fri, Jul 10, 2015 at 04:13:59PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 08, 2015 at 05:36:43PM -0700, Andy Lutomirski wrote:
> > >> In what universe is "static_key_false" a reasonable name for a
> > >> function that returns true if a static key is true?
> 
> > I think the current naming is almost maximally bad.  The naming would
> > be less critical if it were typesafe, though.
> 
> How about something like so on top? It will allow us to slowly migrate
> existing and new users over to the hopefully saner interface?

Anybody? (Adding more Cc's)

> ---
>  include/linux/jump_label.h | 67 +++++++++++++++++++++++++++++++++++++++++++++-
>  kernel/sched/core.c        | 18 ++++++-------
>  2 files changed, 74 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index f4de473f226b..98ed3c2ec78d 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -213,6 +213,71 @@ static inline bool static_key_enabled(struct static_key *key)
>  	return static_key_count(key) > 0;
>  }
>  
> -#endif	/* _LINUX_JUMP_LABEL_H */
> +static inline 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);
> +}
> +
> +static inline 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);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +
> +/*
> + * likely -- default enabled, puts the branch body in-line
> + */
> +
> +struct static_likely_key {
> +	struct static_key key;
> +};
> +
> +#define STATIC_LIKELY_KEY_INIT	(struct static_likely_key){ .key = STATIC_KEY_INIT_TRUE, }
> +
> +static inline bool static_likely_branch(struct static_likely_key *key)
> +{
> +	return static_key_true(&key->key);
> +}
> +
> +/*
> + * unlikely -- default disabled, puts the branch body out-of-line
> + */
> +
> +struct static_unlikely_key {
> +	struct static_key key;
> +};
> +
> +#define STATIC_UNLIKELY_KEY_INIT (struct static_unlikely_key){ .key = STATIC_KEY_INIT_FALSE, }
> +
> +static inline bool static_unlikely_branch(struct static_unlikely_key *key)
> +{
> +	return static_key_false(&key->key);
> +}
> +
> +/*
> + * Advanced usage; refcount, branch is enabled when: count != 0
> + */
> +
> +#define static_branch_inc(_k)	static_key_slow_inc(&(_k)->key)
> +#define static_branch_dec(_k)	static_key_slow_dec(&(_k)->key)
> +
> +/*
> + * Normal usage; boolean enable/disable.
> + */
> +
> +#define static_branch_enable(_k)	static_key_enable(&(_k)->key)
> +#define static_branch_disable(_k)	static_key_disable(&(_k)->key)
>  
>  #endif /* __ASSEMBLY__ */
> +#endif	/* _LINUX_JUMP_LABEL_H */
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 78b4bad10081..22ba92297375 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -164,14 +164,12 @@ struct static_key sched_feat_keys[__SCHED_FEAT_NR] = {
>  
>  static void sched_feat_disable(int i)
>  {
> -	if (static_key_enabled(&sched_feat_keys[i]))
> -		static_key_slow_dec(&sched_feat_keys[i]);
> +	static_key_enable(&sched_feat_keys[i]);
>  }
>  
>  static void sched_feat_enable(int i)
>  {
> -	if (!static_key_enabled(&sched_feat_keys[i]))
> -		static_key_slow_inc(&sched_feat_keys[i]);
> +	static_key_disable(&sched_feat_keys[i]);
>  }
>  #else
>  static void sched_feat_disable(int i) { };
> @@ -2318,17 +2316,17 @@ void wake_up_new_task(struct task_struct *p)
>  
>  #ifdef CONFIG_PREEMPT_NOTIFIERS
>  
> -static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE;
> +static struct static_unlikely_key preempt_notifier_key = STATIC_UNLIKELY_KEY_INIT;
>  
>  void preempt_notifier_inc(void)
>  {
> -	static_key_slow_inc(&preempt_notifier_key);
> +	static_branch_inc(&preempt_notifier_key);
>  }
>  EXPORT_SYMBOL_GPL(preempt_notifier_inc);
>  
>  void preempt_notifier_dec(void)
>  {
> -	static_key_slow_dec(&preempt_notifier_key);
> +	static_branch_dec(&preempt_notifier_key);
>  }
>  EXPORT_SYMBOL_GPL(preempt_notifier_dec);
>  
> @@ -2338,7 +2336,7 @@ EXPORT_SYMBOL_GPL(preempt_notifier_dec);
>   */
>  void preempt_notifier_register(struct preempt_notifier *notifier)
>  {
> -	if (!static_key_false(&preempt_notifier_key))
> +	if (!static_unlikely_branch(&preempt_notifier_key))
>  		WARN(1, "registering preempt_notifier while notifiers disabled\n");
>  
>  	hlist_add_head(&notifier->link, &current->preempt_notifiers);
> @@ -2367,7 +2365,7 @@ static void __fire_sched_in_preempt_notifiers(struct task_struct *curr)
>  
>  static __always_inline void fire_sched_in_preempt_notifiers(struct task_struct *curr)
>  {
> -	if (static_key_false(&preempt_notifier_key))
> +	if (static_unlikely_branch(&preempt_notifier_key))
>  		__fire_sched_in_preempt_notifiers(curr);
>  }
>  
> @@ -2385,7 +2383,7 @@ static __always_inline void
>  fire_sched_out_preempt_notifiers(struct task_struct *curr,
>  				 struct task_struct *next)
>  {
> -	if (static_key_false(&preempt_notifier_key))
> +	if (static_unlikely_branch(&preempt_notifier_key))
>  		__fire_sched_out_preempt_notifiers(curr, next);
>  }
>  

Re: Kernel broken on processors without performance counters

[Thomas Gleixner]

On Tue, 21 Jul 2015, Peter Zijlstra wrote:

> On Fri, Jul 10, 2015 at 04:13:59PM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 08, 2015 at 05:36:43PM -0700, Andy Lutomirski wrote:
> > > >> In what universe is "static_key_false" a reasonable name for a
> > > >> function that returns true if a static key is true?

It might be very well our universe, you just need access to the proper
drugs to feel comfortable with it.

> > > I think the current naming is almost maximally bad.  The naming would
> > > be less critical if it were typesafe, though.
> > 
> > How about something like so on top? It will allow us to slowly migrate
> > existing and new users over to the hopefully saner interface?

> > -#endif	/* _LINUX_JUMP_LABEL_H */
> > +static inline 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);
> > +}
> > +
> > +static inline 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);
> > +}

The functions above are not part of the interface which should be used
in code, right?

To avoid further confusion, can we please move all the existing mess
and the new helpers into jump_label_internals.h and just put the new
interfaces in jump_label.h?

> > +/* -------------------------------------------------------------------------- */
> > +
> > +/*
> > + * likely -- default enabled, puts the branch body in-line
> > + */
> > +
> > +struct static_likely_key {
> > +	struct static_key key;
> > +};
> > +
> > +#define STATIC_LIKELY_KEY_INIT	(struct static_likely_key){ .key = STATIC_KEY_INIT_TRUE, }
> > +
> > +static inline bool static_likely_branch(struct static_likely_key *key)
> > +{
> > +	return static_key_true(&key->key);
> > +}
> > +
> > +/*
> > + * unlikely -- default disabled, puts the branch body out-of-line
> > + */
> > +
> > +struct static_unlikely_key {
> > +	struct static_key key;
> > +};
> > +
> > +#define STATIC_UNLIKELY_KEY_INIT (struct static_unlikely_key){ .key = STATIC_KEY_INIT_FALSE, }
> > +
> > +static inline bool static_unlikely_branch(struct static_unlikely_key *key)
> > +{
> > +	return static_key_false(&key->key);
> > +}
> > +
> > +/*
> > + * Advanced usage; refcount, branch is enabled when: count != 0
> > + */
> > +
> > +#define static_branch_inc(_k)	static_key_slow_inc(&(_k)->key)
> > +#define static_branch_dec(_k)	static_key_slow_dec(&(_k)->key)

Inlines please

> > +/*
> > + * Normal usage; boolean enable/disable.
> > + */
> > +
> > +#define static_branch_enable(_k)	static_key_enable(&(_k)->key)
> > +#define static_branch_disable(_k)	static_key_disable(&(_k)->key)

Ditto

All in all much more understandable than the existing horror.

Thanks,

	tglx

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Tue, Jul 21, 2015 at 05:43:27PM +0200, Thomas Gleixner wrote:
> On Tue, 21 Jul 2015, Peter Zijlstra wrote:
> 
> > > -#endif	/* _LINUX_JUMP_LABEL_H */
> > > +static inline 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);
> > > +}
> > > +
> > > +static inline 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);
> > > +}
> 
> The functions above are not part of the interface which should be used
> in code, right?

They are in fact. They make it easier to deal with the refcount thing
when all you want is boolean states.

> > > +/* -------------------------------------------------------------------------- */
> > > +
> > > +/*
> > > + * likely -- default enabled, puts the branch body in-line
> > > + */
> > > +
> > > +struct static_likely_key {
> > > +	struct static_key key;
> > > +};
> > > +
> > > +#define STATIC_LIKELY_KEY_INIT	(struct static_likely_key){ .key = STATIC_KEY_INIT_TRUE, }
> > > +
> > > +static inline bool static_likely_branch(struct static_likely_key *key)
> > > +{
> > > +	return static_key_true(&key->key);
> > > +}
> > > +
> > > +/*
> > > + * unlikely -- default disabled, puts the branch body out-of-line
> > > + */
> > > +
> > > +struct static_unlikely_key {
> > > +	struct static_key key;
> > > +};
> > > +
> > > +#define STATIC_UNLIKELY_KEY_INIT (struct static_unlikely_key){ .key = STATIC_KEY_INIT_FALSE, }
> > > +
> > > +static inline bool static_unlikely_branch(struct static_unlikely_key *key)
> > > +{
> > > +	return static_key_false(&key->key);
> > > +}
> > > +
> > > +/*
> > > + * Advanced usage; refcount, branch is enabled when: count != 0
> > > + */
> > > +
> > > +#define static_branch_inc(_k)	static_key_slow_inc(&(_k)->key)
> > > +#define static_branch_dec(_k)	static_key_slow_dec(&(_k)->key)
> 
> Inlines please
> 
> > > +/*
> > > + * Normal usage; boolean enable/disable.
> > > + */
> > > +
> > > +#define static_branch_enable(_k)	static_key_enable(&(_k)->key)
> > > +#define static_branch_disable(_k)	static_key_disable(&(_k)->key)
> 
> Ditto
> 
> All in all much more understandable than the existing horror.

They cannot in fact be inlines because we play type tricks. Note how _k
can be either struct static_likely_key or struct static_unlikely_key.

Re: Kernel broken on processors without performance counters

[Andy Lutomirski]

On Tue, Jul 21, 2015 at 8:49 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jul 21, 2015 at 05:43:27PM +0200, Thomas Gleixner wrote:
>> On Tue, 21 Jul 2015, Peter Zijlstra wrote:
>>
>> > > -#endif   /* _LINUX_JUMP_LABEL_H */
>> > > +static inline 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);
>> > > +}
>> > > +
>> > > +static inline 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);
>> > > +}
>>
>> The functions above are not part of the interface which should be used
>> in code, right?
>
> They are in fact. They make it easier to deal with the refcount thing
> when all you want is boolean states.
>
>> > > +/* -------------------------------------------------------------------------- */
>> > > +
>> > > +/*
>> > > + * likely -- default enabled, puts the branch body in-line
>> > > + */
>> > > +
>> > > +struct static_likely_key {
>> > > + struct static_key key;
>> > > +};
>> > > +
>> > > +#define STATIC_LIKELY_KEY_INIT   (struct static_likely_key){ .key = STATIC_KEY_INIT_TRUE, }
>> > > +
>> > > +static inline bool static_likely_branch(struct static_likely_key *key)
>> > > +{
>> > > + return static_key_true(&key->key);
>> > > +}
>> > > +
>> > > +/*
>> > > + * unlikely -- default disabled, puts the branch body out-of-line
>> > > + */
>> > > +
>> > > +struct static_unlikely_key {
>> > > + struct static_key key;
>> > > +};
>> > > +
>> > > +#define STATIC_UNLIKELY_KEY_INIT (struct static_unlikely_key){ .key = STATIC_KEY_INIT_FALSE, }
>> > > +
>> > > +static inline bool static_unlikely_branch(struct static_unlikely_key *key)
>> > > +{
>> > > + return static_key_false(&key->key);
>> > > +}
>> > > +
>> > > +/*
>> > > + * Advanced usage; refcount, branch is enabled when: count != 0
>> > > + */
>> > > +
>> > > +#define static_branch_inc(_k)    static_key_slow_inc(&(_k)->key)
>> > > +#define static_branch_dec(_k)    static_key_slow_dec(&(_k)->key)
>>
>> Inlines please
>>
>> > > +/*
>> > > + * Normal usage; boolean enable/disable.
>> > > + */
>> > > +
>> > > +#define static_branch_enable(_k) static_key_enable(&(_k)->key)
>> > > +#define static_branch_disable(_k)        static_key_disable(&(_k)->key)
>>
>> Ditto
>>
>> All in all much more understandable than the existing horror.
>
> They cannot in fact be inlines because we play type tricks. Note how _k
> can be either struct static_likely_key or struct static_unlikely_key.
>
>

To clarify my (mis-)understanding:

There are two degrees of freedom in a static_key.  They can start out
true or false, and they can be unlikely or likely.  Are those two
degrees of freedom in fact tied together?

--Andy

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Tue, Jul 21, 2015 at 08:51:51AM -0700, Andy Lutomirski wrote:
> To clarify my (mis-)understanding:
> 
> There are two degrees of freedom in a static_key.  They can start out
> true or false, and they can be unlikely or likely.  Are those two
> degrees of freedom in fact tied together?

Yes, if you start out false, you must be unlikely. If you start out
true, you must be likely.

We could maybe try and untangle that if there really is a good use case,
but this is the current state.

The whole reason this happened is because 'false' is like:


	...
	<nop>
1:
	...



label:
	<unlikely code>
	jmp 1b


Where the code if out-of-line by default. The enable will rewrite the
<nop> with a jmp label.

Of course, if you have code that is on by default, you don't want to pay
that out-of-line penalty all the time. So the on by default generates:


	...
	<nop>
	<likely code>
label:
	...


Where, if we disable, we replace the nop with jmp label.

Or rather, that all is the intent, GCC doesn't actually honour hot/cold
attributes on asm labels very well last time I tried.

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/21/2015 12:12 PM, Peter Zijlstra wrote:
> On Tue, Jul 21, 2015 at 08:51:51AM -0700, Andy Lutomirski wrote:
>> To clarify my (mis-)understanding:
>>
>> There are two degrees of freedom in a static_key.  They can start out
>> true or false, and they can be unlikely or likely.  Are those two
>> degrees of freedom in fact tied together?
> Yes, if you start out false, you must be unlikely. If you start out
> true, you must be likely.
>
> We could maybe try and untangle that if there really is a good use case,
> but this is the current state.

We could potentially allow all combinations but it requires a more
complex implementation. Perhaps, by rewriting no-ops to jmps
during build-time? Steve Rostedt posted an implementation a while
back to do that, but in part it wasn't merged due to its
complexity and the fact that it increased the kernel text, which
I don't think we ever got to the bottom of. Steve was actually
trying to reduce the size of the no-ops by first letting the compiler
pick the 'jmp' instruction size (short jumps of only 2-bytes on
x86, instead of the hard-coded 5 bytes we currently have).
However, we could use the same idea here to allow the mixed
branch label and initial variable state.

That said, it seems easy enough to reverse the direction of
the branch in an __init or so when we boot, if required.

> The whole reason this happened is because 'false' is like:
>
>
> 	...
> 	<nop>
> 1:
> 	...
>
>
>
> label:
> 	<unlikely code>
> 	jmp 1b
>
>
> Where the code if out-of-line by default. The enable will rewrite the
> <nop> with a jmp label.
>
> Of course, if you have code that is on by default, you don't want to pay
> that out-of-line penalty all the time. So the on by default generates:
>
>
> 	...
> 	<nop>
> 	<likely code>
> label:
> 	...
>
>
> Where, if we disable, we replace the nop with jmp label.
>
> Or rather, that all is the intent, GCC doesn't actually honour hot/cold
> attributes on asm labels very well last time I tried.

I tried this too not that long ago, and didn't seem to make any
difference. Ideally this could allow us to make variations where
'cold' code is moved further out-of-line.

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Steven Rostedt]

On Tue, 21 Jul 2015 12:57:08 -0400
Jason Baron <jasonbaron0@gmail.com> wrote:


> On 07/21/2015 12:12 PM, Peter Zijlstra wrote:
> > On Tue, Jul 21, 2015 at 08:51:51AM -0700, Andy Lutomirski wrote:
> >> To clarify my (mis-)understanding:
> >>
> >> There are two degrees of freedom in a static_key.  They can start out
> >> true or false, and they can be unlikely or likely.  Are those two
> >> degrees of freedom in fact tied together?
> > Yes, if you start out false, you must be unlikely. If you start out
> > true, you must be likely.
> >
> > We could maybe try and untangle that if there really is a good use case,
> > but this is the current state.
> 
> We could potentially allow all combinations but it requires a more
> complex implementation. Perhaps, by rewriting no-ops to jmps
> during build-time? Steve Rostedt posted an implementation a while
> back to do that, but in part it wasn't merged due to its
> complexity and the fact that it increased the kernel text, which
> I don't think we ever got to the bottom of. Steve was actually

I think that happened because we didn't save enough with the nops to
compensate for the code that it took to implement that change. Perhaps
in the future that will be different as the implementation is a fixed
size, while the usage of jump labels increase.

-- Steve

> trying to reduce the size of the no-ops by first letting the compiler
> pick the 'jmp' instruction size (short jumps of only 2-bytes on
> x86, instead of the hard-coded 5 bytes we currently have).
> However, we could use the same idea here to allow the mixed
> branch label and initial variable state.
> 
> That said, it seems easy enough to reverse the direction of
> the branch in an __init or so when we boot, if required.
> 

Re: Kernel broken on processors without performance counters

[Borislav Petkov]

On Tue, Jul 21, 2015 at 06:12:15PM +0200, Peter Zijlstra wrote:
> Yes, if you start out false, you must be unlikely. If you start out
> true, you must be likely.
> 
> We could maybe try and untangle that if there really is a good use case,
> but this is the current state.
> 
> The whole reason this happened is because 'false' is like:
> 
> 
> 	...
> 	<nop>
> 1:
> 	...
> 
> 
> 
> label:
> 	<unlikely code>
> 	jmp 1b
> 
> 
> Where the code if out-of-line by default. The enable will rewrite the
> <nop> with a jmp label.

Btw, native_sched_clock() is kinda botched because of that, see below.

I'd want that RDTSC to come first with a NOP preceding it which can
become a JMP in case some idiotic CPU can't do RDTSC and needs to use
jiffies. Instead, we *unconditionally* jump to RDTSC which is WTF?! We
can just as well do a normal unlikely() without the static_key:

	.globl	native_sched_clock
	.type	native_sched_clock, @function
native_sched_clock:
	pushq	%rbp	#
	movq	%rsp, %rbp	#,
#APP
# 21 "./arch/x86/include/asm/jump_label.h" 1
	1:.byte 0x0f,0x1f,0x44,0x00,0
	.pushsection __jump_table,  "aw" 
	 .balign 8 
	 .quad 1b, .L122, __use_tsc 	#,
	.popsection 
	
# 0 "" 2
#NO_APP
	movabsq	$-4294667296000000, %rax	#, tmp118
	popq	%rbp	#
	imulq	$1000000, jiffies_64(%rip), %rdx	#, jiffies_64, D.28443
	addq	%rdx, %rax	# D.28443, D.28443
	ret
.L122:
#APP
# 118 "./arch/x86/include/asm/msr.h" 1
	rdtsc
# 0 "" 2

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/21/2015 02:15 PM, Borislav Petkov wrote:
> On Tue, Jul 21, 2015 at 06:12:15PM +0200, Peter Zijlstra wrote:
>> Yes, if you start out false, you must be unlikely. If you start out
>> true, you must be likely.
>>
>> We could maybe try and untangle that if there really is a good use case,
>> but this is the current state.
>>
>> The whole reason this happened is because 'false' is like:
>>
>>
>> 	...
>> 	<nop>
>> 1:
>> 	...
>>
>>
>>
>> label:
>> 	<unlikely code>
>> 	jmp 1b
>>
>>
>> Where the code if out-of-line by default. The enable will rewrite the
>> <nop> with a jmp label.
> Btw, native_sched_clock() is kinda botched because of that, see below.
>
> I'd want that RDTSC to come first with a NOP preceding it which can
> become a JMP in case some idiotic CPU can't do RDTSC and needs to use
> jiffies. Instead, we *unconditionally* jump to RDTSC which is WTF?! We
> can just as well do a normal unlikely() without the static_key:

hmmm...so this is a case where need to the default the branch
to the out-of-line branch at boot. That is, we can't just enable
the out-of-line branch at boot time, b/c it might be too late at
that point? IE native_sched_clock() gets called very early?

Thanks,

-Jason

>
> 	.globl	native_sched_clock
> 	.type	native_sched_clock, @function
> native_sched_clock:
> 	pushq	%rbp	#
> 	movq	%rsp, %rbp	#,
> #APP
> # 21 "./arch/x86/include/asm/jump_label.h" 1
> 	1:.byte 0x0f,0x1f,0x44,0x00,0
> 	.pushsection __jump_table,  "aw" 
> 	 .balign 8 
> 	 .quad 1b, .L122, __use_tsc 	#,
> 	.popsection 
> 	
> # 0 "" 2
> #NO_APP
> 	movabsq	$-4294667296000000, %rax	#, tmp118
> 	popq	%rbp	#
> 	imulq	$1000000, jiffies_64(%rip), %rdx	#, jiffies_64, D.28443
> 	addq	%rdx, %rax	# D.28443, D.28443
> 	ret
> .L122:
> #APP
> # 118 "./arch/x86/include/asm/msr.h" 1
> 	rdtsc
> # 0 "" 2
>

Re: Kernel broken on processors without performance counters

[Andy Lutomirski]

On Tue, Jul 21, 2015 at 11:50 AM, Jason Baron <jasonbaron0@gmail.com> wrote:
>
>
> On 07/21/2015 02:15 PM, Borislav Petkov wrote:
>> On Tue, Jul 21, 2015 at 06:12:15PM +0200, Peter Zijlstra wrote:
>>> Yes, if you start out false, you must be unlikely. If you start out
>>> true, you must be likely.
>>>
>>> We could maybe try and untangle that if there really is a good use case,
>>> but this is the current state.
>>>
>>> The whole reason this happened is because 'false' is like:
>>>
>>>
>>>      ...
>>>      <nop>
>>> 1:
>>>      ...
>>>
>>>
>>>
>>> label:
>>>      <unlikely code>
>>>      jmp 1b
>>>
>>>
>>> Where the code if out-of-line by default. The enable will rewrite the
>>> <nop> with a jmp label.
>> Btw, native_sched_clock() is kinda botched because of that, see below.
>>
>> I'd want that RDTSC to come first with a NOP preceding it which can
>> become a JMP in case some idiotic CPU can't do RDTSC and needs to use
>> jiffies. Instead, we *unconditionally* jump to RDTSC which is WTF?! We
>> can just as well do a normal unlikely() without the static_key:
>
> hmmm...so this is a case where need to the default the branch
> to the out-of-line branch at boot. That is, we can't just enable
> the out-of-line branch at boot time, b/c it might be too late at
> that point? IE native_sched_clock() gets called very early?
>

Could this be done at link time, or perhaps when compressing the
kernel image, instead of at boot time?

--Andy

Re: Kernel broken on processors without performance counters

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On Tue, 21 Jul 2015 11:54:30 -0700, Andy Lutomirski said:

> Could this be done at link time, or perhaps when compressing the
> kernel image, instead of at boot time?

That's only safe to do if the kernel is built for one specific CPU - if it's
a generic kernel that boots on multiple hardware designs, it will be wrong
for some systems.

In other words - safe to do if you're building it for *your* hardware. Totally
unsafe for a distro.

[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

Re: Kernel broken on processors without performance counters

[Andy Lutomirski]

On Tue, Jul 21, 2015 at 12:00 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Tue, 21 Jul 2015 11:54:30 -0700, Andy Lutomirski said:
>
>> Could this be done at link time, or perhaps when compressing the
>> kernel image, instead of at boot time?
>
> That's only safe to do if the kernel is built for one specific CPU - if it's
> a generic kernel that boots on multiple hardware designs, it will be wrong
> for some systems.
>
> In other words - safe to do if you're building it for *your* hardware. Totally
> unsafe for a distro.

That's not what I meant.  We do something in the C code that tells the
build step which way the initial state goes.  At link time, we make
the initial state actually work like that.  Then, at run time, we can
still switch it again if needed.

--Andy

Re: Kernel broken on processors without performance counters

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Tue, 21 Jul 2015 12:29:21 -0700, Andy Lutomirski said:

> That's not what I meant.  We do something in the C code that tells the
> build step which way the initial state goes.  At link time, we make
> the initial state actually work like that.  Then, at run time, we can
> still switch it again if needed.

OK, that's something different than what I read the first time around.

I'm thinking that a good adjective would be "brittle", in the face of
recalcitrant GCC releases.  Look at the fun we've had over the years
getting things that *should* be fairly intuitive to work correctly (such
as "inline").

Having said that, if somebody comes up with something that actually
works, I'd be OK with it...

[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

Re: Kernel broken on processors without performance counters

[Borislav Petkov]

On Tue, Jul 21, 2015 at 02:50:25PM -0400, Jason Baron wrote:
> hmmm...so this is a case where need to the default the branch
> to the out-of-line branch at boot. That is, we can't just enable
> the out-of-line branch at boot time, b/c it might be too late at
> that point? IE native_sched_clock() gets called very early?

Well, even the layout is wrong here. The optimal thing would be to have:

	NOP
	rdtsc

unlikely:
	/* read jiffies */

at build time. And then at boot time, patch in the JMP over the NOP on
!use_tsc boxes. And RDTSC works always, no matter how early.

I'm fairly sure we can do that now with alternatives instead of jump
labels.

The problem currently is that the 5-byte NOP gets patched in with a JMP
so we have an unconditional forwards JMP to the RDTSC.

Now I'd put my money on most arches handling NOPs better then
unconditional JMPs and this is a hot path...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/22/2015 12:24 AM, Borislav Petkov wrote:
> On Tue, Jul 21, 2015 at 02:50:25PM -0400, Jason Baron wrote:
>> hmmm...so this is a case where need to the default the branch
>> to the out-of-line branch at boot. That is, we can't just enable
>> the out-of-line branch at boot time, b/c it might be too late at
>> that point? IE native_sched_clock() gets called very early?
> Well, even the layout is wrong here. The optimal thing would be to have:
>
> 	NOP
> 	rdtsc
>
> unlikely:
> 	/* read jiffies */
>
> at build time. And then at boot time, patch in the JMP over the NOP on
> !use_tsc boxes. And RDTSC works always, no matter how early.
>
> I'm fairly sure we can do that now with alternatives instead of jump
> labels.
>
> The problem currently is that the 5-byte NOP gets patched in with a JMP
> so we have an unconditional forwards JMP to the RDTSC.
>
> Now I'd put my money on most arches handling NOPs better then
> unconditional JMPs and this is a hot path...
>

Ok,

So we could add all 4 possible initial states, where the
branches would be:

static_likely_init_true_branch(struct static_likely_init_true_key *key)
static_likely_init_false_branch(struct static_likely_init_false_key *key)
static_unlikely_init_false_branch(struct static_unlikely_init_false_key *key)
static_unlikely_init_true_branch(struct static_unlikely_init_true_key *key)

So, this last one means 'inline' the false branch, but start the branch as
true. Which is, I think what you want here.

So this addresses the use-case of 'initial' branch direction doesn't
match the the default branch bias. We could also do this with link
time time patching (and potentially reduce the need for 4 different
types), but the implementation below doesn't seem too bad :) Its based
upon Peter's initial patch. It unfortunately does require some arch
specific bits (so we probably need a 'HAVE_ARCH', wrapper until we
have support.

Now, the native_sched_clock() starts as:

ffffffff8200bf10 <native_sched_clock>:
ffffffff8200bf10:       55                      push   %rbp
ffffffff8200bf11:       48 89 e5                mov    %rsp,%rbp
ffffffff8200bf14:       eb 30                   jmp    ffffffff8200bf46 <native_sched_clock+0x36>
ffffffff8200bf16:       0f 31                   rdtsc

And then the '0x3b' gets patch to a 2-byte no-op

Thanks,

-Jason


diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index a4c1cf7..030cf52 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -30,6 +30,20 @@ l_yes:
     return true;
 }
 
+static __always_inline bool arch_static_branch_jump(struct static_key *key)
+{
+    asm_volatile_goto("1:"
+        "jmp %l[l_yes]\n\t"
+        ".pushsection __jump_table,  \"aw\" \n\t"
+        _ASM_ALIGN "\n\t"
+        _ASM_PTR "1b, %l[l_yes], %c0 \n\t"
+        ".popsection \n\t"
+        : :  "i" (key) : : l_yes);
+    return false;
+l_yes:
+    return true;
+}
+
 #ifdef CONFIG_X86_64
 typedef u64 jump_label_t;
 #else
diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index 26d5a55..d40b19d 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -22,6 +22,10 @@ union jump_code_union {
         char jump;
         int offset;
     } __attribute__((packed));
+    struct {
+        char jump_short;
+        char offset_short;
+    } __attribute__((packed));
 };
 
 static void bug_at(unsigned char *ip, int line)
@@ -36,6 +40,8 @@ static void bug_at(unsigned char *ip, int line)
     BUG();
 }
 
+static const unsigned char short_nop[] = { P6_NOP2 };
+
 static void __jump_label_transform(struct jump_entry *entry,
                    enum jump_label_type type,
                    void *(*poker)(void *, const void *, size_t),
@@ -44,47 +50,44 @@ static void __jump_label_transform(struct jump_entry *entry,
     union jump_code_union code;
     const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
     const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
+    void *instruct = (void *)entry->code;
+    unsigned size = JUMP_LABEL_NOP_SIZE;
+    unsigned char opcode = *(unsigned char *)instruct;
 
     if (type == JUMP_LABEL_ENABLE) {
-        if (init) {
-            /*
-             * Jump label is enabled for the first time.
-             * So we expect a default_nop...
-             */
-            if (unlikely(memcmp((void *)entry->code, default_nop, 5)
-                     != 0))
-                bug_at((void *)entry->code, __LINE__);
-        } else {
-            /*
-             * ...otherwise expect an ideal_nop. Otherwise
-             * something went horribly wrong.
-             */
-            if (unlikely(memcmp((void *)entry->code, ideal_nop, 5)
-                     != 0))
-                bug_at((void *)entry->code, __LINE__);
-        }
+        if (opcode == 0xe9 || opcode == 0xeb)
+            return;
 
-        code.jump = 0xe9;
-        code.offset = entry->target -
-                (entry->code + JUMP_LABEL_NOP_SIZE);
-    } else {
-        /*
-         * We are disabling this jump label. If it is not what
-         * we think it is, then something must have gone wrong.
-         * If this is the first initialization call, then we
-         * are converting the default nop to the ideal nop.
-         */
-        if (init) {
-            if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
+        if (memcmp(instruct, short_nop, 2) == 0)
+            size = 2;
+        else if (!(memcmp(instruct, ideal_nop, 5) == 0) &&
+             !(memcmp(instruct, default_nop, 5) == 0))
                 bug_at((void *)entry->code, __LINE__);
+
+        if (size != JUMP_LABEL_NOP_SIZE) {
+            code.jump_short = 0xeb;
+            code.offset = entry->target - (entry->code + size);
         } else {
             code.jump = 0xe9;
             code.offset = entry->target -
-                (entry->code + JUMP_LABEL_NOP_SIZE);
-            if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))
-                bug_at((void *)entry->code, __LINE__);
+                    (entry->code + size);
         }
-        memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
+    } else {
+        if (memcmp(instruct, short_nop, 2) == 0)
+            return;
+
+        if (memcmp(instruct, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE) == 0)
+            return;
+
+        if (opcode == 0xeb)
+            size = 2;
+        else if ((opcode != 0xe9) && !(memcmp(instruct, default_nop, JUMP_LABEL_NOP_SIZE) == 0))
+            bug_at((void *)entry->code, __LINE__);
+
+        if (size != JUMP_LABEL_NOP_SIZE)
+            memcpy(&code, short_nop, size);
+        else
+            memcpy(&code, ideal_nops[NOP_ATOMIC5], size);
     }
 
     /*
@@ -96,10 +99,10 @@ static void __jump_label_transform(struct jump_entry *entry,
      *
      */
     if (poker)
-        (*poker)((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
+        (*poker)((void *)entry->code, &code, size);
     else
-        text_poke_bp((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE,
-                 (void *)entry->code + JUMP_LABEL_NOP_SIZE);
+        text_poke_bp((void *)entry->code, &code, size,
+                 (void *)entry->code + size);
 }
 
 void arch_jump_label_transform(struct jump_entry *entry,
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 5054497..192c92f 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -38,7 +38,7 @@ static int __read_mostly tsc_unstable;
    erroneous rdtsc usage on !cpu_has_tsc processors */
 static int __read_mostly tsc_disabled = -1;
 
-static struct static_key __use_tsc = STATIC_KEY_INIT;
+static struct static_unlikely_init_true_key __use_tsc = STATIC_KEY_UNLIKELY_INIT_TRUE;
 
 int tsc_clocksource_reliable;
 
@@ -284,7 +284,7 @@ u64 native_sched_clock(void)
      *   very important for it to be as fast as the platform
      *   can achieve it. )
      */
-    if (!static_key_false(&__use_tsc)) {
+    if (static_unlikely_init_true_branch(&__use_tsc)) {
         /* No locking but a rare wrong value is not a big deal: */
         return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
     }
@@ -1201,7 +1201,7 @@ void __init tsc_init(void)
     /* now allow native_sched_clock() to use rdtsc */
 
     tsc_disabled = 0;
-    static_key_slow_inc(&__use_tsc);
+    static_branch_dec(&__use_tsc);
 
     if (!no_sched_irq_time)
         enable_sched_clock_irqtime();
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index f4de473..558fef0 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -130,6 +130,16 @@ static __always_inline bool static_key_true(struct static_key *key)
     return !static_key_false(key);
 }
 
+static __always_inline bool static_key_false_jump(struct static_key *key)
+{
+    return arch_static_branch_jump(key);
+}
+
+static __always_inline bool static_key_true_jump(struct static_key *key)
+{
+    return !static_key_false_jump(key);
+}
+
 extern struct jump_entry __start___jump_table[];
 extern struct jump_entry __stop___jump_table[];
 
@@ -152,6 +162,20 @@ extern void jump_label_apply_nops(struct module *mod);
     { .enabled = ATOMIC_INIT(0),                \
       .entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
 
+#define STATIC_KEY_LIKEY_INIT_TRUE ((struct static_unlikely_init_true_key)        \
+    { .key.enabled = ATOMIC_INIT(1),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
+#define STATIC_KEY_LIKEY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
+    { .key.enabled = ATOMIC_INIT(0),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
+
+#define STATIC_KEY_UNLIKELY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
+    { .key.enabled = ATOMIC_INIT(0),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
+#define STATIC_KEY_UNLIKELY_INIT_TRUE ((struct static_unlikely_init_true_key)    \
+    { .key.enabled = ATOMIC_INIT(1),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
+
 #else  /* !HAVE_JUMP_LABEL */
 
 static __always_inline void jump_label_init(void)
@@ -165,6 +189,7 @@ static __always_inline bool static_key_false(struct static_key *key)
         return true;
     return false;
 }
+#define static_key_false_jump static_key_false
 
 static __always_inline bool static_key_true(struct static_key *key)
 {
@@ -172,6 +197,7 @@ static __always_inline bool static_key_true(struct static_key *key)
         return true;
     return false;
 }
+#define static_key_true_jump static_key_true
 
 static inline void static_key_slow_inc(struct static_key *key)
 {
@@ -203,6 +229,15 @@ static inline int jump_label_apply_nops(struct module *mod)
 #define STATIC_KEY_INIT_FALSE ((struct static_key) \
         { .enabled = ATOMIC_INIT(0) })
 
+#define STATIC_KEY_LIKEY_INIT_TRUE ((struct static_key)        \
+        { .enabled = ATOMIC_INIT(1) })
+#define STATIC_KEY_LIKEY_INIT_FALSE ((struct static_key)    \
+        { .enabled = ATOMIC_INIT(0) })
+#define STATIC_KEY_UNLIKELY_INIT_FALSE ((struct static_key)    \
+        { .enabled = ATOMIC_INIT(0) })
+#define STATIC_KEY_UNLIKELY_INIT_TRUE ((struct static_key)    \
+        { .enabled = ATOMIC_INIT(1) })
+
 #endif    /* HAVE_JUMP_LABEL */
 
 #define STATIC_KEY_INIT STATIC_KEY_INIT_FALSE
@@ -213,6 +248,85 @@ static inline bool static_key_enabled(struct static_key *key)
     return static_key_count(key) > 0;
 }
 
-#endif    /* _LINUX_JUMP_LABEL_H */
+static inline 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);
+}
+
+static inline 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);
+}
+
+/* -------------------------------------------------------------------------- */
+
+/*
+ * likely -- default enabled, puts the branch body in-line
+ */
+
+struct static_likely_init_true_key {
+    struct static_key key;
+};
+
+struct static_likely_init_false_key {
+    struct static_key key;
+};
+
+static inline bool static_likely_init_true_branch(struct static_likely_init_true_key *key)
+{
+    return static_key_true(&key->key);
+}
+
+static inline bool static_likely_init_false_branch(struct static_likely_init_false_key *key)
+{
+    return static_key_true_jump(&key->key);
+}
+
+/*
+ * unlikely -- default disabled, puts the branch body out-of-line
+ */
+
+struct static_unlikely_init_false_key {
+    struct static_key key;
+};
+
+struct static_unlikely_init_true_key {
+    struct static_key key;
+};
+
+static inline bool static_unlikely_init_false_branch(struct static_unlikely_init_false_key *key)
+{
+    return static_key_false(&key->key);
+}
+
+static inline bool static_unlikely_init_true_branch(struct static_unlikely_init_true_key *key)
+{
+    return static_key_false_jump(&key->key);
+}
+
+/*
+ * Advanced usage; refcount, branch is enabled when: count != 0
+ */
+
+#define static_branch_inc(_k)    static_key_slow_inc(&(_k)->key)
+#define static_branch_dec(_k)    static_key_slow_dec(&(_k)->key)
+
+/*
+ * Normal usage; boolean enable/disable.
+ */
+
+#define static_branch_enable(_k)    static_key_enable(&(_k)->key)
+#define static_branch_disable(_k)    static_key_disable(&(_k)->key)
 
 #endif /* __ASSEMBLY__ */
+#endif    /* _LINUX_JUMP_LABEL_H */
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 9019f15..47a6478 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -203,7 +203,8 @@ void __init jump_label_init(void)
         struct static_key *iterk;
 
         iterk = (struct static_key *)(unsigned long)iter->key;
-        arch_jump_label_transform_static(iter, jump_label_type(iterk));
+        if (jump_label_type(iterk) == JUMP_LABEL_DISABLE)
+            arch_jump_label_transform_static(iter, JUMP_LABEL_DISABLE);
         if (iterk == key)
             continue;
 
@@ -318,8 +319,7 @@ static int jump_label_add_module(struct module *mod)
         jlm->next = key->next;
         key->next = jlm;
 
-        if (jump_label_type(key) == JUMP_LABEL_ENABLE)
-            __jump_label_update(key, iter, iter_stop, JUMP_LABEL_ENABLE);
+        __jump_label_update(key, iter, iter_stop, jump_label_type(key));
     }
 
     return 0;

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Wed, Jul 22, 2015 at 01:06:44PM -0400, Jason Baron wrote:
> Ok,
> 
> So we could add all 4 possible initial states, where the
> branches would be:
> 
> static_likely_init_true_branch(struct static_likely_init_true_key *key)
> static_likely_init_false_branch(struct static_likely_init_false_key *key)
> static_unlikely_init_false_branch(struct static_unlikely_init_false_key *key)
> static_unlikely_init_true_branch(struct static_unlikely_init_true_key *key)

I'm sorely tempted to go quote cypress hill here...


And I realize part of the problem is that we're wanting to use jump
labels before we can patch them. But surely we can do better.

extern bool ____wrong_branch_error(void);

struct static_key_true;
struct static_key_false;

#define static_branch_likely(x)							\
({										\
	bool branch;								\
	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
		branch = !arch_static_branch(&(x)->key);			\
	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
		branch = !arch_static_branch_jump(&(x)->key);			\
	else									\
		branch = ____wrong_branch_error();				\
	branch;									\
})

#define static_branch_unlikely(x)						\
({										\
	bool branch;								\
	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
		branch = arch_static_branch(&(x)->key);				\
	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
		branch = arch_static_branch_jump(&(x)->key);			\
	else									\
		branch = ____wrong_branch_error();				\
	branch;									\
})

Can't we make something like that work?

So the immediate problem appears to be the 4 different key inits, which don't
seem very supportive of this separation:

+#define STATIC_KEY_LIKEY_INIT_TRUE ((struct static_unlikely_init_true_key)        \
+    { .key.enabled = ATOMIC_INIT(1),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })

+#define STATIC_KEY_LIKEY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
+    { .key.enabled = ATOMIC_INIT(0),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })

+#define STATIC_KEY_UNLIKELY_INIT_TRUE ((struct static_unlikely_init_true_key)    \
+    { .key.enabled = ATOMIC_INIT(1),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })

+#define STATIC_KEY_UNLIKELY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
+    { .key.enabled = ATOMIC_INIT(0),                \
+      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })


But I think we can fix that by using a second __jump_table section, then
we can augment the LABEL_TYPE_{TRUE,FALSE} thing with the section we
find the jump_entry in.

Then we can do:

#define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
#define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }

And invert the jump_label_type if we're in the second section.

I think we'll need a second argument to the arch_static_branch*()
functions to indicate which section it needs to go in.


static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
{
	if (!inv) {
		asm_volatile_goto("1:"
			".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
			".pushsection __jump_table,  \"aw\" \n\t"
			_ASM_ALIGN "\n\t"
			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
			".popsection \n\t"
			: :  "i" (key) : : l_yes);
	} else {
		asm_volatile_goto("1:"
			".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
			".pushsection __jump_table_inv,  \"aw\" \n\t"
			_ASM_ALIGN "\n\t"
			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
			".popsection \n\t"
			: :  "i" (key) : : l_yes);
	}
	return false;
l_yes:
	return true;
}

static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
{
	if (!inv) {
		asm_volatile_goto("1:"
			"jmp %l[l_yes]\n\t"
			".pushsection __jump_table,  \"aw\" \n\t"
			_ASM_ALIGN "\n\t"
			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
			".popsection \n\t"
			: :  "i" (key) : : l_yes);
	} else {
		asm_volatile_goto("1:"
			"jmp %l[l_yes]\n\t"
			".pushsection __jump_table_inv,  \"aw\" \n\t"
			_ASM_ALIGN "\n\t"
			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
			".popsection \n\t"
			: :  "i" (key) : : l_yes);
	}
	return false;
l_yes:
	return true;
}


And change the branch macros thusly:


#define static_branch_likely(x)							\
({										\
	bool branch;								\
	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
		branch = !arch_static_branch(&(x)->key, false);			\
	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
		branch = !arch_static_branch_jump(&(x)->key, true);		\
	else									\
		branch = ____wrong_branch_error();				\
	branch;									\
})

#define static_branch_unlikely(x)						\
({										\
	bool branch;								\
	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
		branch = arch_static_branch(&(x)->key, true);			\
	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
		branch = arch_static_branch_jump(&(x)->key, false);		\
	else									\
		branch = ____wrong_branch_error();				\
	branch;									\
})


And I think it'll all work. Hmm?

Re: Kernel broken on processors without performance counters

[Borislav Petkov]

On Thu, Jul 23, 2015 at 12:42:15PM +0200, Peter Zijlstra wrote:
> > static_likely_init_true_branch(struct static_likely_init_true_key *key)
> > static_likely_init_false_branch(struct static_likely_init_false_key *key)
> > static_unlikely_init_false_branch(struct static_unlikely_init_false_key *key)
> > static_unlikely_init_true_branch(struct static_unlikely_init_true_key *key)
> 
> I'm sorely tempted to go quote cypress hill here...

Yah, those are at least too long and nuts.

> And I realize part of the problem is that we're wanting to use jump
> labels before we can patch them. But surely we can do better.
> 
> extern bool ____wrong_branch_error(void);
> 
> struct static_key_true;
> struct static_key_false;
> 
> #define static_branch_likely(x)							\
> ({										\
> 	bool branch;								\
> 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> 		branch = !arch_static_branch(&(x)->key);			\
> 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> 		branch = !arch_static_branch_jump(&(x)->key);			\
> 	else									\
> 		branch = ____wrong_branch_error();				\
> 	branch;									\
> })
> 
> #define static_branch_unlikely(x)						\
> ({										\
> 	bool branch;								\
> 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> 		branch = arch_static_branch(&(x)->key);				\
> 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> 		branch = arch_static_branch_jump(&(x)->key);			\
> 	else									\
> 		branch = ____wrong_branch_error();				\
> 	branch;									\
> })
> 
> Can't we make something like that work?
> 
> So the immediate problem appears to be the 4 different key inits, which don't
> seem very supportive of this separation:
> 
> +#define STATIC_KEY_LIKEY_INIT_TRUE ((struct static_unlikely_init_true_key)        \

LIKELY

> +    { .key.enabled = ATOMIC_INIT(1),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
> 
> +#define STATIC_KEY_LIKEY_INIT_FALSE ((struct static_unlikely_init_false_key)    \

Yuck, those struct names are still too long IMO.

> +    { .key.enabled = ATOMIC_INIT(0),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
> 
> +#define STATIC_KEY_UNLIKELY_INIT_TRUE ((struct static_unlikely_init_true_key)    \
> +    { .key.enabled = ATOMIC_INIT(1),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
> 
> +#define STATIC_KEY_UNLIKELY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
> +    { .key.enabled = ATOMIC_INIT(0),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
> 
> 
> But I think we can fix that by using a second __jump_table section, then
> we can augment the LABEL_TYPE_{TRUE,FALSE} thing with the section we
> find the jump_entry in.
> 
> Then we can do:
> 
> #define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
> #define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }

Let's abbreviate that "STATIC_KEY" thing too:

SK_TRUE_INIT
SK_FALSE_INIT
...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/23/2015 06:42 AM, Peter Zijlstra wrote:
> On Wed, Jul 22, 2015 at 01:06:44PM -0400, Jason Baron wrote:
>> Ok,
>>
>> So we could add all 4 possible initial states, where the
>> branches would be:
>>
>> static_likely_init_true_branch(struct static_likely_init_true_key *key)
>> static_likely_init_false_branch(struct static_likely_init_false_key *key)
>> static_unlikely_init_false_branch(struct static_unlikely_init_false_key *key)
>> static_unlikely_init_true_branch(struct static_unlikely_init_true_key *key)
> I'm sorely tempted to go quote cypress hill here...
>
>
> And I realize part of the problem is that we're wanting to use jump
> labels before we can patch them. But surely we can do better.
>
> extern bool ____wrong_branch_error(void);
>
> struct static_key_true;
> struct static_key_false;
>
> #define static_branch_likely(x)							\
> ({										\
> 	bool branch;								\
> 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> 		branch = !arch_static_branch(&(x)->key);			\
> 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> 		branch = !arch_static_branch_jump(&(x)->key);			\
> 	else									\
> 		branch = ____wrong_branch_error();				\
> 	branch;									\
> })
>
> #define static_branch_unlikely(x)						\
> ({										\
> 	bool branch;								\
> 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> 		branch = arch_static_branch(&(x)->key);				\
> 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> 		branch = arch_static_branch_jump(&(x)->key);			\
> 	else									\
> 		branch = ____wrong_branch_error();				\
> 	branch;									\
> })
>
> Can't we make something like that work?
>
> So the immediate problem appears to be the 4 different key inits, which don't
> seem very supportive of this separation:
>
> +#define STATIC_KEY_LIKEY_INIT_TRUE ((struct static_unlikely_init_true_key)        \
> +    { .key.enabled = ATOMIC_INIT(1),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
>
> +#define STATIC_KEY_LIKEY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
> +    { .key.enabled = ATOMIC_INIT(0),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_TRUE_BRANCH })
>
> +#define STATIC_KEY_UNLIKELY_INIT_TRUE ((struct static_unlikely_init_true_key)    \
> +    { .key.enabled = ATOMIC_INIT(1),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
>
> +#define STATIC_KEY_UNLIKELY_INIT_FALSE ((struct static_unlikely_init_false_key)    \
> +    { .key.enabled = ATOMIC_INIT(0),                \
> +      .key.entries = (void *)JUMP_LABEL_TYPE_FALSE_BRANCH })
>
>
> But I think we can fix that by using a second __jump_table section, then
> we can augment the LABEL_TYPE_{TRUE,FALSE} thing with the section we
> find the jump_entry in.
>
> Then we can do:
>
> #define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
> #define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }
>
> And invert the jump_label_type if we're in the second section.
>
> I think we'll need a second argument to the arch_static_branch*()
> functions to indicate which section it needs to go in.
>
>
> static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
> {
> 	if (!inv) {
> 		asm_volatile_goto("1:"
> 			".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
> 			".pushsection __jump_table,  \"aw\" \n\t"
> 			_ASM_ALIGN "\n\t"
> 			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 			".popsection \n\t"
> 			: :  "i" (key) : : l_yes);
> 	} else {
> 		asm_volatile_goto("1:"
> 			".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
> 			".pushsection __jump_table_inv,  \"aw\" \n\t"
> 			_ASM_ALIGN "\n\t"
> 			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 			".popsection \n\t"
> 			: :  "i" (key) : : l_yes);
> 	}
> 	return false;
> l_yes:
> 	return true;
> }
>
> static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> {
> 	if (!inv) {
> 		asm_volatile_goto("1:"
> 			"jmp %l[l_yes]\n\t"
> 			".pushsection __jump_table,  \"aw\" \n\t"
> 			_ASM_ALIGN "\n\t"
> 			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 			".popsection \n\t"
> 			: :  "i" (key) : : l_yes);
> 	} else {
> 		asm_volatile_goto("1:"
> 			"jmp %l[l_yes]\n\t"
> 			".pushsection __jump_table_inv,  \"aw\" \n\t"
> 			_ASM_ALIGN "\n\t"
> 			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 			".popsection \n\t"
> 			: :  "i" (key) : : l_yes);
> 	}
> 	return false;
> l_yes:
> 	return true;
> }
>
>
> And change the branch macros thusly:
>
>
> #define static_branch_likely(x)							\
> ({										\
> 	bool branch;								\
> 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> 		branch = !arch_static_branch(&(x)->key, false);			\
> 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> 		branch = !arch_static_branch_jump(&(x)->key, true);		\
> 	else									\
> 		branch = ____wrong_branch_error();				\
> 	branch;									\
> })
>
> #define static_branch_unlikely(x)						\
> ({										\
> 	bool branch;								\
> 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> 		branch = arch_static_branch(&(x)->key, true);			\
> 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> 		branch = arch_static_branch_jump(&(x)->key, false);		\
> 	else									\
> 		branch = ____wrong_branch_error();				\
> 	branch;									\
> })
>

In 'static_branch_unlikely()', I think  arch_static_branch() and
arch_static_branch_jump() are reversed. Another way to do the 'inv'
thing is to simply have the likely() branches all be in one table and
the unlikely() in the other. Then, for example for a given  _inc()
operation we would no-op all the likely() branches and jmp all the
unlikely().

> And I think it'll all work. Hmm?

Cool. This also gives an extra degree of freedom in that it allows keys to
be arbitrarily mixed with the likely/unlikely branch types. I'm not sure that's
come up as a use-case, but seems like it would be good. It also implies
that the LABEL_TYPE_{TRUE,FALSE}, is no longer associated with the key
b/c a key could be used in both and unlikely() or likely() branch. So that
would eventually go away, and the 'struct static_key()', I guess could point
to its relevant entries in both tables. Although, that means an extra
pointer in the 'struct static_key'. It may be simpler to simply add another
field to the jump table that specifies if the branch is likely/unlikely,
and then we are back to one table? IE  arch_static_branch() could add
that field to the jump table.

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 10:19:52AM -0400, Jason Baron wrote:
> > And I think it'll all work. Hmm?
> 
> Cool. This also gives an extra degree of freedom in that it allows keys to
> be arbitrarily mixed with the likely/unlikely branch types. I'm not sure that's
> come up as a use-case, but seems like it would be good. It also implies
> that the LABEL_TYPE_{TRUE,FALSE}, is no longer associated with the key
> b/c a key could be used in both and unlikely() or likely() branch. So that
> would eventually go away, and the 'struct static_key()', I guess could point
> to its relevant entries in both tables. Although, that means an extra
> pointer in the 'struct static_key'. It may be simpler to simply add another
> field to the jump table that specifies if the branch is likely/unlikely,
> and then we are back to one table? IE  arch_static_branch() could add
> that field to the jump table.

Way ahead of you, while implementing the dual section I ran into trouble
and found that it would be far easier to indeed stick it in the
jump_entry.

However, instead of growing the thing, I've used the LSB of the key
field, that's a pointer so it has at least two bits free anyhow.

I've also implemented it for all archs (+- compile failures, I've not
gotten that far).

Lemme finish this and I'll post it.

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 04:33:08PM +0200, Peter Zijlstra wrote:
> Lemme finish this and I'll post it.

Compile tested on x86_64 only..

Please have a look, I think you said I got some of the logic wrong, I've
not double checked that.

I'll go write comments and double check things.

---
 arch/arm/include/asm/jump_label.h     |  22 +++++++-
 arch/arm64/include/asm/jump_label.h   |  22 +++++++-
 arch/mips/include/asm/jump_label.h    |  23 +++++++-
 arch/powerpc/include/asm/jump_label.h |  23 +++++++-
 arch/s390/include/asm/jump_label.h    |  23 +++++++-
 arch/sparc/include/asm/jump_label.h   |  38 ++++++++++---
 arch/x86/include/asm/jump_label.h     |  24 +++++++-
 include/linux/jump_label.h            | 101 +++++++++++++++++++++++++++++++++-
 kernel/jump_label.c                   |  89 +++++++++++++++---------------
 9 files changed, 297 insertions(+), 68 deletions(-)

diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h
index 5f337dc5c108..6c9789b33497 100644
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -13,14 +13,32 @@
 #define JUMP_LABEL_NOP	"nop"
 #endif
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
+	unsigned long kval = (unsigned long)key + inv;
+
 	asm_volatile_goto("1:\n\t"
 		 JUMP_LABEL_NOP "\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".word 1b, %l[l_yes], %c0\n\t"
 		 ".popsection\n\t"
-		 : :  "i" (key) :  : l_yes);
+		 : :  "i" (kval) :  : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("1:\n\t"
+		 "b %l[l_yes]\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection\n\t"
+		 : :  "i" (kval) :  : l_yes);
 
 	return false;
 l_yes:
diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
index c0e5165c2f76..e5cda5d75c62 100644
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -26,14 +26,32 @@
 
 #define JUMP_LABEL_NOP_SIZE		AARCH64_INSN_SIZE
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
+	unsigned long kval = (unsigned long)key + inv;
+
 	asm goto("1: nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 ".align 3\n\t"
 		 ".quad 1b, %l[l_yes], %c0\n\t"
 		 ".popsection\n\t"
-		 :  :  "i"(key) :  : l_yes);
+		 :  :  "i"(kval) :  : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm goto("1: b %l[l_yes]\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".align 3\n\t"
+		 ".quad 1b, %l[l_yes], %c0\n\t"
+		 ".popsection\n\t"
+		 :  :  "i"(kval) :  : l_yes);
 
 	return false;
 l_yes:
diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
index 608aa57799c8..d9fca6f52a93 100644
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -26,14 +26,33 @@
 #define NOP_INSN "nop"
 #endif
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
+	unsigned long kval = (unsigned long)key + inv;
+
 	asm_volatile_goto("1:\t" NOP_INSN "\n\t"
 		"nop\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"
-		: :  "i" (key) : : l_yes);
+		: :  "i" (kval) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("1:\tj %l[l_yes]\n\t"
+		"nop\n\t"
+		".pushsection __jump_table,  \"aw\"\n\t"
+		WORD_INSN " 1b, %l[l_yes], %0\n\t"
+		".popsection\n\t"
+		: :  "i" (kval) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
index efbf9a322a23..c7cffedb1801 100644
--- a/arch/powerpc/include/asm/jump_label.h
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -18,14 +18,33 @@
 #define JUMP_ENTRY_TYPE		stringify_in_c(FTR_ENTRY_LONG)
 #define JUMP_LABEL_NOP_SIZE	4
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
+	unsigned long kval = (unsigned long)key + inv;
+
 	asm_volatile_goto("1:\n\t"
 		 "nop\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
 		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
 		 ".popsection \n\t"
-		 : :  "i" (key) : : l_yes);
+		 : :  "i" (kval) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("1:\n\t"
+		 "b %l[l_yes]\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (kval) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
diff --git a/arch/s390/include/asm/jump_label.h b/arch/s390/include/asm/jump_label.h
index 69972b7957ee..4734b09b9fce 100644
--- a/arch/s390/include/asm/jump_label.h
+++ b/arch/s390/include/asm/jump_label.h
@@ -12,14 +12,33 @@
  * We use a brcl 0,2 instruction for jump labels at compile time so it
  * can be easily distinguished from a hotpatch generated instruction.
  */
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
+	unsigned long kval = (unsigned long)key + inv;
+
 	asm_volatile_goto("0:	brcl 0,"__stringify(JUMP_LABEL_NOP_OFFSET)"\n"
 		".pushsection __jump_table, \"aw\"\n"
 		".balign 8\n"
 		".quad 0b, %l[label], %0\n"
 		".popsection\n"
-		: : "X" (key) : : label);
+		: : "X" (kval) : : label);
+
+	return false;
+label:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("0:	j %l[l_yes]\n"
+		".pushsection __jump_table, \"aw\"\n"
+		".balign 8\n"
+		".quad 0b, %l[label], %0\n"
+		".popsection\n"
+		: : "X" (kval) : : label);
+
 	return false;
 label:
 	return true;
diff --git a/arch/sparc/include/asm/jump_label.h b/arch/sparc/include/asm/jump_label.h
index cc9b04a2b11b..764f3c21da21 100644
--- a/arch/sparc/include/asm/jump_label.h
+++ b/arch/sparc/include/asm/jump_label.h
@@ -7,16 +7,36 @@
 
 #define JUMP_LABEL_NOP_SIZE 4
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
-		asm_volatile_goto("1:\n\t"
-			 "nop\n\t"
-			 "nop\n\t"
-			 ".pushsection __jump_table,  \"aw\"\n\t"
-			 ".align 4\n\t"
-			 ".word 1b, %l[l_yes], %c0\n\t"
-			 ".popsection \n\t"
-			 : :  "i" (key) : : l_yes);
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("1:\n\t"
+		 "nop\n\t"
+		 "nop\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".align 4\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (kval) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("1:\n\t"
+		 "b %l[l_yes]\n\t"
+		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".align 4\n\t"
+		 ".word 1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (kval) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index a4c1cf7e93f8..43531baaee38 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -16,15 +16,35 @@
 # define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC
 #endif
 
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
 {
+	unsigned long kval = (unsigned long)key + inv;
+
 	asm_volatile_goto("1:"
 		".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
 		".pushsection __jump_table,  \"aw\" \n\t"
 		_ASM_ALIGN "\n\t"
 		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
 		".popsection \n\t"
-		: :  "i" (key) : : l_yes);
+		: :  "i" (kval) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
+{
+	unsigned long kval = (unsigned long)key + inv;
+
+	asm_volatile_goto("1:"
+		"jmp %l[l_yes]\n\t"
+		".pushsection __jump_table,  \"aw\" \n\t"
+		_ASM_ALIGN "\n\t"
+		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
+		".popsection \n\t"
+		: :  "i" (kval) : : l_yes);
+
 	return false;
 l_yes:
 	return true;
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index f4de473f226b..4c97fed7acea 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -122,7 +122,7 @@ static inline bool jump_label_get_branch_default(struct static_key *key)
 
 static __always_inline bool static_key_false(struct static_key *key)
 {
-	return arch_static_branch(key);
+	return arch_static_branch(key, false);
 }
 
 static __always_inline bool static_key_true(struct static_key *key)
@@ -213,6 +213,105 @@ static inline bool static_key_enabled(struct static_key *key)
 	return static_key_count(key) > 0;
 }
 
+static inline 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);
+}
+
+static inline 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);
+}
+
+
+/* -------------------------------------------------------------------------- */
+
+/*
+ * Two type wrappers around static_key, such that we can use compile time
+ * type differentiation to emit the right code.
+ *
+ * All the below code is macros in order to play type games.
+ */
+
+struct static_key_true {
+	struct static_key key;
+};
+
+struct static_key_false {
+	struct static_key key;
+};
+
+#define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
+#define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }
+
+#define DEFINE_STATIC_KEY_TRUE(name)	\
+	struct static_key_true name = STATIC_KEY_TRUE_INIT
+
+#define DEFINE_STATIC_KEY_FALSE(name)	\
+	struct static_key_false name = STATIC_KEY_FALSE_INIT
+
+#ifdef HAVE_JUMP_LABEL
+
+/*
+ * Combine the right initial value (type) with the right branch order and section
+ * to generate the desired result.
+ */
+
+#define static_branch_likely(x)                                                 \
+({                                                                              \
+        bool branch;                                                            \
+        if (__builtin_types_compatible_p(typeof(x), struct static_key_true))    \
+                branch = !arch_static_branch(&(x)->key, false);                 \
+        else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
+                branch = !arch_static_branch_jump(&(x)->key, true);             \
+        else                                                                    \
+                branch = ____wrong_branch_error();                              \
+        branch;                                                                 \
+})
+
+#define static_branch_unlikely(x)                                               \
+({                                                                              \
+        bool branch;                                                            \
+        if (__builtin_types_compatible_p(typeof(x), struct static_key_true))    \
+                branch = arch_static_branch(&(x)->key, true);                   \
+        else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
+                branch = arch_static_branch_jump(&(x)->key, false);             \
+        else                                                                    \
+                branch = ____wrong_branch_error();                              \
+        branch;                                                                 \
+})
+
+#else /* !HAVE_JUMP_LABEL */
+
+#define static_branch_likely(x)		static_key_true(&(x)->key)
+#define static_branch_unlikely(x)	static_key_false(&(x)->key)
+
+#endif /* HAVE_JUMP_LABEL */
+
+/*
+ * Advanced usage; refcount, branch is enabled when: count != 0
+ */
+
+#define static_branch_inc(x)  static_key_slow_inc(&(x)->key)
+#define static_branch_dec(x)  static_key_slow_dec(&(x)->key)
+
+/*
+ * Normal usage; boolean enable/disable.
+ */
+
+#define static_branch_enable(x)       static_key_enable(&(x)->key)
+#define static_branch_disable(x)      static_key_disable(&(x)->key)
+
 #endif	/* _LINUX_JUMP_LABEL_H */
 
 #endif /* __ASSEMBLY__ */
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 52ebaca1b9fc..8d0cb621c5bc 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -54,7 +54,7 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
 	sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
 }
 
-static void jump_label_update(struct static_key *key, int enable);
+static void jump_label_update(struct static_key *key);
 
 void static_key_slow_inc(struct static_key *key)
 {
@@ -63,13 +63,8 @@ void static_key_slow_inc(struct static_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_read(&key->enabled) == 0) {
-		if (!jump_label_get_branch_default(key))
-			jump_label_update(key, JUMP_LABEL_ENABLE);
-		else
-			jump_label_update(key, JUMP_LABEL_DISABLE);
-	}
-	atomic_inc(&key->enabled);
+	if (atomic_inc_and_test(&key->enabled))
+		jump_label_update(key);
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
@@ -87,10 +82,7 @@ static void __static_key_slow_dec(struct static_key *key,
 		atomic_inc(&key->enabled);
 		schedule_delayed_work(work, rate_limit);
 	} else {
-		if (!jump_label_get_branch_default(key))
-			jump_label_update(key, JUMP_LABEL_DISABLE);
-		else
-			jump_label_update(key, JUMP_LABEL_ENABLE);
+		jump_label_update(key);
 	}
 	jump_label_unlock();
 }
@@ -149,7 +141,7 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
 	return 0;
 }
 
-/* 
+/*
  * Update code which is definitely not currently executing.
  * Architectures which need heavyweight synchronization to modify
  * running code can override this to make the non-live update case
@@ -158,37 +150,44 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
 void __weak __init_or_module arch_jump_label_transform_static(struct jump_entry *entry,
 					    enum jump_label_type type)
 {
-	arch_jump_label_transform(entry, type);	
+	arch_jump_label_transform(entry, type);
+}
+
+static struct static_key *static_key_cast(jump_label_t key_addr)
+{
+	return (struct static_key *)(key_addr & ~1UL);
+}
+
+static bool static_key_inv(jump_label_t key_addr)
+{
+	return key_addr & 1UL;
+}
+
+static enum jump_label_type jump_label_type(struct jump_entry *entry)
+{
+	struct static_key *key = static_key_cast(iter->key);
+	bool true_branch = jump_label_get_branch_default(key);
+	bool state = static_key_enabled(key);
+	bool inv = static_key_inv(iter->key);
+
+	return (true_branch ^ state) ^ inv;
 }
 
 static void __jump_label_update(struct static_key *key,
 				struct jump_entry *entry,
-				struct jump_entry *stop, int enable)
+				struct jump_entry *stop)
 {
-	for (; (entry < stop) &&
-	      (entry->key == (jump_label_t)(unsigned long)key);
-	      entry++) {
+	for (; (entry < stop) && (static_key_cast(entry->key) == key); entry++) {
 		/*
 		 * entry->code set to 0 invalidates module init text sections
 		 * kernel_text_address() verifies we are not in core kernel
 		 * init code, see jump_label_invalidate_module_init().
 		 */
 		if (entry->code && kernel_text_address(entry->code))
-			arch_jump_label_transform(entry, enable);
+			arch_jump_label_transform(entry, jump_label_type(entry));
 	}
 }
 
-static enum jump_label_type jump_label_type(struct static_key *key)
-{
-	bool true_branch = jump_label_get_branch_default(key);
-	bool state = static_key_enabled(key);
-
-	if ((!true_branch && state) || (true_branch && !state))
-		return JUMP_LABEL_ENABLE;
-
-	return JUMP_LABEL_DISABLE;
-}
-
 void __init jump_label_init(void)
 {
 	struct jump_entry *iter_start = __start___jump_table;
@@ -202,8 +201,8 @@ void __init jump_label_init(void)
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct static_key *iterk;
 
-		iterk = (struct static_key *)(unsigned long)iter->key;
-		arch_jump_label_transform_static(iter, jump_label_type(iterk));
+		arch_jump_label_transform_static(iter, jump_label_type(iter));
+		iterk = static_key_cast(iter->key);
 		if (iterk == key)
 			continue;
 
@@ -243,17 +242,15 @@ static int __jump_label_mod_text_reserved(void *start, void *end)
 				start, end);
 }
 
-static void __jump_label_mod_update(struct static_key *key, int enable)
+static void __jump_label_mod_update(struct static_key *key)
 {
-	struct static_key_mod *mod = key->next;
+	struct static_key_mod *mod;
 
-	while (mod) {
+	for (mod = key->next; mod; mod = mod->next) {
 		struct module *m = mod->mod;
 
 		__jump_label_update(key, mod->entries,
-				    m->jump_entries + m->num_jump_entries,
-				    enable);
-		mod = mod->next;
+				    m->jump_entries + m->num_jump_entries)
 	}
 }
 
@@ -276,7 +273,8 @@ void jump_label_apply_nops(struct module *mod)
 		return;
 
 	for (iter = iter_start; iter < iter_stop; iter++) {
-		arch_jump_label_transform_static(iter, JUMP_LABEL_DISABLE);
+		if (jump_label_type(iter) == JUMP_LABEL_DISABLE)
+			arch_jump_label_transform_static(iter, JUMP_LABEL_DISABLE);
 	}
 }
 
@@ -297,7 +295,7 @@ static int jump_label_add_module(struct module *mod)
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct static_key *iterk;
 
-		iterk = (struct static_key *)(unsigned long)iter->key;
+		iterk = static_key_cast(iter->key);
 		if (iterk == key)
 			continue;
 
@@ -318,8 +316,7 @@ static int jump_label_add_module(struct module *mod)
 		jlm->next = key->next;
 		key->next = jlm;
 
-		if (jump_label_type(key) == JUMP_LABEL_ENABLE)
-			__jump_label_update(key, iter, iter_stop, JUMP_LABEL_ENABLE);
+		__jump_label_update(key, iter, iter_stop);
 	}
 
 	return 0;
@@ -334,10 +331,10 @@ static void jump_label_del_module(struct module *mod)
 	struct static_key_mod *jlm, **prev;
 
 	for (iter = iter_start; iter < iter_stop; iter++) {
-		if (iter->key == (jump_label_t)(unsigned long)key)
+		if (static_key_cast(iter->key) == key)
 			continue;
 
-		key = (struct static_key *)(unsigned long)iter->key;
+		key = static_key_cast(iter->key);
 
 		if (within_module(iter->key, mod))
 			continue;
@@ -439,7 +436,7 @@ int jump_label_text_reserved(void *start, void *end)
 	return ret;
 }
 
-static void jump_label_update(struct static_key *key, int enable)
+static void jump_label_update(struct static_key *key)
 {
 	struct jump_entry *stop = __stop___jump_table;
 	struct jump_entry *entry = jump_label_get_entries(key);
@@ -456,7 +453,7 @@ static void jump_label_update(struct static_key *key, int enable)
 #endif
 	/* if there are no users, entry can be NULL */
 	if (entry)
-		__jump_label_update(key, entry, stop, enable);
+		__jump_label_update(key, entry, stop);
 }
 
 #endif

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/23/2015 10:49 AM, Peter Zijlstra wrote:
> On Thu, Jul 23, 2015 at 04:33:08PM +0200, Peter Zijlstra wrote:
>> Lemme finish this and I'll post it.
> Compile tested on x86_64 only..
>
> Please have a look, I think you said I got some of the logic wrong, I've
> not double checked that.
>
> I'll go write comments and double check things.
>
> ---
>  arch/arm/include/asm/jump_label.h     |  22 +++++++-
>  arch/arm64/include/asm/jump_label.h   |  22 +++++++-
>  arch/mips/include/asm/jump_label.h    |  23 +++++++-
>  arch/powerpc/include/asm/jump_label.h |  23 +++++++-
>  arch/s390/include/asm/jump_label.h    |  23 +++++++-
>  arch/sparc/include/asm/jump_label.h   |  38 ++++++++++---
>  arch/x86/include/asm/jump_label.h     |  24 +++++++-
>  include/linux/jump_label.h            | 101 +++++++++++++++++++++++++++++++++-
>  kernel/jump_label.c                   |  89 +++++++++++++++---------------
>  9 files changed, 297 insertions(+), 68 deletions(-)
>
> diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h
> index 5f337dc5c108..6c9789b33497 100644
> --- a/arch/arm/include/asm/jump_label.h
> +++ b/arch/arm/include/asm/jump_label.h
> @@ -13,14 +13,32 @@
>  #define JUMP_LABEL_NOP	"nop"
>  #endif
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> +	unsigned long kval = (unsigned long)key + inv;
> +
>  	asm_volatile_goto("1:\n\t"
>  		 JUMP_LABEL_NOP "\n\t"
>  		 ".pushsection __jump_table,  \"aw\"\n\t"
>  		 ".word 1b, %l[l_yes], %c0\n\t"
>  		 ".popsection\n\t"
> -		 : :  "i" (key) :  : l_yes);
> +		 : :  "i" (kval) :  : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("1:\n\t"
> +		 "b %l[l_yes]\n\t"
> +		 ".pushsection __jump_table,  \"aw\"\n\t"
> +		 ".word 1b, %l[l_yes], %c0\n\t"
> +		 ".popsection\n\t"
> +		 : :  "i" (kval) :  : l_yes);
>  
>  	return false;
>  l_yes:
> diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> index c0e5165c2f76..e5cda5d75c62 100644
> --- a/arch/arm64/include/asm/jump_label.h
> +++ b/arch/arm64/include/asm/jump_label.h
> @@ -26,14 +26,32 @@
>  
>  #define JUMP_LABEL_NOP_SIZE		AARCH64_INSN_SIZE
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> +	unsigned long kval = (unsigned long)key + inv;
> +
>  	asm goto("1: nop\n\t"
>  		 ".pushsection __jump_table,  \"aw\"\n\t"
>  		 ".align 3\n\t"
>  		 ".quad 1b, %l[l_yes], %c0\n\t"
>  		 ".popsection\n\t"
> -		 :  :  "i"(key) :  : l_yes);
> +		 :  :  "i"(kval) :  : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm goto("1: b %l[l_yes]\n\t"
> +		 ".pushsection __jump_table,  \"aw\"\n\t"
> +		 ".align 3\n\t"
> +		 ".quad 1b, %l[l_yes], %c0\n\t"
> +		 ".popsection\n\t"
> +		 :  :  "i"(kval) :  : l_yes);
>  
>  	return false;
>  l_yes:
> diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
> index 608aa57799c8..d9fca6f52a93 100644
> --- a/arch/mips/include/asm/jump_label.h
> +++ b/arch/mips/include/asm/jump_label.h
> @@ -26,14 +26,33 @@
>  #define NOP_INSN "nop"
>  #endif
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> +	unsigned long kval = (unsigned long)key + inv;
> +
>  	asm_volatile_goto("1:\t" NOP_INSN "\n\t"
>  		"nop\n\t"
>  		".pushsection __jump_table,  \"aw\"\n\t"
>  		WORD_INSN " 1b, %l[l_yes], %0\n\t"
>  		".popsection\n\t"
> -		: :  "i" (key) : : l_yes);
> +		: :  "i" (kval) : : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("1:\tj %l[l_yes]\n\t"
> +		"nop\n\t"
> +		".pushsection __jump_table,  \"aw\"\n\t"
> +		WORD_INSN " 1b, %l[l_yes], %0\n\t"
> +		".popsection\n\t"
> +		: :  "i" (kval) : : l_yes);
> +
>  	return false;
>  l_yes:
>  	return true;
> diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
> index efbf9a322a23..c7cffedb1801 100644
> --- a/arch/powerpc/include/asm/jump_label.h
> +++ b/arch/powerpc/include/asm/jump_label.h
> @@ -18,14 +18,33 @@
>  #define JUMP_ENTRY_TYPE		stringify_in_c(FTR_ENTRY_LONG)
>  #define JUMP_LABEL_NOP_SIZE	4
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> +	unsigned long kval = (unsigned long)key + inv;
> +
>  	asm_volatile_goto("1:\n\t"
>  		 "nop\n\t"
>  		 ".pushsection __jump_table,  \"aw\"\n\t"
>  		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
>  		 ".popsection \n\t"
> -		 : :  "i" (key) : : l_yes);
> +		 : :  "i" (kval) : : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("1:\n\t"
> +		 "b %l[l_yes]\n\t"
> +		 ".pushsection __jump_table,  \"aw\"\n\t"
> +		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
> +		 ".popsection \n\t"
> +		 : :  "i" (kval) : : l_yes);
> +
>  	return false;
>  l_yes:
>  	return true;
> diff --git a/arch/s390/include/asm/jump_label.h b/arch/s390/include/asm/jump_label.h
> index 69972b7957ee..4734b09b9fce 100644
> --- a/arch/s390/include/asm/jump_label.h
> +++ b/arch/s390/include/asm/jump_label.h
> @@ -12,14 +12,33 @@
>   * We use a brcl 0,2 instruction for jump labels at compile time so it
>   * can be easily distinguished from a hotpatch generated instruction.
>   */
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> +	unsigned long kval = (unsigned long)key + inv;
> +
>  	asm_volatile_goto("0:	brcl 0,"__stringify(JUMP_LABEL_NOP_OFFSET)"\n"
>  		".pushsection __jump_table, \"aw\"\n"
>  		".balign 8\n"
>  		".quad 0b, %l[label], %0\n"
>  		".popsection\n"
> -		: : "X" (key) : : label);
> +		: : "X" (kval) : : label);
> +
> +	return false;
> +label:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("0:	j %l[l_yes]\n"
> +		".pushsection __jump_table, \"aw\"\n"
> +		".balign 8\n"
> +		".quad 0b, %l[label], %0\n"
> +		".popsection\n"
> +		: : "X" (kval) : : label);
> +
>  	return false;
>  label:
>  	return true;
> diff --git a/arch/sparc/include/asm/jump_label.h b/arch/sparc/include/asm/jump_label.h
> index cc9b04a2b11b..764f3c21da21 100644
> --- a/arch/sparc/include/asm/jump_label.h
> +++ b/arch/sparc/include/asm/jump_label.h
> @@ -7,16 +7,36 @@
>  
>  #define JUMP_LABEL_NOP_SIZE 4
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> -		asm_volatile_goto("1:\n\t"
> -			 "nop\n\t"
> -			 "nop\n\t"
> -			 ".pushsection __jump_table,  \"aw\"\n\t"
> -			 ".align 4\n\t"
> -			 ".word 1b, %l[l_yes], %c0\n\t"
> -			 ".popsection \n\t"
> -			 : :  "i" (key) : : l_yes);
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("1:\n\t"
> +		 "nop\n\t"
> +		 "nop\n\t"
> +		 ".pushsection __jump_table,  \"aw\"\n\t"
> +		 ".align 4\n\t"
> +		 ".word 1b, %l[l_yes], %c0\n\t"
> +		 ".popsection \n\t"
> +		 : :  "i" (kval) : : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("1:\n\t"
> +		 "b %l[l_yes]\n\t"
> +		 ".pushsection __jump_table,  \"aw\"\n\t"
> +		 ".align 4\n\t"
> +		 ".word 1b, %l[l_yes], %c0\n\t"
> +		 ".popsection \n\t"
> +		 : :  "i" (kval) : : l_yes);
> +
>  	return false;
>  l_yes:
>  	return true;
> diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
> index a4c1cf7e93f8..43531baaee38 100644
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -16,15 +16,35 @@
>  # define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC
>  #endif
>  
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch(struct static_key *key, bool inv)
>  {
> +	unsigned long kval = (unsigned long)key + inv;
> +
>  	asm_volatile_goto("1:"
>  		".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
>  		".pushsection __jump_table,  \"aw\" \n\t"
>  		_ASM_ALIGN "\n\t"
>  		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
>  		".popsection \n\t"
> -		: :  "i" (key) : : l_yes);
> +		: :  "i" (kval) : : l_yes);
> +
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> +{
> +	unsigned long kval = (unsigned long)key + inv;
> +
> +	asm_volatile_goto("1:"
> +		"jmp %l[l_yes]\n\t"
> +		".pushsection __jump_table,  \"aw\" \n\t"
> +		_ASM_ALIGN "\n\t"
> +		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> +		".popsection \n\t"
> +		: :  "i" (kval) : : l_yes);
> +
>  	return false;
>  l_yes:
>  	return true;
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index f4de473f226b..4c97fed7acea 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -122,7 +122,7 @@ static inline bool jump_label_get_branch_default(struct static_key *key)
>  
>  static __always_inline bool static_key_false(struct static_key *key)
>  {
> -	return arch_static_branch(key);
> +	return arch_static_branch(key, false);
>  }
>  
>  static __always_inline bool static_key_true(struct static_key *key)
> @@ -213,6 +213,105 @@ static inline bool static_key_enabled(struct static_key *key)
>  	return static_key_count(key) > 0;
>  }
>  
> +static inline 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);
> +}
> +
> +static inline 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);
> +}
> +
> +
> +/* -------------------------------------------------------------------------- */
> +
> +/*
> + * Two type wrappers around static_key, such that we can use compile time
> + * type differentiation to emit the right code.
> + *
> + * All the below code is macros in order to play type games.
> + */
> +
> +struct static_key_true {
> +	struct static_key key;
> +};
> +
> +struct static_key_false {
> +	struct static_key key;
> +};
> +
> +#define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
> +#define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }
> +
> +#define DEFINE_STATIC_KEY_TRUE(name)	\
> +	struct static_key_true name = STATIC_KEY_TRUE_INIT
> +
> +#define DEFINE_STATIC_KEY_FALSE(name)	\
> +	struct static_key_false name = STATIC_KEY_FALSE_INIT
> +
> +#ifdef HAVE_JUMP_LABEL
> +
> +/*
> + * Combine the right initial value (type) with the right branch order and section
> + * to generate the desired result.
> + */
> +
> +#define static_branch_likely(x)                                                 \
> +({                                                                              \
> +        bool branch;                                                            \
> +        if (__builtin_types_compatible_p(typeof(x), struct static_key_true))    \
> +                branch = !arch_static_branch(&(x)->key, false);                 \
> +        else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> +                branch = !arch_static_branch_jump(&(x)->key, true);             \
> +        else                                                                    \
> +                branch = ____wrong_branch_error();                              \
> +        branch;                                                                 \
> +})
> +
> +#define static_branch_unlikely(x)                                               \
> +({                                                                              \
> +        bool branch;                                                            \
> +        if (__builtin_types_compatible_p(typeof(x), struct static_key_true))    \
> +                branch = arch_static_branch(&(x)->key, true);                   \
> +        else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> +                branch = arch_static_branch_jump(&(x)->key, false);             \
> +        else                                                                    \
> +                branch = ____wrong_branch_error();                              \
> +        branch;                                                                 \
> +})
> +
> +#else /* !HAVE_JUMP_LABEL */
> +
> +#define static_branch_likely(x)		static_key_true(&(x)->key)
> +#define static_branch_unlikely(x)	static_key_false(&(x)->key)
> +
> +#endif /* HAVE_JUMP_LABEL */
> +
> +/*
> + * Advanced usage; refcount, branch is enabled when: count != 0
> + */
> +
> +#define static_branch_inc(x)  static_key_slow_inc(&(x)->key)
> +#define static_branch_dec(x)  static_key_slow_dec(&(x)->key)
> +
> +/*
> + * Normal usage; boolean enable/disable.
> + */
> +
> +#define static_branch_enable(x)       static_key_enable(&(x)->key)
> +#define static_branch_disable(x)      static_key_disable(&(x)->key)
> +
>  #endif	/* _LINUX_JUMP_LABEL_H */
>  
>  #endif /* __ASSEMBLY__ */
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 52ebaca1b9fc..8d0cb621c5bc 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -54,7 +54,7 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>  	sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
>  }
>  
> -static void jump_label_update(struct static_key *key, int enable);
> +static void jump_label_update(struct static_key *key);
>  
>  void static_key_slow_inc(struct static_key *key)
>  {
> @@ -63,13 +63,8 @@ void static_key_slow_inc(struct static_key *key)
>  		return;
>  
>  	jump_label_lock();
> -	if (atomic_read(&key->enabled) == 0) {
> -		if (!jump_label_get_branch_default(key))
> -			jump_label_update(key, JUMP_LABEL_ENABLE);
> -		else
> -			jump_label_update(key, JUMP_LABEL_DISABLE);
> -	}
> -	atomic_inc(&key->enabled);
> +	if (atomic_inc_and_test(&key->enabled))
> +		jump_label_update(key);
>  	jump_label_unlock();
>  }
>  EXPORT_SYMBOL_GPL(static_key_slow_inc);
> @@ -87,10 +82,7 @@ static void __static_key_slow_dec(struct static_key *key,
>  		atomic_inc(&key->enabled);
>  		schedule_delayed_work(work, rate_limit);
>  	} else {
> -		if (!jump_label_get_branch_default(key))
> -			jump_label_update(key, JUMP_LABEL_DISABLE);
> -		else
> -			jump_label_update(key, JUMP_LABEL_ENABLE);
> +		jump_label_update(key);
>  	}
>  	jump_label_unlock();
>  }
> @@ -149,7 +141,7 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
>  	return 0;
>  }
>  
> -/* 
> +/*
>   * Update code which is definitely not currently executing.
>   * Architectures which need heavyweight synchronization to modify
>   * running code can override this to make the non-live update case
> @@ -158,37 +150,44 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start,
>  void __weak __init_or_module arch_jump_label_transform_static(struct jump_entry *entry,
>  					    enum jump_label_type type)
>  {
> -	arch_jump_label_transform(entry, type);	
> +	arch_jump_label_transform(entry, type);
> +}
> +
> +static struct static_key *static_key_cast(jump_label_t key_addr)
> +{
> +	return (struct static_key *)(key_addr & ~1UL);
> +}
> +
> +static bool static_key_inv(jump_label_t key_addr)
> +{
> +	return key_addr & 1UL;
> +}
> +
> +static enum jump_label_type jump_label_type(struct jump_entry *entry)
> +{
> +	struct static_key *key = static_key_cast(iter->key);
> +	bool true_branch = jump_label_get_branch_default(key);
> +	bool state = static_key_enabled(key);
> +	bool inv = static_key_inv(iter->key);
> +
> +	return (true_branch ^ state) ^ inv;

iiuc...this allows both the old style keys to co-exist with the
new ones. IE state wouldn't be set for the new ones. And inv
shouldn't be set for the old ones. Is that correct?

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 03:14:13PM -0400, Jason Baron wrote:
> > +static enum jump_label_type jump_label_type(struct jump_entry *entry)
> > +{
> > +	struct static_key *key = static_key_cast(iter->key);
> > +	bool true_branch = jump_label_get_branch_default(key);
> > +	bool state = static_key_enabled(key);
> > +	bool inv = static_key_inv(iter->key);
> > +
> > +	return (true_branch ^ state) ^ inv;
> 
> iiuc...this allows both the old style keys to co-exist with the
> new ones. IE state wouldn't be set for the new ones. And inv
> shouldn't be set for the old ones. Is that correct?

@state is the dynamic variable here, the other two are compile time.
@true_branch denotes the default (compile time) value, and @inv denotes
the (compile time) branch preference.

So @state will still be set for the new ones, but you're right in that
@inv will not be set for the 'legacy' stuff.

This one little line needs a big comment.

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Fri, Jul 24, 2015 at 12:56:02PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 23, 2015 at 03:14:13PM -0400, Jason Baron wrote:
> > > +static enum jump_label_type jump_label_type(struct jump_entry *entry)
> > > +{
> > > +	struct static_key *key = static_key_cast(iter->key);
> > > +	bool true_branch = jump_label_get_branch_default(key);
> > > +	bool state = static_key_enabled(key);
> > > +	bool inv = static_key_inv(iter->key);
> > > +
> > > +	return (true_branch ^ state) ^ inv;
> > 
> > iiuc...this allows both the old style keys to co-exist with the
> > new ones. IE state wouldn't be set for the new ones. And inv
> > shouldn't be set for the old ones. Is that correct?
> 
> @state is the dynamic variable here, the other two are compile time.
> @true_branch denotes the default (compile time) value, and @inv denotes
> the (compile time) branch preference.

Ha!, so that wasn't entirely correct, it turned out @inv means
arch_static_branch_jump().

That would let us remove the whole argument to the arch functions.

That said, I generated the logic table for @inv meaning the branch type
and then found a logic similar to what you outlined:


/*
 * Combine the right initial value (type) with the right branch order
 * to generate the desired result.
 *
 *
 *  type	likely (1)		unlikely (0)
 * -----------+-----------------------+------------------
 *            |                       |
 *  true (1)  |	   ...		      |	   ...
 *            |    NOP		      |	   JMP L
 *            |    <br-stmts>	      |	1: ...
 *            |	L: ...		      |
 *            |			      |
 *            |			      |	L: <br-stmts>
 *            |			      |	   jmp 1b
 *            |                       |
 * -----------+-----------------------+------------------
 *            |                       |
 *  false (0) |	   ...		      |	   ...
 *            |    JMP L	      |	   NOP
 *            |    <br-stmts>	      |	1: ...
 *            |	L: ...		      |
 *            |			      |
 *            |			      |	L: <br-stmts>
 *            |			      |	   jmp 1b
 *            |                       |
 * -----------+-----------------------+------------------
 *
 * The initial value is encoded in the LSB of static_key::entries,
 * type: 0 = false, 1 = true.
 *
 * The branch type is encoded in the LSB of jump_entry::key,
 * branch: 0 = unlikely, 1 = likely.
 *
 * This gives the following logic table:
 *
 *	enabled	type	branch	  instuction
 * -----------------------------+-----------
 *	0	0	0	| NOP
 *	0	0	1	| JMP
 *	0	1	0	| NOP
 *	0	1	1	| JMP
 *
 *	1	0	0	| JMP
 *	1	0	1	| NOP
 *	1	1	0	| JMP
 *	1	1	1	| NOP
 *
 */

This gives a map: ins = enabled ^ branch, which shows @type to be
redundant.

And we can trivially switch over the old static_key_{true,false}() to
emit the right branch type.

Whcih would mean we could remove the type encoding entirely, but I'll
leave that be for now, maybe it'll come in handy later or whatnot.

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/24/2015 08:36 AM, Peter Zijlstra wrote:
> On Fri, Jul 24, 2015 at 12:56:02PM +0200, Peter Zijlstra wrote:
>> On Thu, Jul 23, 2015 at 03:14:13PM -0400, Jason Baron wrote:
>>>> +static enum jump_label_type jump_label_type(struct jump_entry *entry)
>>>> +{
>>>> +	struct static_key *key = static_key_cast(iter->key);
>>>> +	bool true_branch = jump_label_get_branch_default(key);
>>>> +	bool state = static_key_enabled(key);
>>>> +	bool inv = static_key_inv(iter->key);
>>>> +
>>>> +	return (true_branch ^ state) ^ inv;
>>> iiuc...this allows both the old style keys to co-exist with the
>>> new ones. IE state wouldn't be set for the new ones. And inv
>>> shouldn't be set for the old ones. Is that correct?
>> @state is the dynamic variable here, the other two are compile time.
>> @true_branch denotes the default (compile time) value, and @inv denotes
>> the (compile time) branch preference.
> Ha!, so that wasn't entirely correct, it turned out @inv means
> arch_static_branch_jump().
>
> That would let us remove the whole argument to the arch functions.
>
> That said, I generated the logic table for @inv meaning the branch type
> and then found a logic similar to what you outlined:
>
>
> /*
>  * Combine the right initial value (type) with the right branch order
>  * to generate the desired result.
>  *
>  *
>  *  type	likely (1)		unlikely (0)
>  * -----------+-----------------------+------------------
>  *            |                       |
>  *  true (1)  |	   ...		      |	   ...
>  *            |    NOP		      |	   JMP L
>  *            |    <br-stmts>	      |	1: ...
>  *            |	L: ...		      |
>  *            |			      |
>  *            |			      |	L: <br-stmts>
>  *            |			      |	   jmp 1b
>  *            |                       |
>  * -----------+-----------------------+------------------
>  *            |                       |
>  *  false (0) |	   ...		      |	   ...
>  *            |    JMP L	      |	   NOP
>  *            |    <br-stmts>	      |	1: ...
>  *            |	L: ...		      |
>  *            |			      |
>  *            |			      |	L: <br-stmts>
>  *            |			      |	   jmp 1b
>  *            |                       |
>  * -----------+-----------------------+------------------
>  *
>  * The initial value is encoded in the LSB of static_key::entries,
>  * type: 0 = false, 1 = true.
>  *
>  * The branch type is encoded in the LSB of jump_entry::key,
>  * branch: 0 = unlikely, 1 = likely.
>  *
>  * This gives the following logic table:
>  *
>  *	enabled	type	branch	  instuction
>  * -----------------------------+-----------
>  *	0	0	0	| NOP
>  *	0	0	1	| JMP
>  *	0	1	0	| NOP
>  *	0	1	1	| JMP
>  *
>  *	1	0	0	| JMP
>  *	1	0	1	| NOP
>  *	1	1	0	| JMP
>  *	1	1	1	| NOP
>  *
>  */
>
> This gives a map: ins = enabled ^ branch, which shows @type to be
> redundant.
>
> And we can trivially switch over the old static_key_{true,false}() to
> emit the right branch type.
>
> Whcih would mean we could remove the type encoding entirely, but I'll
> leave that be for now, maybe it'll come in handy later or whatnot.
>

I would just remove 'type'. Essentially, before we were storing
the 'branch' with the key. However, in this new model the
'branch' is really associated with the branch location, since the
same key can be used now with different branches.

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 10:19:52AM -0400, Jason Baron wrote:
> >
> > #define static_branch_likely(x)							\
> > ({										\
> > 	bool branch;								\
> > 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> > 		branch = !arch_static_branch(&(x)->key, false);			\
> > 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> > 		branch = !arch_static_branch_jump(&(x)->key, true);		\
> > 	else									\
> > 		branch = ____wrong_branch_error();				\
> > 	branch;									\
> > })
> >
> > #define static_branch_unlikely(x)						\
> > ({										\
> > 	bool branch;								\
> > 	if (__builtin_types_compatible_p(typeof(x), struct static_key_true))	\
> > 		branch = arch_static_branch(&(x)->key, true);			\
> > 	else if (__builtin_types_compatible_p(typeof(x), struct static_key_false)) \
> > 		branch = arch_static_branch_jump(&(x)->key, false);		\
> > 	else									\
> > 		branch = ____wrong_branch_error();				\
> > 	branch;									\
> > })
> >
> 
> In 'static_branch_unlikely()', I think  arch_static_branch() and
> arch_static_branch_jump() are reversed.

Yes, you're right. But I think I need a nap before touching this stuff
again :-)

Re: Kernel broken on processors without performance counters

[Steven Rostedt]

On Thu, 23 Jul 2015 12:42:15 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> {
> 	if (!inv) {
> 		asm_volatile_goto("1:"
> 			"jmp %l[l_yes]\n\t"

And what happens when this gets converted to a two byte jump?

-- Steve

> 			".pushsection __jump_table,  \"aw\" \n\t"
> 			_ASM_ALIGN "\n\t"
> 			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 			".popsection \n\t"
> 			: :  "i" (key) : : l_yes);
> 	} else {
> 		asm_volatile_goto("1:"
> 			"jmp %l[l_yes]\n\t"
> 			".pushsection __jump_table_inv,  \"aw\" \n\t"
> 			_ASM_ALIGN "\n\t"
> 			_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 			".popsection \n\t"
> 			: :  "i" (key) : : l_yes);
> 	}
> 	return false;
> l_yes:
> 	return true;
> }
> 
> 

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 11:34:50AM -0400, Steven Rostedt wrote:
> On Thu, 23 Jul 2015 12:42:15 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> > {
> > 	if (!inv) {
> > 		asm_volatile_goto("1:"
> > 			"jmp %l[l_yes]\n\t"
> 
> And what happens when this gets converted to a two byte jump?
> 

That would be bad, how can we force it to emit 5 bytes?

Re: Kernel broken on processors without performance counters

[Steven Rostedt]

On Thu, 23 Jul 2015 19:08:11 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jul 23, 2015 at 11:34:50AM -0400, Steven Rostedt wrote:
> > On Thu, 23 Jul 2015 12:42:15 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> > > {
> > > 	if (!inv) {
> > > 		asm_volatile_goto("1:"
> > > 			"jmp %l[l_yes]\n\t"
> > 
> > And what happens when this gets converted to a two byte jump?
> > 
> 
> That would be bad, how can we force it to emit 5 bytes?

No idea, but I could pull out that old code that converted them :-)

The complexity was in the elf parser that was run at kernel compile
time. It was based on the same code that does the work with
record-mcount.c to find all the mcount callers and made the sections
for them. In fact, it wasn't much different, as record-mcount.c will
convert the black listed sections into nops, so they do not bother
calling mcount at all. But those sections were not recorded, as they
were blacklisted anyway (not whitelisted really, as to be a blacklisted
section, it just had to not be in the whitelisted list).

If we got the jmp conversion in, I was going to clean up the code such
that both record-mcount.c and the jmp conversions used the same code
where applicable.

I would probably still convert every jmp to a nop (2 or 5 byte), and
then at boot up convert those back to jmps that are needed.

-- Steve

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/23/2015 01:08 PM, Peter Zijlstra wrote:
> On Thu, Jul 23, 2015 at 11:34:50AM -0400, Steven Rostedt wrote:
>> On Thu, 23 Jul 2015 12:42:15 +0200
>> Peter Zijlstra <peterz@infradead.org> wrote:
>>
>>> static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
>>> {
>>> 	if (!inv) {
>>> 		asm_volatile_goto("1:"
>>> 			"jmp %l[l_yes]\n\t"
>> And what happens when this gets converted to a two byte jump?
>>
> That would be bad, how can we force it to emit 5 bytes?
hmm....I don't think that's an issue, the patching code can
detect if its a 2-byte jump - 0xeb, or 5-byte: 0xe9, and do
the correct no-op. Same going the other way. See the code
I posted a few mails back. In fact, this gets us to the
smaller 2-byte no-ops in cases where we are initialized
to jump.

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Steven Rostedt]

On Thu, 23 Jul 2015 13:33:36 -0400
Jason Baron <jasonbaron0@gmail.com> wrote:

> On 07/23/2015 01:08 PM, Peter Zijlstra wrote:
> > On Thu, Jul 23, 2015 at 11:34:50AM -0400, Steven Rostedt wrote:
> >> On Thu, 23 Jul 2015 12:42:15 +0200
> >> Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >>> static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> >>> {
> >>> 	if (!inv) {
> >>> 		asm_volatile_goto("1:"
> >>> 			"jmp %l[l_yes]\n\t"
> >> And what happens when this gets converted to a two byte jump?
> >>
> > That would be bad, how can we force it to emit 5 bytes?
> hmm....I don't think that's an issue, the patching code can
> detect if its a 2-byte jump - 0xeb, or 5-byte: 0xe9, and do
> the correct no-op. Same going the other way. See the code
> I posted a few mails back. In fact, this gets us to the
> smaller 2-byte no-ops in cases where we are initialized
> to jump.
>

Ah right, and I already have the code that checks that (from the
original plan).

-- Steve

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 01:33:36PM -0400, Jason Baron wrote:
> > That would be bad, how can we force it to emit 5 bytes?

> hmm....I don't think that's an issue, the patching code can
> detect if its a 2-byte jump - 0xeb, or 5-byte: 0xe9, and do
> the correct no-op. Same going the other way. See the code
> I posted a few mails back. In fact, this gets us to the
> smaller 2-byte no-ops in cases where we are initialized
> to jump.

Ah yes, I looked right over that.

I think that if we want to go do that, it should be a separate patch
though.

Re: Kernel broken on processors without performance counters

[Andy Lutomirski]

On Jul 23, 2015 10:08 AM, "Peter Zijlstra" <peterz@infradead.org> wrote:
>
> On Thu, Jul 23, 2015 at 11:34:50AM -0400, Steven Rostedt wrote:
> > On Thu, 23 Jul 2015 12:42:15 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> > > {
> > >     if (!inv) {
> > >             asm_volatile_goto("1:"
> > >                     "jmp %l[l_yes]\n\t"
> >
> > And what happens when this gets converted to a two byte jump?
> >
>
> That would be bad, how can we force it to emit 5 bytes?

jmp.d32 on newer toolchains IIRC.

--Andy

Re: Kernel broken on processors without performance counters

[Borislav Petkov]

On Thu, Jul 23, 2015 at 07:08:11PM +0200, Peter Zijlstra wrote:
> That would be bad, how can we force it to emit 5 bytes?

.byte 0xe9 like we used to do in static_cpu_has_safe().

Or you can copy the insn sizing from the alternatives macros which I
added recently.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Thu, Jul 23, 2015 at 07:54:36PM +0200, Borislav Petkov wrote:
> On Thu, Jul 23, 2015 at 07:08:11PM +0200, Peter Zijlstra wrote:
> > That would be bad, how can we force it to emit 5 bytes?
> 
> .byte 0xe9 like we used to do in static_cpu_has_safe().

Like so then?

static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
{
	unsigned long kval = (unsigned long)key + inv;

	asm_volatile_goto("1:"
		".byte 0xe9\n\t .long %l[l_yes]\n\t"
		".pushsection __jump_table,  \"aw\" \n\t"
		_ASM_ALIGN "\n\t"
		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
		".popsection \n\t"
		: :  "i" (kval) : : l_yes);

	return false;
l_yes:
	return true;
}

Re: Kernel broken on processors without performance counters

[Borislav Petkov]

On Thu, Jul 23, 2015 at 09:02:14PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 23, 2015 at 07:54:36PM +0200, Borislav Petkov wrote:
> > On Thu, Jul 23, 2015 at 07:08:11PM +0200, Peter Zijlstra wrote:
> > > That would be bad, how can we force it to emit 5 bytes?
> > 
> > .byte 0xe9 like we used to do in static_cpu_has_safe().
> 
> Like so then?
> 
> static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> {
> 	unsigned long kval = (unsigned long)key + inv;
> 
> 	asm_volatile_goto("1:"
> 		".byte 0xe9\n\t .long %l[l_yes]\n\t"
> 		".pushsection __jump_table,  \"aw\" \n\t"
> 		_ASM_ALIGN "\n\t"
> 		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> 		".popsection \n\t"
> 		: :  "i" (kval) : : l_yes);
> 
> 	return false;
> l_yes:
> 	return true;
> }

Yap.

But, we can do even better and note down what kind of JMP the compiler
generated and teach __jump_label_transform() to generate the right one.
Maybe this struct jump_entry would get a flags member or so. This way
we're optimal.

Methinks...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Fri, Jul 24, 2015 at 07:29:05AM +0200, Borislav Petkov wrote:
> On Thu, Jul 23, 2015 at 09:02:14PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 23, 2015 at 07:54:36PM +0200, Borislav Petkov wrote:
> > > On Thu, Jul 23, 2015 at 07:08:11PM +0200, Peter Zijlstra wrote:
> > > > That would be bad, how can we force it to emit 5 bytes?
> > > 
> > > .byte 0xe9 like we used to do in static_cpu_has_safe().
> > 
> > Like so then?
> > 
> > static __always_inline bool arch_static_branch_jump(struct static_key *key, bool inv)
> > {
> > 	unsigned long kval = (unsigned long)key + inv;
> > 
> > 	asm_volatile_goto("1:"
> > 		".byte 0xe9\n\t .long %l[l_yes]\n\t"
> > 		".pushsection __jump_table,  \"aw\" \n\t"
> > 		_ASM_ALIGN "\n\t"
> > 		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> > 		".popsection \n\t"
> > 		: :  "i" (kval) : : l_yes);
> > 
> > 	return false;
> > l_yes:
> > 	return true;
> > }
> 
> Yap.
> 
> But, we can do even better and note down what kind of JMP the compiler
> generated and teach __jump_label_transform() to generate the right one.
> Maybe this struct jump_entry would get a flags member or so. This way
> we're optimal.
> 
> Methinks...

Yes, Jason and Steve already have patches to go do that, but I'd really
like to keep that separate for now, this thing is big enough as is.

Re: Kernel broken on processors without performance counters

[Mikulas Patocka]

On Wed, 22 Jul 2015, Borislav Petkov wrote:

> On Tue, Jul 21, 2015 at 02:50:25PM -0400, Jason Baron wrote:
> > hmmm...so this is a case where need to the default the branch
> > to the out-of-line branch at boot. That is, we can't just enable
> > the out-of-line branch at boot time, b/c it might be too late at
> > that point? IE native_sched_clock() gets called very early?
> 
> Well, even the layout is wrong here. The optimal thing would be to have:
> 
> 	NOP
> 	rdtsc
> 
> unlikely:
> 	/* read jiffies */
> 
> at build time. And then at boot time, patch in the JMP over the NOP on
> !use_tsc boxes. And RDTSC works always, no matter how early.

RDTSC doesn't work on 486...

Mikulas

Re: Kernel broken on processors without performance counters

[Thomas Gleixner]

On Tue, 21 Jul 2015, Peter Zijlstra wrote:
> On Tue, Jul 21, 2015 at 05:43:27PM +0200, Thomas Gleixner wrote:
> > > > +#define static_branch_inc(_k)	static_key_slow_inc(&(_k)->key)
> > > > +#define static_branch_dec(_k)	static_key_slow_dec(&(_k)->key)
> > 
> > Inlines please
> > 
> > > > +/*
> > > > + * Normal usage; boolean enable/disable.
> > > > + */
> > > > +
> > > > +#define static_branch_enable(_k)	static_key_enable(&(_k)->key)
> > > > +#define static_branch_disable(_k)	static_key_disable(&(_k)->key)
> > 
> > Ditto
> > 
> > All in all much more understandable than the existing horror.
> 
> They cannot in fact be inlines because we play type tricks. Note how _k
> can be either struct static_likely_key or struct static_unlikely_key.

Indeed. Care to add a comment?

Thanks,

	tglx

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Tue, Jul 21, 2015 at 05:49:59PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 21, 2015 at 05:43:27PM +0200, Thomas Gleixner wrote:
> > On Tue, 21 Jul 2015, Peter Zijlstra wrote:
> > 
> > > > -#endif	/* _LINUX_JUMP_LABEL_H */
> > > > +static inline 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);
> > > > +}
> > > > +
> > > > +static inline 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);
> > > > +}
> > 
> > The functions above are not part of the interface which should be used
> > in code, right?
> 
> They are in fact. They make it easier to deal with the refcount thing
> when all you want is boolean states.

That is, things like sched_feat_keys[] which is an array of static keys,
the split types doesn't work -- an array can after all have only one
type.

In such cases you do have to be very careful to not wreck things.

Re: Kernel broken on processors without performance counters

[Peter Zijlstra]

On Wed, Jul 08, 2015 at 04:04:32PM -0400, Jason Baron wrote:
> So the 'static_key_false' is really branch is initially false. We had
> a naming discussion before, but if ppl think its confusing,
> 'static_key_init_false', or 'static_key_default_false' might be better,
> or other ideas.... I agree its confusing.

Jason, while I have you here on the subject; mind posting that true/fase
thing again? That makes more sense than the inc/dec thing for some usage
sites.

Re: Kernel broken on processors without performance counters

[Jason Baron]

On 07/09/2015 01:11 PM, Peter Zijlstra wrote:
> On Wed, Jul 08, 2015 at 04:04:32PM -0400, Jason Baron wrote:
>> So the 'static_key_false' is really branch is initially false. We had
>> a naming discussion before, but if ppl think its confusing,
>> 'static_key_init_false', or 'static_key_default_false' might be better,
>> or other ideas.... I agree its confusing.
> Jason, while I have you here on the subject; mind posting that true/fase
> thing again? That makes more sense than the inc/dec thing for some usage
> sites.
>
>

Ok, sounds good. I will try and get to this next week.

Thanks,

-Jason

Re: Kernel broken on processors without performance counters

[Borislav Petkov]

On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
> Hi
> 
> I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks 
> the kernel on processors without performance counters, such as AMD K6-3.

Out of curiosity: are you actually using this piece of computer history
or you're only booting new kernels for regressions?

:)

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: Kernel broken on processors without performance counters

[Mikulas Patocka]

On Tue, 14 Jul 2015, Borislav Petkov wrote:

> On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
> > Hi
> > 
> > I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks 
> > the kernel on processors without performance counters, such as AMD K6-3.
> 
> Out of curiosity: are you actually using this piece of computer history
> or you're only booting new kernels for regressions?
> 
> :)
> 
> Thanks.

I use it, but mostly for connecting to other machines. I wanted to test 
some other patch, I compiled a new kernel on K6-3 and found out this 
crash.

Mikulas