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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 03, 2019 at 12:59:36PM +0000, 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>

Just one comment, regardless of which:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> v2: Adjust xenpm output wording a little. Explicitly log "unlimited".
> ---
> 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
> @@ -1376,6 +1376,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.
> +
>   ### 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);
> +    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);
> +
>       if ( !xc_set_cpuidle_max_cstate(xc_handle, (uint32_t)value) )
> -        printf("set max_cstate to C%d succeeded\n", value);
> +        printf("max C-state set to %s\n", value >= 0 ? buf : argv[0]);
>       else
> -        fprintf(stderr, "set max_cstate to C%d failed (%d - %s)\n",
> -                value, errno, strerror(errno));
> +        fprintf(stderr, "Failed to set max C-state to %s (%d - %s)\n",
> +                value >= 0 ? buf : argv[0], errno, strerror(errno));
>   }
>   
>   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;

Not sure whether it would be clearer to just use
XEN_SYSCTL_CX_UNLIMITED here instead of UINT_MAX.

Thanks, Roger.

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