Re: [PATCH v3 1/5] x86/mwait-idle: add 'preferred-cstates' command line option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.10.2022 18:22, Roger Pau Monné wrote:
> On Thu, Aug 18, 2022 at 03:03:33PM +0200, Jan Beulich wrote:
>> @@ -1309,6 +1360,39 @@ static int __init mwait_idle_probe(void)
>>      pr_debug(PREFIX "lapic_timer_reliable_states %#x\n",
>>               lapic_timer_reliable_states);
>>  
>> +    str = preferred_states;
>> +    if (isdigit(str[0]))
>> +            preferred_states_mask = simple_strtoul(str, &str, 0);
>> +    else if (str[0])
>> +    {
>> +            const char *ss;
>> +
>> +            do {
>> +                    const struct cpuidle_state *state = icpu->state_table;
>> +                    unsigned int bit = 1;
>> +
>> +                    ss = strchr(str, ',');
>> +                    if (!ss)
>> +                            ss = strchr(str, '\0');
>> +
>> +                    for (; state->name[0]; ++state) {
>> +                            bit <<= 1;
>> +                            if (!cmdline_strcmp(str, state->name)) {
>> +                                    preferred_states_mask |= bit;
>> +                                    break;
>> +                            }
>> +                    }
>> +                    if (!state->name[0])
>> +                            break;
>> +
>> +                    str = ss + 1;
>> +        } while (*ss);
>> +
>> +        str -= str == ss + 1;
> 
> I would add parentheses to the sum for clarity.

If I was to add parentheses here, then like this:

    str -= (str == ss + 1);

Looks like I've screwed up with indentation here, though.

>> @@ -1400,8 +1484,18 @@ static int cf_check mwait_idle_cpu_init(
>>      if (icpu->byt_auto_demotion_disable_flag)
>>              on_selected_cpus(cpumask_of(cpu), byt_auto_demotion_disable, 
>> NULL, 1);
>>  
>> -    if (icpu->disable_promotion_to_c1e)
>> +    switch (icpu->c1e_promotion) {
>> +    case C1E_PROMOTION_DISABLE:
>>              on_selected_cpus(cpumask_of(cpu), c1e_promotion_disable, NULL, 
>> 1);
>> +            break;
>> +
>> +    case C1E_PROMOTION_ENABLE:
>> +            on_selected_cpus(cpumask_of(cpu), c1e_promotion_enable, NULL, 
>> 1);
>> +            break;
>> +
>> +    case C1E_PROMOTION_PRESERVE:
>> +            break;
>> +    }
> 
> I find it kind of weird to user a notifier for this, won't it be
> easier to set the C1E promotion as part of the CPU bringup process?

A CPU notifier _is_ part of the CPU bringup process, isn't it? So it's
not clear to me what alternative you're thinking of.

> I see we also set other bits in the same way.

Well, yes - right here I only extend what we already have in place.
Re-working that in whichever way ought to be a separate topic imo, and
preferably not be part of a port of a commit coming from Linux.

Jan