On Wed, 16 Nov 2022 at 16:29, Peter Zijlstra [off-list ref] wrote:

Sorry; things keep getting in the way of finishing this :/

As such, I need a bit of time to get on-track again..

On Tue, Oct 04, 2022 at 01:03:57PM +0200, Ulf Hansson wrote:

quoted

quoted

--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c

@@ -1200,6 +1200,8 @@ static int acpi_processor_setup_lpi_stat
                state->target_residency = lpi->min_residency;
                if (lpi->arch_flags)
                        state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+               if (lpi->entry_method == ACPI_CSTATE_FFH)
+                       state->flags |= CPUIDLE_FLAG_RCU_IDLE;

I assume the state index here will never be 0?

If not, it may lead to that acpi_processor_ffh_lpi_enter() may trigger
CPU_PM_CPU_IDLE_ENTER_PARAM() to call ct_cpuidle_enter|exit() for an
idle-state that doesn't have the CPUIDLE_FLAG_RCU_IDLE bit set.

I'm not quite sure I see how. AFAICT this condition above implies
acpi_processor_ffh_lpi_enter() gets called, no?

Which in turn is an unconditional __CPU_PM_CPU_IDLE_ENTER() user, so
even if idx==0, it ends up in ct_idle_{enter,exit}().

Seems like I was overlooking something here, you are right, this
shouldn't really be a problem.

quoted

quoted

                state->enter = acpi_idle_lpi_enter;
                drv->safe_state_index = i;
        }

--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c

@@ -53,6 +53,7 @@ static struct cpuidle_driver arm_idle_dr
         * handler for idle state index 0.
         */
        .states[0] = {
+               .flags                  = CPUIDLE_FLAG_RCU_IDLE,

Comparing arm64 and arm32 idle-states/idle-drivers, the $subject
series ends up setting the CPUIDLE_FLAG_RCU_IDLE for the ARM WFI idle
state (state zero), but only for the arm64 and psci cases (mostly
arm64). For arm32 we would need to update the ARM_CPUIDLE_WFI_STATE
too, as that is what most arm32 idle-drivers are using. My point is,
the code becomes a bit inconsistent.

True.

quoted

Perhaps it's easier to avoid setting the CPUIDLE_FLAG_RCU_IDLE bit for
all of the ARM WFI idle states, for both arm64 and arm32?

As per the below?

quoted

quoted

                .enter                  = arm_enter_idle_state,
                .exit_latency           = 1,
                .target_residency       = 1,

quoted

quoted

--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h

@@ -282,14 +282,18 @@ extern s64 cpuidle_governor_latency_req(
        int __ret = 0;                                                  \
                                                                        \
        if (!idx) {                                                     \
+               ct_idle_enter();                                        \

According to my comment above, we should then drop these calls to
ct_idle_enter and ct_idle_exit() here. Right?

Yes, if we ensure idx==0 never has RCU_IDLE set then these must be
removed.

quoted

quoted

                cpu_do_idle();                                          \
+               ct_idle_exit();                                         \
                return idx;                                             \
        }                                                               \
                                                                        \
        if (!is_retention)                                              \
                __ret =  cpu_pm_enter();                                \
        if (!__ret) {                                                   \
+               ct_idle_enter();                                        \
                __ret = low_level_idle_enter(state);                    \
+               ct_idle_exit();                                         \
                if (!is_retention)                                      \
                        cpu_pm_exit();                                  \
        }                                                               \

So the basic premise is that everything that needs RCU inside the idle
callback must set CPUIDLE_FLAG_RCU_IDLE and by doing that promise to
call ct_idle_{enter,exit}() themselves.

Setting RCU_IDLE is required when there is RCU usage, however even if
there is no RCU usage, setting RCU_IDLE is fine, as long as
ct_idle_{enter,exit}() then get called.

Right, I was thinking that it could make sense to shrink the window
for users getting this wrong. In other words, we shouldn't set the
CPUIDLE_FLAG_RCU_IDLE unless we really need to.

And as I said, consistent behaviour is also nice to have.

So does the below (delta) look better to you?

Yes, it does!

Although, one minor comment below.

Nitpick: I don't think we need to explicitly clear the flag, as it
should already be zeroed by the compiler from its static declaration.
Right?

Kind regards
Uffe