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

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

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.

Thanks, Roger.