| Message ID | 20250128141139.2033088-1-darcari@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | intel_idle: introduce 'use_acpi_cst' module parameter | expand |
Hi David, On Tue, 2025-01-28 at 09:11 -0500, David Arcari wrote: > +The ``use_acpi_cst`` module parameter (recognized by ``intel_idle`` if the > +kernel has been configured with ACPI support) can be set to make the driver > +ignore the per cpu idle states in lieu of ACPI idle states. ``use_acpi_cst`` > +has no effect if ``no_acpi`` is set). With this change, there will be three parameters: * no_acpi * use_acpi * use_acpi_cst I would like to make the naming as intuitive as possible. We do not rename the first 2, but for the 3rd one, I think "force_acpi" would be a better name. Or perhaps "no_native"? * no_acpi - Do not use ACPI at all. Only native mode is available, no ACPI mode. * use_acpi - No-op in ACPI mode, consult ACPI tables for C-states on/off status in native mode. * force_acpi (or no_native?) - Work only in ACPI mode, no native mode available (ignore all custom tables). Additionally, I think we should enhance the documentation for 'no_acpi' and 'use_acpi' while we're at it. Otherwise, it is hard to distinguish between these three options. Would you consider another patch that improves the documentation for 'no_acpi' and 'use_acpi', and then adds the third parameter? Thanks, Artem!
Hi Artem, On 2/4/25 7:23 AM, Artem Bityutskiy wrote: > Hi David, > > On Tue, 2025-01-28 at 09:11 -0500, David Arcari wrote: > >> +The ``use_acpi_cst`` module parameter (recognized by ``intel_idle`` if the >> +kernel has been configured with ACPI support) can be set to make the driver >> +ignore the per cpu idle states in lieu of ACPI idle states. ``use_acpi_cst`` >> +has no effect if ``no_acpi`` is set). > > With this change, there will be three parameters: > > * no_acpi > * use_acpi > * use_acpi_cst > > I would like to make the naming as intuitive as possible. We do not rename the > first 2, but for the 3rd one, I think "force_acpi" would be a better name. Or > perhaps "no_native"? The problem with force_acpi is it is very similar to force_use_acpi which is what intel_idle.c uses internally: drivers/idle/intel_idle.c:module_param_named(use_acpi, force_use_acpi, bool, 0444); That said, I am not attached to the 'use_acpi_cst' parameter name. > > * no_acpi - Do not use ACPI at all. Only native mode is available, no ACPI mode. > * use_acpi - No-op in ACPI mode, consult ACPI tables for C-states on/off > status in native mode. > * force_acpi (or no_native?) - Work only in ACPI mode, no native mode available > (ignore all custom tables). > > Additionally, I think we should enhance the documentation for 'no_acpi' and > 'use_acpi' while we're at it. Otherwise, it is hard to distinguish between these > three options. Would you consider another patch that improves the documentation > for 'no_acpi' and 'use_acpi', and then adds the third parameter? I'm happy to resubmit. I guess I could use 'no_native' for the new parameter and then update the documentation as you suggest above. Does that work? > > Thanks, Artem! > Best, -DA
On 2/4/25 7:52 AM, David Arcari wrote: > > Hi Artem, > > On 2/4/25 7:23 AM, Artem Bityutskiy wrote: >> Hi David, >> >> On Tue, 2025-01-28 at 09:11 -0500, David Arcari wrote: >> >>> +The ``use_acpi_cst`` module parameter (recognized by ``intel_idle`` >>> if the >>> +kernel has been configured with ACPI support) can be set to make the >>> driver >>> +ignore the per cpu idle states in lieu of ACPI idle states. >>> ``use_acpi_cst`` >>> +has no effect if ``no_acpi`` is set). >> >> With this change, there will be three parameters: >> >> * no_acpi >> * use_acpi >> * use_acpi_cst >> >> I would like to make the naming as intuitive as possible. We do not >> rename the >> first 2, but for the 3rd one, I think "force_acpi" would be a better >> name. Or >> perhaps "no_native"? > > The problem with force_acpi is it is very similar to force_use_acpi > which is what intel_idle.c uses internally: > Given that @Artem is suggesting we change the names of things -- perhaps we should fix "force_use_acpi" as well? P. > drivers/idle/intel_idle.c:module_param_named(use_acpi, force_use_acpi, > bool, 0444); > > That said, I am not attached to the 'use_acpi_cst' parameter name. > >> >> * no_acpi - Do not use ACPI at all. Only native mode is available, no >> ACPI mode. >> * use_acpi - No-op in ACPI mode, consult ACPI tables for C-states on/off >> status in native mode. >> * force_acpi (or no_native?) - Work only in ACPI mode, no native mode >> available >> (ignore all custom tables). >> >> Additionally, I think we should enhance the documentation for >> 'no_acpi' and >> 'use_acpi' while we're at it. Otherwise, it is hard to distinguish >> between these >> three options. Would you consider another patch that improves the >> documentation >> for 'no_acpi' and 'use_acpi', and then adds the third parameter? > > I'm happy to resubmit. I guess I could use 'no_native' for the new > parameter and then update the documentation as you suggest above. > > Does that work? > >> >> Thanks, Artem! >> > > Best, > -DA >
On Tue, Feb 4, 2025 at 2:07 PM David Arcari <darcari@redhat.com> wrote: > > > Hi Artem, > > On 2/4/25 7:23 AM, Artem Bityutskiy wrote: > > Hi David, > > > > On Tue, 2025-01-28 at 09:11 -0500, David Arcari wrote: > > > >> +The ``use_acpi_cst`` module parameter (recognized by ``intel_idle`` if the > >> +kernel has been configured with ACPI support) can be set to make the driver > >> +ignore the per cpu idle states in lieu of ACPI idle states. ``use_acpi_cst`` > >> +has no effect if ``no_acpi`` is set). > > > > With this change, there will be three parameters: > > > > * no_acpi > > * use_acpi > > * use_acpi_cst > > > > I would like to make the naming as intuitive as possible. We do not rename the > > first 2, but for the 3rd one, I think "force_acpi" would be a better name. Or > > perhaps "no_native"? > > The problem with force_acpi is it is very similar to force_use_acpi > which is what intel_idle.c uses internally: > > drivers/idle/intel_idle.c:module_param_named(use_acpi, force_use_acpi, > bool, 0444); > > That said, I am not attached to the 'use_acpi_cst' parameter name. IMV this is rather about ignoring the built-in states table altogether, IOW something like "pretend that you don't recognize the processor". But it could be something like "prefer_acpi" as far as I'm concerned.
On Tue, 2025-02-04 at 07:52 -0500, David Arcari wrote: > I'm happy to resubmit. I guess I could use 'no_native' for the new > parameter and then update the documentation as you suggest above. > > Does that work? I'd suggest to wait for Rafael's response first. Thanks! Artem.
On Tue, 2025-02-04 at 16:21 +0100, Rafael J. Wysocki wrote:
> But it could be something like "prefer_acpi" as far as I'm concerned.
When I see "prefer_acpi", my intuition tells that it is just a preference:
"prefer ACPI, but may be native too". But I understood that the patch is about
"only ACPI and never native".
The reasons I suggested "no_native":
* Sort of consistent with "no_acpi"
* Suggests that native won't work.
Thanks.
On Tue, Feb 4, 2025 at 5:30 PM Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Tue, 2025-02-04 at 16:21 +0100, Rafael J. Wysocki wrote: > > But it could be something like "prefer_acpi" as far as I'm concerned. > > When I see "prefer_acpi", my intuition tells that it is just a preference: > "prefer ACPI, but may be native too". But I understood that the patch is about > "only ACPI and never native". > > The reasons I suggested "no_native": > * Sort of consistent with "no_acpi" > * Suggests that native won't work. "no_native" would be fine by me too.
On 2/4/25 12:33 PM, Rafael J. Wysocki wrote: > On Tue, Feb 4, 2025 at 5:30 PM Artem Bityutskiy <dedekind1@gmail.com> wrote: >> >> On Tue, 2025-02-04 at 16:21 +0100, Rafael J. Wysocki wrote: >>> But it could be something like "prefer_acpi" as far as I'm concerned. >> >> When I see "prefer_acpi", my intuition tells that it is just a preference: >> "prefer ACPI, but may be native too". But I understood that the patch is about >> "only ACPI and never native". >> >> The reasons I suggested "no_native": >> * Sort of consistent with "no_acpi" >> * Suggests that native won't work. > > "no_native" would be fine by me too. > I will send out a v2 using 'no_native' for the new parameter. -DA
diff --git a/Documentation/admin-guide/pm/intel_idle.rst b/Documentation/admin-guide/pm/intel_idle.rst index 39bd6ecce7de..a87238bcf33d 100644 --- a/Documentation/admin-guide/pm/intel_idle.rst +++ b/Documentation/admin-guide/pm/intel_idle.rst @@ -198,6 +198,11 @@ 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 ``use_acpi_cst`` module parameter (recognized by ``intel_idle`` if the +kernel has been configured with ACPI support) can be set to make the driver +ignore the per cpu idle states in lieu of ACPI idle states. ``use_acpi_cst`` +has no effect if ``no_acpi`` is set). + 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..b8a536b930e7 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 use_acpi_cst __read_mostly; /* No effect if no_acpi is set. */ +module_param_named(use_acpi_cst, use_acpi_cst, bool, 0444); +MODULE_PARM_DESC(use_acpi_cst, "Ignore cpu specific idle states in lieu of ACPI idle states"); + static struct acpi_processor_power acpi_state_table __initdata; /** @@ -1836,6 +1840,7 @@ static bool __init intel_idle_off_by_default(unsigned int flags, u32 mwait_hint) } #else /* !CONFIG_ACPI_PROCESSOR_CSTATE */ #define force_use_acpi (false) +#define use_acpi_cst (false) static inline bool intel_idle_acpi_cst_extract(void) { return false; } static inline void intel_idle_init_cstates_acpi(struct cpuidle_driver *drv) { } @@ -2328,6 +2333,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 (use_acpi_cst && !no_acpi) { + if (icpu) { + pr_debug("ignoring cpu idle states\n"); + icpu = NULL; + } + } if (icpu) { if (icpu->state_table) cpuidle_state_table = icpu->state_table;
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 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 'use_acpi_cst' module parameter to provide this functionality (not to be confused with the 'use_acpi' module parameter which has a different function). 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: 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> --- Documentation/admin-guide/pm/intel_idle.rst | 5 +++++ drivers/idle/intel_idle.c | 11 +++++++++++ 2 files changed, 16 insertions(+)