Re: Kernel broken on processors without performance counters

[Jason Baron <jasonbaron0@gmail.com>]

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