Re: Kernel broken on processors without performance counters

[Jason Baron <jasonbaron0@gmail.com>]

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