Re: [PATCH for-4.22 v2 7/8] x86/mwait-idle: Add cmdline option to adjust C-states table

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 15, 2026 at 08:57:45AM +0200, Jan Beulich wrote:
> On 14.05.2026 17:18, Roger Pau Monné wrote:
> > On Tue, May 12, 2026 at 05:38:08PM +0200, Jan Beulich wrote:
> >> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >>
> >> Add a new module parameter that allows adjusting the C-states table used by
> >> the driver.
> >>
> >> Currently, the C-states table is hardcoded in the driver based on the CPU
> >> model. The goal is to have good enough defaults for most users.
> >>
> >> However, C-state characteristics, such as exit latency and residency, can
> >> vary between different variants of the same CPU model and BIOS settings.
> >> Moreover, different platform usage models and user preferences may benefit
> >> from different C-state target_residency values.
> >>
> >> Provide a way for users to adjust the C-states table via a module parameter
> >> "table". The general format is:
> >> "state1:latency1:target_residency1,state2:latency2:target_residency2,..."
> >>
> >> In other words, represent each C-state by its name, exit latency (in
> >> microseconds), and target residency (in microseconds), separated by colons.
> >> Separate multiple C-states by commas.
> >>
> >> For example, suppose a CPU has 3 C-states with the following
> >> characteristics:
> >>   C1:  exit_latency=1, target_residency=2
> >>   C1E: exit_latency=10, target_residency=10
> >>   C6:  exit_latency=100, target_residency=500
> >>
> >> Users can specify a custom C-states table as follows:
> >>
> >> 1. intel_idle.table="C1:2:2,C1E:5:20,C6:150:600"
> >>    Result: C1:  exit_latency=2, target_residency=2
> >>            C1E: exit_latency=5, target_residency=20
> >>            C6:  exit_latency=150, target_residency=600
> >> 2. intel_idle.table="C6::400"
> >>    Result: C1:  exit_latency=1, target_residency=2 (unchanged)
> >>            C1E: exit_latency=10, target_residency=10 (unchanged)
> >>            C6:  exit_latency=100, target_residency=400
> >>                 (only target_residency changed)
> >>
> >> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >> Link: https://patch.msgid.link/20251216080402.156988-3-dedekind1@gmail.com
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 111f77a23348
> >>
> >> Add __init to get_cmdline_field(). Put cmdline_table_str[] in .init.data.
> >> Other adjustments to fit our env.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> +/**
> >> + * cmdline_table_adjust - Adjust the C-states table with data from cmdline.
> >> + *
> >> + * Adjust the C-states table with data from the 'mwait-idle.table' parameter
> >> + * (if specified).
> >> + */
> >> +static void __init cmdline_table_adjust(void)
> >> +{
> >> +	char *args = cmdline_table_str;
> >> +	struct cpuidle_state *state;
> >> +	unsigned int i, state_count;
> >> +
> >> +	if (args[0] == '\0')
> >> +		/* The 'mwait-idle.table' module parameter was not specified */
> >> +		return;
> >> +
> >> +	/* Create a copy of the C-states table */
> >> +	for (i = 0;
> >> +	     i < ARRAY_SIZE(cmdline_states) && icpu.state_table[i].name[0];
> >> +	     i++)
> >> +		cmdline_states[i] = icpu.state_table[i];
> >> +
> >> +	state_count = i;
> >> +
> >> +	/*
> >> +	 * Adjust the C-states table copy with data from the 'mwait-idle.table'
> >> +	 * module parameter.
> >> +	 */
> >> +	while (args) {
> >> +		char *fields, *name, *val;
> >> +
> >> +		/*
> >> +		 * Get the next C-state definition, which is expected to be
> >> +		 * '<name>:<latency_us>:<target_residency_us>'. Treat "empty"
> >> +		 * fields as unchanged. For example,
> >> +		 * '<name>::<target_residency_us>' leaves the latency unchanged.
> >> +		 */
> >> +		args = get_cmdline_field(args, &fields, ',');
> >> +
> >> +		/* name */
> >> +		fields = get_cmdline_field(fields, &name, ':');
> >> +		if (!fields)
> >> +			goto error;
> >> +
> >> +		/* Find the C-state by its name */
> >> +		state = NULL;
> >> +		for (i = 0; i < state_count; i++) {
> >> +			if (!strcmp(name, cmdline_states[i].name)) {
> >> +				state = &cmdline_states[i];
> >> +				break;
> >> +			}
> >> +		}
> >> +
> >> +		if (!state) {
> >> +			printk(XENLOG_ERR PREFIX "C-state '%s' was not found\n",
> >> +			       name);
> >> +			continue;
> >> +		}
> >> +
> >> +		/* Latency */
> >> +		fields = get_cmdline_field(fields, &val, ':');
> >> +		if (!fields)
> >> +			goto error;
> >> +
> >> +		if (*val) {
> >> +			const char *end;
> >> +			unsigned long n = simple_strtoul(val, &end, 0);
> >> +
> >> +			state->exit_latency = n;
> >> +			if (*end || state->exit_latency != n)
> >> +				goto error;
> >> +		}
> >> +
> >> +		/* Target residency */
> >> +		fields = get_cmdline_field(fields, &val, ':');
> >> +
> >> +		if (*val) {
> >> +			const char *end;
> >> +			unsigned long n = simple_strtoul(val, &end, 0);
> >> +
> >> +			state->target_residency = n;
> >> +			if (*end || state->target_residency != n)
> >> +				goto error;
> >> +		}
> >> +
> >> +		/*
> >> +		 * Allow for 3 more fields, but ignore them. Helps to make
> >> +		 * possible future extensions of the cmdline format backward
> >> +		 * compatible.
> >> +		 */
> >> +		for (i = 0; fields && i < 3; i++) {
> >> +			fields = get_cmdline_field(fields, &val, ':');
> >> +			if (!fields)
> >> +				break;
> >> +		}
> >> +
> >> +		if (fields) {
> >> +			printk(XENLOG_ERR PREFIX
> >> +			       "Too many fields for C-state '%s'\n",
> >> +			       state->name);
> >> +			goto error;
> >> +		}
> >> +
> >> +		printk(XENLOG_INFO PREFIX
> >> +		       "C-state from cmdline: name=%s, latency=%u, residency=%u\n",
> >> +		       state->name, state->exit_latency, state->target_residency);
> >> +	}
> >> +
> >> +	/* Copy the adjusted C-states table back */
> >> +	for (i = 0; i < state_count; i++)
> >> +		icpu.state_table[i] = cmdline_states[i];
> >> +
> >> +	printk(XENLOG_INFO PREFIX
> >> +	       "Adjusted C-states with data from 'mwait-idle.table'\n");
> >> +	return;
> >> +
> >> + error:
> >> +	printk(PREFIX
> > 
> > XENLOG_ERR ahead of the prefix maybe?
> 
> I did already raise the level from info to warning, compared to the Linux
> original. I didn't want to go yet farther with this, hence why I'd prefer
> to leave out XENLOG_* altogether here.

It seems like the outlier to me.  All other printk() instances in the
function use an explicit prefix, so I would think it might be best to
also add an explicit prefix here.

Thanks, Roger.