Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753319AbbGWOUA (ORCPT ); Thu, 23 Jul 2015 10:20:00 -0400 Received: from mail-qg0-f42.google.com ([209.85.192.42]:34411 "EHLO mail-qg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752437AbbGWOTz (ORCPT ); Thu, 23 Jul 2015 10:19:55 -0400 Message-ID: <55B0F808.2060302@gmail.com> Date: Thu, 23 Jul 2015 10:19:52 -0400 From: Jason Baron User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Peter Zijlstra CC: Borislav Petkov , Andy Lutomirski , Thomas Gleixner , Mikulas Patocka , Paul Mackerras , Arnaldo Carvalho de Melo , Kees Cook , Andrea Arcangeli , Vince Weaver , "hillf.zj" , Valdis Kletnieks , "linux-kernel@vger.kernel.org" , Steven Rostedt Subject: Re: Kernel broken on processors without performance counters References: <20150710141359.GJ19282@twins.programming.kicks-ass.net> <20150721082107.GE18673@twins.programming.kicks-ass.net> <20150721154959.GS19282@twins.programming.kicks-ass.net> <20150721161215.GU19282@twins.programming.kicks-ass.net> <20150721181553.GA3378@nazgul.tnic> <55AE9471.1000601@gmail.com> <20150722042403.GA6345@nazgul.tnic> <55AFCDA4.5010406@gmail.com> <20150723104215.GH25159@twins.programming.kicks-ass.net> In-Reply-To: <20150723104215.GH25159@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6875 Lines: 189 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 -- 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/