cpu/hotplug: Add cpuhp_invoke_callback_range()

Factorizing and unifying cpuhp callback range invocations, especially for
the hotunplug path, where two different ways of decrementing were used. The
first one, decrements before the callback is called:

 cpuhp_thread_fun()
     state = st->state;
     st->state--;
     cpuhp_invoke_callback(state);

The second one, after:

 take_down_cpu()|cpuhp_down_callbacks()
     cpuhp_invoke_callback(st->state);
     st->state--;

This is problematic for rolling back the steps in case of error, as
depending on the decrement, the rollback will start from N or N-1. It also
makes tracing inconsistent, between steps run in the cpuhp thread and
the others.

Additionally, avoid useless cpuhp_thread_fun() loops by skipping empty
steps.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lkml.kernel.org/r/20210216103506.416286-4-vincent.donnefort@arm.com

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 680ed8f..23505d6 100644 (file)
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -135,6 +135,11 @@ static struct cpuhp_step *cpuhp_get_step(enum cpuhp_state state)
        return cpuhp_hp_states + state;
 }
 
+static bool cpuhp_step_empty(bool bringup, struct cpuhp_step *step)
+{
+       return bringup ? !step->startup.single : !step->teardown.single;
+}
+
 /**
  * cpuhp_invoke_callback _ Invoke the callbacks for a given state
  * @cpu:       The cpu for which the callback should be invoked
@@ -157,26 +162,24 @@ static int cpuhp_invoke_callback(unsigned int cpu, enum cpuhp_state state,
 
        if (st->fail == state) {
                st->fail = CPUHP_INVALID;
-
-               if (!(bringup ? step->startup.single : step->teardown.single))
-                       return 0;
-
                return -EAGAIN;
        }
 
+       if (cpuhp_step_empty(bringup, step)) {
+               WARN_ON_ONCE(1);
+               return 0;
+       }
+
        if (!step->multi_instance) {
                WARN_ON_ONCE(lastp && *lastp);
                cb = bringup ? step->startup.single : step->teardown.single;
-               if (!cb)
-                       return 0;
+
                trace_cpuhp_enter(cpu, st->target, state, cb);
                ret = cb(cpu);
                trace_cpuhp_exit(cpu, st->state, state, ret);
                return ret;
        }
        cbm = bringup ? step->startup.multi : step->teardown.multi;
-       if (!cbm)
-               return 0;
 
        /* Single invocation for instance add/remove */
        if (node) {
@@ -475,6 +478,15 @@ cpuhp_set_state(struct cpuhp_cpu_state *st, enum cpuhp_state target)
 static inline void
 cpuhp_reset_state(struct cpuhp_cpu_state *st, enum cpuhp_state prev_state)
 {
+       st->target = prev_state;
+
+       /*
+        * Already rolling back. No need invert the bringup value or to change
+        * the current state.
+        */
+       if (st->rollback)
+               return;
+
        st->rollback = true;
 
        /*
@@ -488,7 +500,6 @@ cpuhp_reset_state(struct cpuhp_cpu_state *st, enum cpuhp_state prev_state)
                        st->state++;
        }
 
-       st->target = prev_state;
        st->bringup = !st->bringup;
 }
 
@@ -591,10 +602,53 @@ static int finish_cpu(unsigned int cpu)
  * Hotplug state machine related functions
  */
 
-static void undo_cpu_up(unsigned int cpu, struct cpuhp_cpu_state *st)
+/*
+ * Get the next state to run. Empty ones will be skipped. Returns true if a
+ * state must be run.
+ *
+ * st->state will be modified ahead of time, to match state_to_run, as if it
+ * has already ran.
+ */
+static bool cpuhp_next_state(bool bringup,
+                            enum cpuhp_state *state_to_run,
+                            struct cpuhp_cpu_state *st,
+                            enum cpuhp_state target)
 {
-       for (st->state--; st->state > st->target; st->state--)
-               cpuhp_invoke_callback(cpu, st->state, false, NULL, NULL);
+       do {
+               if (bringup) {
+                       if (st->state >= target)
+                               return false;
+
+                       *state_to_run = ++st->state;
+               } else {
+                       if (st->state <= target)
+                               return false;
+
+                       *state_to_run = st->state--;
+               }
+
+               if (!cpuhp_step_empty(bringup, cpuhp_get_step(*state_to_run)))
+                       break;
+       } while (true);
+
+       return true;
+}
+
+static int cpuhp_invoke_callback_range(bool bringup,
+                                      unsigned int cpu,
+                                      struct cpuhp_cpu_state *st,
+                                      enum cpuhp_state target)
+{
+       enum cpuhp_state state;
+       int err = 0;
+
+       while (cpuhp_next_state(bringup, &state, st, target)) {
+               err = cpuhp_invoke_callback(cpu, state, bringup, NULL, NULL);
+               if (err)
+                       break;
+       }
+
+       return err;
 }
 
 static inline bool can_rollback_cpu(struct cpuhp_cpu_state *st)
@@ -617,16 +671,12 @@ static int cpuhp_up_callbacks(unsigned int cpu, struct cpuhp_cpu_state *st,
        enum cpuhp_state prev_state = st->state;
        int ret = 0;
 
-       while (st->state < target) {
-               st->state++;
-               ret = cpuhp_invoke_callback(cpu, st->state, true, NULL, NULL);
-               if (ret) {
-                       if (can_rollback_cpu(st)) {
-                               st->target = prev_state;
-                               undo_cpu_up(cpu, st);
-                       }
-                       break;
-               }
+       ret = cpuhp_invoke_callback_range(true, cpu, st, target);
+       if (ret) {
+               cpuhp_reset_state(st, prev_state);
+               if (can_rollback_cpu(st))
+                       WARN_ON(cpuhp_invoke_callback_range(false, cpu, st,
+                                                           prev_state));
        }
        return ret;
 }
@@ -690,17 +740,9 @@ static void cpuhp_thread_fun(unsigned int cpu)
                state = st->cb_state;
                st->should_run = false;
        } else {
-               if (bringup) {
-                       st->state++;
-                       state = st->state;
-                       st->should_run = (st->state < st->target);
-                       WARN_ON_ONCE(st->state > st->target);
-               } else {
-                       state = st->state;
-                       st->state--;
-                       st->should_run = (st->state > st->target);
-                       WARN_ON_ONCE(st->state < st->target);
-               }
+               st->should_run = cpuhp_next_state(bringup, &state, st, st->target);
+               if (!st->should_run)
+                       goto end;
        }
 
        WARN_ON_ONCE(!cpuhp_is_ap_state(state));
@@ -728,6 +770,7 @@ static void cpuhp_thread_fun(unsigned int cpu)
                st->should_run = false;
        }
 
+end:
        cpuhp_lock_release(bringup);
        lockdep_release_cpus_lock();
 
@@ -881,19 +924,18 @@ static int take_cpu_down(void *_param)
                return err;
 
        /*
-        * We get here while we are in CPUHP_TEARDOWN_CPU state and we must not
-        * do this step again.
+        * Must be called from CPUHP_TEARDOWN_CPU, which means, as we are going
+        * down, that the current state is CPUHP_TEARDOWN_CPU - 1.
         */
-       WARN_ON(st->state != CPUHP_TEARDOWN_CPU);
-       st->state--;
+       WARN_ON(st->state != (CPUHP_TEARDOWN_CPU - 1));
+
        /* Invoke the former CPU_DYING callbacks */
-       for (; st->state > target; st->state--) {
-               ret = cpuhp_invoke_callback(cpu, st->state, false, NULL, NULL);
-               /*
-                * DYING must not fail!
-                */
-               WARN_ON_ONCE(ret);
-       }
+       ret = cpuhp_invoke_callback_range(false, cpu, st, target);
+
+       /*
+        * DYING must not fail!
+        */
+       WARN_ON_ONCE(ret);
 
        /* Give up timekeeping duties */
        tick_handover_do_timer();
@@ -975,27 +1017,22 @@ void cpuhp_report_idle_dead(void)
                                 cpuhp_complete_idle_dead, st, 0);
 }
 
-static void undo_cpu_down(unsigned int cpu, struct cpuhp_cpu_state *st)
-{
-       for (st->state++; st->state < st->target; st->state++)
-               cpuhp_invoke_callback(cpu, st->state, true, NULL, NULL);
-}
-
 static int cpuhp_down_callbacks(unsigned int cpu, struct cpuhp_cpu_state *st,
                                enum cpuhp_state target)
 {
        enum cpuhp_state prev_state = st->state;
        int ret = 0;
 
-       for (; st->state > target; st->state--) {
-               ret = cpuhp_invoke_callback(cpu, st->state, false, NULL, NULL);
-               if (ret) {
-                       st->target = prev_state;
-                       if (st->state < prev_state)
-                               undo_cpu_down(cpu, st);
-                       break;
-               }
+       ret = cpuhp_invoke_callback_range(false, cpu, st, target);
+       if (ret) {
+
+               cpuhp_reset_state(st, prev_state);
+
+               if (st->state < prev_state)
+                       WARN_ON(cpuhp_invoke_callback_range(true, cpu, st,
+                                                           prev_state));
        }
+
        return ret;
 }
 
@@ -1168,14 +1205,12 @@ void notify_cpu_starting(unsigned int cpu)
 
        rcu_cpu_starting(cpu);  /* Enables RCU usage on this CPU. */
        cpumask_set_cpu(cpu, &cpus_booted_once_mask);
-       while (st->state < target) {
-               st->state++;
-               ret = cpuhp_invoke_callback(cpu, st->state, true, NULL, NULL);
-               /*
-                * STARTING must not fail!
-                */
-               WARN_ON_ONCE(ret);
-       }
+       ret = cpuhp_invoke_callback_range(true, cpu, st, target);
+
+       /*
+        * STARTING must not fail!
+        */
+       WARN_ON_ONCE(ret);
 }
 
 /*
@@ -1781,8 +1816,7 @@ static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
         * If there's nothing to do, we done.
         * Relies on the union for multi_instance.
         */
-       if ((bringup && !sp->startup.single) ||
-           (!bringup && !sp->teardown.single))
+       if (cpuhp_step_empty(bringup, sp))
                return 0;
        /*
         * The non AP bound callbacks can fail on bringup. On teardown