Re: Kernel broken on processors without performance counters

[Jason Baron <jasonbaron0@gmail.com>]

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