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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

> +
>  ### max_gsi_irqs (x86)
>  > `= <integer>`
>  
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -64,7 +64,7 @@ void show_help(void)
>              " set-sched-smt           enable|disable enable/disable 
> scheduler smt power saving\n"
>              " set-vcpu-migration-delay      <num> set scheduler vcpu 
> migration delay in us\n"
>              " get-vcpu-migration-delay            get scheduler vcpu 
> migration delay\n"
> -            " set-max-cstate        <num>         set the C-State limitation 
> (<num> >= 0)\n"
> +            " set-max-cstate        <num>|'unlimited' set the C-State 
> limitation (<num> >= 0)\n"
>              " start [seconds]                     start collect Cx/Px 
> statistics,\n"
>              "                                     output after CTRL-C or 
> SIGINT or several seconds.\n"
>              " enable-turbo-mode     [cpuid]       enable Turbo Mode for 
> processors that support it.\n"
> @@ -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 ?

> +    else
> +        printf("All C-states allowed\n\n");
> +
>      return 0;
>  }
>  
> @@ -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.

> +
>      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.

>      else
> -        fprintf(stderr, "set max_cstate to C%d failed (%d - %s)\n",
> -                value, errno, strerror(errno));
> +        fprintf(stderr, "set max C-state to %s failed (%d - %s)\n",
> +                value >= 0 ? buf : argv[0], errno, strerror(errno));

Similarly, I'd prefix "Attempt to " to this message, or alternatively
phrase it as "Failed to set ... (%d - %s)\n".

>  }
>  
>  void enable_turbo_mode(int argc, char *argv[])
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -103,7 +103,7 @@ bool lapic_timer_init(void)
>  }
>  
>  void (*__read_mostly pm_idle_save)(void);
> -unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
> +unsigned int max_cstate __read_mostly = UINT_MAX;
>  integer_param("max_cstate", max_cstate);
>  static bool __read_mostly local_apic_timer_c2_ok;
>  boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
> @@ -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.

~Andrew

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