Re: [PATCH v4] intel_idle: introduce 'no_native' module parameter

[David Arcari <darcari@redhat.com>]

Hi Rafael,

On 2/19/25 4:27 PM, Rafael J. Wysocki wrote:
> On Thu, Feb 13, 2025 at 5:07 PM David Arcari <darcari@redhat.com> wrote:
>>
>> Since commit 18734958e9bf ("intel_idle: Use ACPI _CST for processor models
>> without C-state tables") the intel_idle driver has had the ability to use
>> the ACPI _CST to populate C-states when the processor model is not
>> recognized. However, even when the processor model is recognized (native
>> mode) there are cases where it is useful to make the driver ignore the per
>> cpu idle states in lieu of ACPI C-states (such as specific application
>> performance). Add the 'no_native' module parameter to provide this
>> functionality.
>>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: David Arcari <darcari@redhat.com>
>> Cc: Artem Bityutskiy <dedekind1@gmail.com>
>> Cc: Prarit Bhargava <prarit@redhat.com>
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: David Arcari <darcari@redhat.com>
>> ---
>> v4: fix !CONFIG_ACPI_PROCESSOR_CSTATE compilation issue
>> v3: more documentation cleanup
>> v2: renamed parameter, cleaned up documentation
>>
>> Documentation/admin-guide/pm/intel_idle.rst | 18 +++++++++++++-----
>>   drivers/idle/intel_idle.c                   | 16 ++++++++++++++++
>>   2 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/pm/intel_idle.rst b/Documentation/admin-guide/pm/intel_idle.rst
>> index 39bd6ecce7de..5940528146eb 100644
>> --- a/Documentation/admin-guide/pm/intel_idle.rst
>> +++ b/Documentation/admin-guide/pm/intel_idle.rst
>> @@ -192,11 +192,19 @@ even if they have been enumerated (see :ref:`cpu-pm-qos` in
>>   Documentation/admin-guide/pm/cpuidle.rst).
>>   Setting ``max_cstate`` to 0 causes the ``intel_idle`` initialization to fail.
>>
>> -The ``no_acpi`` and ``use_acpi`` module parameters (recognized by ``intel_idle``
>> -if the kernel has been configured with ACPI support) can be set to make the
>> -driver ignore the system's ACPI tables entirely or use them for all of the
>> -recognized processor models, respectively (they both are unset by default and
>> -``use_acpi`` has no effect if ``no_acpi`` is set).
>> +The ``no_acpi``, ``use_acpi`` and ``no_native`` module parameters are
>> +recognized by ``intel_idle`` if the kernel has been configured with ACPI
>> +support.  In the case that ACPI is not configured these flags have no impact
>> +on functionality.
>> +
>> +``no_acpi`` - Do not use ACPI at all.  Only native mode is available, no
>> +ACPI mode.
>> +
>> +``use_acpi`` - No-op in ACPI mode, the driver will consult ACPI tables for
>> +C-states on/off status in native mode.
>> +
>> +``no_native`` - Work only in ACPI mode, no native mode available (ignore
>> +all custom tables).
>>
>>   The value of the ``states_off`` module parameter (0 by default) represents a
>>   list of idle states to be disabled by default in the form of a bitmask.
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index 118fe1d37c22..b0be5ef43ffc 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -1695,6 +1695,10 @@ static bool force_use_acpi __read_mostly; /* No effect if no_acpi is set. */
>>   module_param_named(use_acpi, force_use_acpi, bool, 0444);
>>   MODULE_PARM_DESC(use_acpi, "Use ACPI _CST for building the idle states list");
>>
>> +static bool no_native __read_mostly; /* No effect if no_acpi is set. */
>> +module_param_named(no_native, no_native, bool, 0444);
>> +MODULE_PARM_DESC(no_native, "Ignore cpu specific (native) idle states in lieu of ACPI idle states");
>> +
>>   static struct acpi_processor_power acpi_state_table __initdata;
>>
>>   /**
>> @@ -1834,6 +1838,11 @@ static bool __init intel_idle_off_by_default(unsigned int flags, u32 mwait_hint)
>>          }
>>          return true;
>>   }
>> +
>> +static inline bool ignore_native(void)
>> +{
>> +       return no_native & !no_acpi;
>> +}
>>   #else /* !CONFIG_ACPI_PROCESSOR_CSTATE */
>>   #define force_use_acpi (false)
>>
>> @@ -1843,6 +1852,7 @@ static inline bool intel_idle_off_by_default(unsigned int flags, u32 mwait_hint)
>>   {
>>          return false;
>>   }
>> +static inline bool ignore_native(void) { return false; }
>>   #endif /* !CONFIG_ACPI_PROCESSOR_CSTATE */
>>
>>   /**
>> @@ -2328,6 +2338,12 @@ static int __init intel_idle_init(void)
>>          pr_debug("MWAIT substates: 0x%x\n", mwait_substates);
>>
>>          icpu = (const struct idle_cpu *)id->driver_data;
>> +       if (ignore_native()) {
>> +               if (icpu) {
>> +                       pr_debug("ignoring native cpu idle states\n");
>> +                       icpu = NULL;
>> +               }
>> +       }
> 
> Why not
> 
> +       if (icpu && ignore_native()) {
> +              pr_debug("disregarding built-in CPU idle states table\n");
> +              icpu = NULL;
> +       }

That's cleaner.  I'll submit a v5 shortly.

Should the pr_debug be a pr_info?  I've waffled on this, but think that 
pr_info is probably a good idea.  Or do you prefer pr_debug?

Thanks,
-DA

> 
>>          if (icpu) {
>>                  if (icpu->state_table)
>>                          cpuidle_state_table = icpu->state_table;
>> --
>