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 <jbeul...@suse.com>
> ---
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
