Re: [Xen-devel] [PATCH 1/5] x86/cpuidle: switch to uniform meaning of "max_cstate="

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.19 at 17:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/05/2019 13:16, Jan Beulich wrote:
>> While the MWAIT idle driver already takes it to mean an actual C state,
>> the ACPI idle driver so far used it as a list index. The list index,
>> however, is an implementation detail of Xen and affected by firmware
>> settings (i.e. not necessarily uniform for a particular system).
>>
>> While touching this code also avoid invoking menu_get_trace_data()
>> when tracing is not active. For consistency do this also for the
>> MWAIT driver.
>>
>> Note that I'm intentionally not adding any sorting logic to set_cx():
>> Before and after this patch we assume entries to arrive in order, so
>> this would be an orthogonal change.
>>
>> Take the opportunity and add minimal documentation for the command line
>> option.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> TBD: I wonder if we really need struct acpi_processor_cx's idx field
>>      anymore. It's used in a number of (questionable) places ...
>>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1371,6 +1371,8 @@ This option is ignored in **pv-shim** mo
>>  ### max_cstate (x86)
>>  > `= <integer>`
>>  
>> +Specify the deepest C-state CPUs are permitted to be placed in.
> 
> Are these ACPI C states or system specific C states?

As per the description, with the other changes here these are now
uniformly ACPI C-states.

>> @@ -194,7 +194,11 @@ static int show_max_cstate(xc_interface
>>      if ( (ret = xc_get_cpuidle_max_cstate(xc_handle, &value)) )
>>          return ret;
>>  
>> -    printf("Max possible C-state: C%d\n\n", value);
>> +    if ( value < XEN_SYSCTL_CX_UNLIMITED )
>> +        printf("Max possible C-state: C%"PRIu32"\n\n", value);
> 
> %u ?

Why? In the tool stack we shouldn't make assumptions like we
do in the hypervisor. "value" is of type "uint32_t", hence its
format specifier ought to be PRIu32.

>> @@ -1117,18 +1121,24 @@ void get_vcpu_migration_delay_func(int a
>>  void set_max_cstate_func(int argc, char *argv[])
>>  {
>>      int value;
>> +    char buf[12];
>>  
>> -    if ( argc != 1 || sscanf(argv[0], "%d", &value) != 1 || value < 0 )
>> +    if ( argc != 1 ||
>> +         (sscanf(argv[0], "%d", &value) == 1
>> +          ? value < 0
>> +          : (value = XEN_SYSCTL_CX_UNLIMITED, strcmp(argv[0], 
>> "unlimited"))) )
>>      {
>> -        fprintf(stderr, "Missing or invalid argument(s)\n");
>> +        fprintf(stderr, "Missing, excess, or invalid argument(s)\n");
>>          exit(EINVAL);
>>      }
>>  
>> +    snprintf(buf, ARRAY_SIZE(buf), "C%d", value);
> 
> The logic below would be much more simple if buf was always correct,
> even in the unlimited case.

What do you mean by "always correct"? Do you want me to copy
"unlimited" into it for value < 0? This can be done, but the gain is
just two eliminated conditional expressions afaics. Not _much_
more simple imo.

>> +
>>      if ( !xc_set_cpuidle_max_cstate(xc_handle, (uint32_t)value) )
>> -        printf("set max_cstate to C%d succeeded\n", value);
>> +        printf("set max C-state to %s succeeded\n", value >= 0 ? buf : 
>> argv[0]);
> 
> I'd drop the "succeeded" part.  Its a bit awkward grammatically, and is
> superfluous to clear understanding of the message.

Well, I would have done so already if "set" on its own wasn't
ambiguous - this could be a "successfully done" as much as a
"I'm now going to" message. But thinking about it again, I
could make it "max C-state set to %s".

>> @@ -344,7 +344,8 @@ static void dump_cx(unsigned char key)
>>      unsigned int cpu;
>>  
>>      printk("'%c' pressed -> printing ACPI Cx structures\n", key);
>> -    printk("max cstate: C%u\n", max_cstate);
>> +    if ( max_cstate < UINT_MAX )
>> +        printk("max state: C%u\n", max_cstate);
> 
> As this is a diagnostic, it would benefit from explicitly printing
> unlimited.

Well, I typically try to produce less output where possible (and
where not becoming ambiguous), but since you ask for it I can
as well make it more verbose.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel