Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1190118 > unrolled thread

Re: Kernel broken on processors without performance counters

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Kernel broken on processors without performance counters Jason Baron <jasonbaron0@gmail.com> - 2015-07-22 19:10 +0200
    Re: Kernel broken on processors without performance counters Peter Zijlstra <peterz@infradead.org> - 2015-07-23 19:10 +0200
      Re: Kernel broken on processors without performance counters Steven Rostedt <rostedt@goodmis.org> - 2015-07-23 19:20 +0200
      Re: Kernel broken on processors without performance counters Jason Baron <jasonbaron0@gmail.com> - 2015-07-23 19:40 +0200
        Re: Kernel broken on processors without performance counters Steven Rostedt <rostedt@goodmis.org> - 2015-07-23 20:20 +0200
        Re: Kernel broken on processors without performance counters Peter Zijlstra <peterz@infradead.org> - 2015-07-23 21:10 +0200
      Re: Kernel broken on processors without performance counters Andy Lutomirski <luto@amacapital.net> - 2015-07-23 19:40 +0200
      Re: Kernel broken on processors without performance counters Borislav Petkov <bp@alien8.de> - 2015-07-23 20:00 +0200
        Re: Kernel broken on processors without performance counters Peter Zijlstra <peterz@infradead.org> - 2015-07-23 21:10 +0200
          Re: Kernel broken on processors without performance counters Peter Zijlstra <peterz@infradead.org> - 2015-07-24 12:40 +0200
    Re: Kernel broken on processors without performance counters Peter Zijlstra <peterz@infradead.org> - 2015-07-24 13:00 +0200
      Re: Kernel broken on processors without performance counters Peter Zijlstra <peterz@infradead.org> - 2015-07-24 14:40 +0200
        Re: Kernel broken on processors without performance counters Jason Baron <jasonbaron0@gmail.com> - 2015-07-24 16:20 +0200

#1190118 — Re: Kernel broken on processors without performance counters

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;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191130

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191147

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191170

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191198

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191243

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191178

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191184

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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191236

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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191704

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191715

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191784

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191876

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web