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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2026 09:59, Roger Pau Monné wrote:
> 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@xxxxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxx>
>>>> Link: https://patch.msgid.link/20251216080402.156988-3-dedekind1@xxxxxxxxx
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>>> 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@xxxxxxxx>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> 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.

Okay, I added one (warning, not error).

Jan