[PATCH 00/11] Parallel CPU bringup for x86_64

[PATCH 00/11] Parallel CPU bringup for x86_64

[David Woodhouse]

Dusting off this patch series from February, in which we reduce the time 
taken for bringing up CPUs on a 96-way Skylake box from 500ms to about 
34ms.

There is more parallelism to be had here, including a 1:many TSC sync 
(or just *no* TSC sync, in the kexec case), and letting the APs all run 
through their own states from CPUHP_BRINGUP_CPU to CPUHP_AP_ONLINE_IDLE 
in parallel too. But I'll take a mere factor of 15 for the time being.

We can also have a careful look at the remaining time spent in the 
initial INIT/SIPI phase and see what we can shave off it.

David Woodhouse (10):
      x86/apic/x2apic: Fix parallel handling of cluster_mask
      rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion
      rcu: Add mutex for rcu boost kthread spawning and affinity setting
      cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU
      x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()
      x86/smpboot: Split up native_cpu_up into separate phases
      cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>
      x86/tsc: Avoid synchronizing TSCs with multiple CPUs in parallel
      x86/smp: Bring up secondary CPUs in parallel
      x86/kvm: Silence per-cpu pr_info noise about KVM clocks and steal time

Thomas Gleixner (1):
      x86/boot: Support parallel startup of secondary CPUs

 arch/x86/include/asm/realmode.h       |   3 +++
 arch/x86/include/asm/smp.h            |   9 ++++++-
 arch/x86/kernel/acpi/sleep.c          |   1 +
 arch/x86/kernel/apic/apic.c           |   2 +-
 arch/x86/kernel/apic/x2apic_cluster.c |  82 +++++++++++++++++++++++++++++++++++++-------------------------
 arch/x86/kernel/head_64.S             |  71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/kvm.c                 |   6 ++---
 arch/x86/kernel/kvmclock.c            |   2 +-
 arch/x86/kernel/smpboot.c             | 243 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
 arch/x86/kernel/tsc_sync.c            |   7 ++++++
 arch/x86/realmode/init.c              |   3 +++
 arch/x86/realmode/rm/trampoline_64.S  |  14 +++++++++++
 include/linux/cpuhotplug.h            |   2 ++
 include/linux/smpboot.h               |   7 ++++++
 kernel/cpu.c                          |  27 +++++++++++++++++++--
 kernel/rcu/tree.c                     |  65 ++++++++++++++++++++++++-------------------------
 kernel/rcu/tree.h                     |   7 +++---
 kernel/rcu/tree_plugin.h              |  10 ++++++--
 kernel/smpboot.c                      |   2 +-
 kernel/smpboot.h                      |   2 --
 20 files changed, 418 insertions(+), 147 deletions(-)

[PATCH 01/11] x86/apic/x2apic: Fix parallel handling of cluster_mask

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

For each CPU being brought up, the alloc_clustermask() function
allocates a new struct cluster_mask just in case it's needed. Then the
target CPU actually runs, and in init_x2apic_ldr() it either uses a
cluster_mask from a previous CPU in the same cluster, or consumes the
"spare" one and sets the global pointer to NULL.

That isn't going to parallelise stunningly well.

Ditch the global variable, let alloc_clustermask() install the struct
*directly* in the per_cpu data for the CPU being brought up. As an
optimisation, actually make it do so for *all* present CPUs in the same
cluster, which means only one iteration over for_each_present_cpu()
instead of doing so repeatedly, once for each CPU.

This was a harmless "bug" while CPU bringup wasn't actually happening in
parallel. It's about to become less harmless...

Fixes: 023a611748fd5 ("x86/apic/x2apic: Simplify cluster management")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/apic/x2apic_cluster.c | 82 ++++++++++++++++-----------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index e696e22d0531..4ff6a6005ad6 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -24,7 +24,6 @@ static u32 *x86_cpu_to_logical_apicid __read_mostly;
 
 static DEFINE_PER_CPU(cpumask_var_t, ipi_mask);
 static DEFINE_PER_CPU_READ_MOSTLY(struct cluster_mask *, cluster_masks);
-static struct cluster_mask *cluster_hotplug_mask;
 
 static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
@@ -106,54 +105,71 @@ static u32 x2apic_calc_apicid(unsigned int cpu)
 static void init_x2apic_ldr(void)
 {
 	struct cluster_mask *cmsk = this_cpu_read(cluster_masks);
-	u32 cluster, apicid = apic_read(APIC_LDR);
-	unsigned int cpu;
 
-	x86_cpu_to_logical_apicid[smp_processor_id()] = apicid;
+	BUG_ON(!cmsk);
 
-	if (cmsk)
-		goto update;
-
-	cluster = apicid >> 16;
-	for_each_online_cpu(cpu) {
-		cmsk = per_cpu(cluster_masks, cpu);
-		/* Matching cluster found. Link and update it. */
-		if (cmsk && cmsk->clusterid == cluster)
-			goto update;
-	}
-	cmsk = cluster_hotplug_mask;
-	cmsk->clusterid = cluster;
-	cluster_hotplug_mask = NULL;
-update:
-	this_cpu_write(cluster_masks, cmsk);
 	cpumask_set_cpu(smp_processor_id(), &cmsk->mask);
 }
 
-static int alloc_clustermask(unsigned int cpu, int node)
+static int alloc_clustermask(unsigned int cpu, u32 cluster, int node)
 {
+	struct cluster_mask *cmsk = NULL;
+	unsigned int cpu_i;
+	u32 apicid;
+
 	if (per_cpu(cluster_masks, cpu))
 		return 0;
-	/*
-	 * If a hotplug spare mask exists, check whether it's on the right
-	 * node. If not, free it and allocate a new one.
+
+	/* For the hotplug case, don't always allocate a new one */
+	for_each_present_cpu(cpu_i) {
+		apicid = apic->cpu_present_to_apicid(cpu_i);
+		if (apicid != BAD_APICID && apicid >> 4 == cluster) {
+			cmsk = per_cpu(cluster_masks, cpu_i);
+			if (cmsk)
+				break;
+		}
+	}
+	if (!cmsk) {
+		cmsk = kzalloc_node(sizeof(*cmsk), GFP_KERNEL, node);
+	}
+	if (!cmsk)
+		return -ENOMEM;
+
+	cmsk->node = node;
+	cmsk->clusterid = cluster;
+
+	per_cpu(cluster_masks, cpu) = cmsk;
+
+        /*
+	 * As an optimisation during boot, set the cluster_mask for *all*
+	 * present CPUs at once, to prevent *each* of them having to iterate
+	 * over the others to find the existing cluster_mask.
 	 */
-	if (cluster_hotplug_mask) {
-		if (cluster_hotplug_mask->node == node)
-			return 0;
-		kfree(cluster_hotplug_mask);
+	if (system_state < SYSTEM_RUNNING) {
+		for_each_present_cpu(cpu) {
+			u32 apicid = apic->cpu_present_to_apicid(cpu);
+			if (apicid != BAD_APICID && apicid >> 4 == cluster) {
+				struct cluster_mask **cpu_cmsk = &per_cpu(cluster_masks, cpu);
+				if (*cpu_cmsk)
+					BUG_ON(*cpu_cmsk != cmsk);
+				else
+					*cpu_cmsk = cmsk;
+			}
+		}
 	}
 
-	cluster_hotplug_mask = kzalloc_node(sizeof(*cluster_hotplug_mask),
-					    GFP_KERNEL, node);
-	if (!cluster_hotplug_mask)
-		return -ENOMEM;
-	cluster_hotplug_mask->node = node;
 	return 0;
 }
 
 static int x2apic_prepare_cpu(unsigned int cpu)
 {
-	if (alloc_clustermask(cpu, cpu_to_node(cpu)) < 0)
+	u32 phys_apicid = apic->cpu_present_to_apicid(cpu);
+	u32 cluster = phys_apicid >> 4;
+	u32 logical_apicid = (cluster << 16) | (1 << (phys_apicid & 0xf));
+
+	x86_cpu_to_logical_apicid[cpu] = logical_apicid;
+
+	if (alloc_clustermask(cpu, cluster, cpu_to_node(cpu)) < 0)
 		return -ENOMEM;
 	if (!zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL))
 		return -ENOMEM;
-- 
2.31.1

[PATCH 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

If we allow architectures to bring APs online in parallel, then we end
up requiring rcu_cpu_starting() to be reentrant. But currently, the
manipulation of rnp->ofl_seq is not thread-safe.

However, rnp->ofl_seq is also fairly much pointless anyway since both
rcu_cpu_starting() and rcu_report_dead() hold rcu_state.ofl_lock for
fairly much the whole time that rnp->ofl_seq is set to an odd number
to indicate that an operation is in progress.

So drop rnp->ofl_seq completely, and use only rcu_state.ofl_lock.

This has a couple of minor complexities: lockdep will complain when we
take rcu_state.ofl_lock, and currently accepts the 'excuse' of having
an odd value in rnp->ofl_seq. So switch it to an arch_spinlock_t to
avoid that false positive complaint. Since we're killing rnp->ofl_seq
of course that 'excuse' has to be changed too, so make it check for
arch_spin_is_locked(rcu_state.ofl_lock).

There's no arch_spin_lock_irqsave() so we have to manually save and
restore local interrupts around the locking.

At Paul's request, make rcu_gp_init not just wait but *exclude* any
CPU online/offline activity, which was fairly much true already by
virtue of it holding rcu_state.ofl_lock.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 kernel/rcu/tree.c | 64 +++++++++++++++++++++++------------------------
 kernel/rcu/tree.h |  4 +--
 2 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index ef8d36f580fc..a1bb0b1229ed 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -91,7 +91,7 @@ static struct rcu_state rcu_state = {
 	.abbr = RCU_ABBR,
 	.exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
 	.exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
-	.ofl_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.ofl_lock),
+	.ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
 };
 
 /* Dump rcu_node combining tree at boot to verify correct setup. */
@@ -1168,7 +1168,15 @@ bool rcu_lockdep_current_cpu_online(void)
 	preempt_disable_notrace();
 	rdp = this_cpu_ptr(&rcu_data);
 	rnp = rdp->mynode;
-	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
+	/*
+	 * Strictly, we care here about the case where the current CPU is
+	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
+	 * not being up to date. So arch_spin_is_locked() might have a
+	 * false positive if it's held by some *other* CPU, but that's
+	 * OK because that just means a false *negative* on the warning.
+	 */
+	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) ||
+	    arch_spin_is_locked(&rcu_state.ofl_lock))
 		ret = true;
 	preempt_enable_notrace();
 	return ret;
@@ -1731,7 +1739,6 @@ static void rcu_strict_gp_boundary(void *unused)
  */
 static noinline_for_stack bool rcu_gp_init(void)
 {
-	unsigned long firstseq;
 	unsigned long flags;
 	unsigned long oldmask;
 	unsigned long mask;
@@ -1774,22 +1781,17 @@ static noinline_for_stack bool rcu_gp_init(void)
 	 * of RCU's Requirements documentation.
 	 */
 	WRITE_ONCE(rcu_state.gp_state, RCU_GP_ONOFF);
+	/* Exclude CPU hotplug operations. */
 	rcu_for_each_leaf_node(rnp) {
-		// Wait for CPU-hotplug operations that might have
-		// started before this grace period did.
-		smp_mb(); // Pair with barriers used when updating ->ofl_seq to odd values.
-		firstseq = READ_ONCE(rnp->ofl_seq);
-		if (firstseq & 0x1)
-			while (firstseq == READ_ONCE(rnp->ofl_seq))
-				schedule_timeout_idle(1);  // Can't wake unless RCU is watching.
-		smp_mb(); // Pair with barriers used when updating ->ofl_seq to even values.
-		raw_spin_lock(&rcu_state.ofl_lock);
-		raw_spin_lock_irq_rcu_node(rnp);
+		local_irq_save(flags);
+		arch_spin_lock(&rcu_state.ofl_lock);
+		raw_spin_lock_rcu_node(rnp);
 		if (rnp->qsmaskinit == rnp->qsmaskinitnext &&
 		    !rnp->wait_blkd_tasks) {
 			/* Nothing to do on this leaf rcu_node structure. */
-			raw_spin_unlock_irq_rcu_node(rnp);
-			raw_spin_unlock(&rcu_state.ofl_lock);
+			raw_spin_unlock_rcu_node(rnp);
+			arch_spin_unlock(&rcu_state.ofl_lock);
+			local_irq_restore(flags);
 			continue;
 		}
 
@@ -1824,8 +1826,9 @@ static noinline_for_stack bool rcu_gp_init(void)
 				rcu_cleanup_dead_rnp(rnp);
 		}
 
-		raw_spin_unlock_irq_rcu_node(rnp);
-		raw_spin_unlock(&rcu_state.ofl_lock);
+		raw_spin_unlock_rcu_node(rnp);
+		arch_spin_unlock(&rcu_state.ofl_lock);
+		local_irq_restore(flags);
 	}
 	rcu_gp_slow(gp_preinit_delay); /* Races with CPU hotplug. */
 
@@ -4233,7 +4236,7 @@ int rcutree_offline_cpu(unsigned int cpu)
  */
 void rcu_cpu_starting(unsigned int cpu)
 {
-	unsigned long flags;
+	unsigned long flags, seq_flags;
 	unsigned long mask;
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
@@ -4246,11 +4249,11 @@ void rcu_cpu_starting(unsigned int cpu)
 
 	rnp = rdp->mynode;
 	mask = rdp->grpmask;
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
+	local_irq_save(seq_flags);
+	arch_spin_lock(&rcu_state.ofl_lock);
 	rcu_dynticks_eqs_online();
 	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	raw_spin_lock_irqsave_rcu_node(rnp, flags);
+	raw_spin_lock_rcu_node(rnp);
 	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
 	newcpu = !(rnp->expmaskinitnext & mask);
 	rnp->expmaskinitnext |= mask;
@@ -4269,9 +4272,8 @@ void rcu_cpu_starting(unsigned int cpu)
 	} else {
 		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 	}
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
+	arch_spin_unlock(&rcu_state.ofl_lock);
+	local_irq_restore(seq_flags);
 	smp_mb(); /* Ensure RCU read-side usage follows above initialization. */
 }
 
@@ -4285,7 +4287,7 @@ void rcu_cpu_starting(unsigned int cpu)
  */
 void rcu_report_dead(unsigned int cpu)
 {
-	unsigned long flags;
+	unsigned long flags, seq_flags;
 	unsigned long mask;
 	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
 	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
@@ -4299,10 +4301,8 @@ void rcu_report_dead(unsigned int cpu)
 
 	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
 	mask = rdp->grpmask;
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	raw_spin_lock(&rcu_state.ofl_lock);
+	local_irq_save(seq_flags);
+	arch_spin_lock(&rcu_state.ofl_lock);
 	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
 	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
 	rdp->rcu_ofl_gp_flags = READ_ONCE(rcu_state.gp_flags);
@@ -4313,10 +4313,8 @@ void rcu_report_dead(unsigned int cpu)
 	}
 	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
-	raw_spin_unlock(&rcu_state.ofl_lock);
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
+	arch_spin_unlock(&rcu_state.ofl_lock);
+	local_irq_restore(seq_flags);
 
 	rdp->cpu_started = false;
 }
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 305cf6aeb408..aff4cc9303fb 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -56,8 +56,6 @@ struct rcu_node {
 				/*  Initialized from ->qsmaskinitnext at the */
 				/*  beginning of each grace period. */
 	unsigned long qsmaskinitnext;
-	unsigned long ofl_seq;	/* CPU-hotplug operation sequence count. */
-				/* Online CPUs for next grace period. */
 	unsigned long expmask;	/* CPUs or groups that need to check in */
 				/*  to allow the current expedited GP */
 				/*  to complete. */
@@ -358,7 +356,7 @@ struct rcu_state {
 	const char *name;			/* Name of structure. */
 	char abbr;				/* Abbreviated name. */
 
-	raw_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
+	arch_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
 						/* Synchronize offline with */
 						/*  GP pre-initialization. */
 };
-- 
2.31.1

Re: [PATCH 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[Paul E. McKenney]

On Thu, Dec 09, 2021 at 03:09:29PM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If we allow architectures to bring APs online in parallel, then we end
> up requiring rcu_cpu_starting() to be reentrant. But currently, the
> manipulation of rnp->ofl_seq is not thread-safe.
> 
> However, rnp->ofl_seq is also fairly much pointless anyway since both
> rcu_cpu_starting() and rcu_report_dead() hold rcu_state.ofl_lock for
> fairly much the whole time that rnp->ofl_seq is set to an odd number
> to indicate that an operation is in progress.
> 
> So drop rnp->ofl_seq completely, and use only rcu_state.ofl_lock.
> 
> This has a couple of minor complexities: lockdep will complain when we
> take rcu_state.ofl_lock, and currently accepts the 'excuse' of having
> an odd value in rnp->ofl_seq. So switch it to an arch_spinlock_t to
> avoid that false positive complaint. Since we're killing rnp->ofl_seq
> of course that 'excuse' has to be changed too, so make it check for
> arch_spin_is_locked(rcu_state.ofl_lock).
> 
> There's no arch_spin_lock_irqsave() so we have to manually save and
> restore local interrupts around the locking.
> 
> At Paul's request, make rcu_gp_init not just wait but *exclude* any
> CPU online/offline activity, which was fairly much true already by
> virtue of it holding rcu_state.ofl_lock.

Looks good!

Could you please also make the first clause read something like this?

	"At Paul's request based on Neeraj's analysis, ..."

I am going to pull this into -rcu for more intensive testing of this
code for non-concurrent CPU-online operations (making the above change
to the commit log).  At some point, rcutorture needs to learn how to
do concurrent CPU-online operations, but it would be good to bake the
RCU-specific patches for a bit beforehand.

Depending on timing, you might wish to send this patch with the
rest of this group, so:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

If testing goes well and if you don't get it there first, I expect
to push this during the v5.18 merge window.

							Thanx, Paul

> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  kernel/rcu/tree.c | 64 +++++++++++++++++++++++------------------------
>  kernel/rcu/tree.h |  4 +--
>  2 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index ef8d36f580fc..a1bb0b1229ed 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -91,7 +91,7 @@ static struct rcu_state rcu_state = {
>  	.abbr = RCU_ABBR,
>  	.exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
>  	.exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
> -	.ofl_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.ofl_lock),
> +	.ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
>  };
>  
>  /* Dump rcu_node combining tree at boot to verify correct setup. */
> @@ -1168,7 +1168,15 @@ bool rcu_lockdep_current_cpu_online(void)
>  	preempt_disable_notrace();
>  	rdp = this_cpu_ptr(&rcu_data);
>  	rnp = rdp->mynode;
> -	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
> +	/*
> +	 * Strictly, we care here about the case where the current CPU is
> +	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
> +	 * not being up to date. So arch_spin_is_locked() might have a
> +	 * false positive if it's held by some *other* CPU, but that's
> +	 * OK because that just means a false *negative* on the warning.
> +	 */
> +	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) ||
> +	    arch_spin_is_locked(&rcu_state.ofl_lock))
>  		ret = true;
>  	preempt_enable_notrace();
>  	return ret;
> @@ -1731,7 +1739,6 @@ static void rcu_strict_gp_boundary(void *unused)
>   */
>  static noinline_for_stack bool rcu_gp_init(void)
>  {
> -	unsigned long firstseq;
>  	unsigned long flags;
>  	unsigned long oldmask;
>  	unsigned long mask;
> @@ -1774,22 +1781,17 @@ static noinline_for_stack bool rcu_gp_init(void)
>  	 * of RCU's Requirements documentation.
>  	 */
>  	WRITE_ONCE(rcu_state.gp_state, RCU_GP_ONOFF);
> +	/* Exclude CPU hotplug operations. */
>  	rcu_for_each_leaf_node(rnp) {
> -		// Wait for CPU-hotplug operations that might have
> -		// started before this grace period did.
> -		smp_mb(); // Pair with barriers used when updating ->ofl_seq to odd values.
> -		firstseq = READ_ONCE(rnp->ofl_seq);
> -		if (firstseq & 0x1)
> -			while (firstseq == READ_ONCE(rnp->ofl_seq))
> -				schedule_timeout_idle(1);  // Can't wake unless RCU is watching.
> -		smp_mb(); // Pair with barriers used when updating ->ofl_seq to even values.
> -		raw_spin_lock(&rcu_state.ofl_lock);
> -		raw_spin_lock_irq_rcu_node(rnp);
> +		local_irq_save(flags);
> +		arch_spin_lock(&rcu_state.ofl_lock);
> +		raw_spin_lock_rcu_node(rnp);
>  		if (rnp->qsmaskinit == rnp->qsmaskinitnext &&
>  		    !rnp->wait_blkd_tasks) {
>  			/* Nothing to do on this leaf rcu_node structure. */
> -			raw_spin_unlock_irq_rcu_node(rnp);
> -			raw_spin_unlock(&rcu_state.ofl_lock);
> +			raw_spin_unlock_rcu_node(rnp);
> +			arch_spin_unlock(&rcu_state.ofl_lock);
> +			local_irq_restore(flags);
>  			continue;
>  		}
>  
> @@ -1824,8 +1826,9 @@ static noinline_for_stack bool rcu_gp_init(void)
>  				rcu_cleanup_dead_rnp(rnp);
>  		}
>  
> -		raw_spin_unlock_irq_rcu_node(rnp);
> -		raw_spin_unlock(&rcu_state.ofl_lock);
> +		raw_spin_unlock_rcu_node(rnp);
> +		arch_spin_unlock(&rcu_state.ofl_lock);
> +		local_irq_restore(flags);
>  	}
>  	rcu_gp_slow(gp_preinit_delay); /* Races with CPU hotplug. */
>  
> @@ -4233,7 +4236,7 @@ int rcutree_offline_cpu(unsigned int cpu)
>   */
>  void rcu_cpu_starting(unsigned int cpu)
>  {
> -	unsigned long flags;
> +	unsigned long flags, seq_flags;
>  	unsigned long mask;
>  	struct rcu_data *rdp;
>  	struct rcu_node *rnp;
> @@ -4246,11 +4249,11 @@ void rcu_cpu_starting(unsigned int cpu)
>  
>  	rnp = rdp->mynode;
>  	mask = rdp->grpmask;
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> +	local_irq_save(seq_flags);
> +	arch_spin_lock(&rcu_state.ofl_lock);
>  	rcu_dynticks_eqs_online();
>  	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	raw_spin_lock_irqsave_rcu_node(rnp, flags);
> +	raw_spin_lock_rcu_node(rnp);
>  	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
>  	newcpu = !(rnp->expmaskinitnext & mask);
>  	rnp->expmaskinitnext |= mask;
> @@ -4269,9 +4272,8 @@ void rcu_cpu_starting(unsigned int cpu)
>  	} else {
>  		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  	}
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
> +	arch_spin_unlock(&rcu_state.ofl_lock);
> +	local_irq_restore(seq_flags);
>  	smp_mb(); /* Ensure RCU read-side usage follows above initialization. */
>  }
>  
> @@ -4285,7 +4287,7 @@ void rcu_cpu_starting(unsigned int cpu)
>   */
>  void rcu_report_dead(unsigned int cpu)
>  {
> -	unsigned long flags;
> +	unsigned long flags, seq_flags;
>  	unsigned long mask;
>  	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
>  	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
> @@ -4299,10 +4301,8 @@ void rcu_report_dead(unsigned int cpu)
>  
>  	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
>  	mask = rdp->grpmask;
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	raw_spin_lock(&rcu_state.ofl_lock);
> +	local_irq_save(seq_flags);
> +	arch_spin_lock(&rcu_state.ofl_lock);
>  	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
>  	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>  	rdp->rcu_ofl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> @@ -4313,10 +4313,8 @@ void rcu_report_dead(unsigned int cpu)
>  	}
>  	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> -	raw_spin_unlock(&rcu_state.ofl_lock);
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
> +	arch_spin_unlock(&rcu_state.ofl_lock);
> +	local_irq_restore(seq_flags);
>  
>  	rdp->cpu_started = false;
>  }
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 305cf6aeb408..aff4cc9303fb 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -56,8 +56,6 @@ struct rcu_node {
>  				/*  Initialized from ->qsmaskinitnext at the */
>  				/*  beginning of each grace period. */
>  	unsigned long qsmaskinitnext;
> -	unsigned long ofl_seq;	/* CPU-hotplug operation sequence count. */
> -				/* Online CPUs for next grace period. */
>  	unsigned long expmask;	/* CPUs or groups that need to check in */
>  				/*  to allow the current expedited GP */
>  				/*  to complete. */
> @@ -358,7 +356,7 @@ struct rcu_state {
>  	const char *name;			/* Name of structure. */
>  	char abbr;				/* Abbreviated name. */
>  
> -	raw_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
> +	arch_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
>  						/* Synchronize offline with */
>  						/*  GP pre-initialization. */
>  };
> -- 
> 2.31.1
> 

Re: [PATCH 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

On Thu, 2021-12-09 at 09:18 -0800, Paul E. McKenney wrote:
> On Thu, Dec 09, 2021 at 03:09:29PM +0000, David Woodhouse wrote:
> > From: David Woodhouse <
> > dwmw@amazon.co.uk
> > >
> > 
> > If we allow architectures to bring APs online in parallel, then we end
> > up requiring rcu_cpu_starting() to be reentrant. But currently, the
> > manipulation of rnp->ofl_seq is not thread-safe.
> > 
> > However, rnp->ofl_seq is also fairly much pointless anyway since both
> > rcu_cpu_starting() and rcu_report_dead() hold rcu_state.ofl_lock for
> > fairly much the whole time that rnp->ofl_seq is set to an odd number
> > to indicate that an operation is in progress.
> > 
> > So drop rnp->ofl_seq completely, and use only rcu_state.ofl_lock.
> > 
> > This has a couple of minor complexities: lockdep will complain when we
> > take rcu_state.ofl_lock, and currently accepts the 'excuse' of having
> > an odd value in rnp->ofl_seq. So switch it to an arch_spinlock_t to
> > avoid that false positive complaint. Since we're killing rnp->ofl_seq
> > of course that 'excuse' has to be changed too, so make it check for
> > arch_spin_is_locked(rcu_state.ofl_lock).
> > 
> > There's no arch_spin_lock_irqsave() so we have to manually save and
> > restore local interrupts around the locking.
> > 
> > At Paul's request, make rcu_gp_init not just wait but *exclude* any
> > CPU online/offline activity, which was fairly much true already by
> > virtue of it holding rcu_state.ofl_lock.
> 
> Looks good!
> 
> Could you please also make the first clause read something like this?
> 
> 	"At Paul's request based on Neeraj's analysis, ..."
> 
> I am going to pull this into -rcu for more intensive testing of this
> code for non-concurrent CPU-online operations (making the above change
> to the commit log).  At some point, rcutorture needs to learn how to
> do concurrent CPU-online operations, but it would be good to bake the
> RCU-specific patches for a bit beforehand.
> 
> Depending on timing, you might wish to send this patch with the
> rest of this group, so:
> 
> Acked-by: Paul E. McKenney <paulmck@kernel.org>
> 
> If testing goes well and if you don't get it there first, I expect
> to push this during the v5.18 merge window.


Thanks.

I think the best option is probably to let all the cleanups and fixes
get merged separately through whatever tree is most appropriate, and
then we can just merge that final patch to actually *enable* parallel
boot for x86 last of all.

I'll address Neeraj's feedback and repost this one.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[Neeraj Upadhyay]

Hi,

On 12/9/2021 8:39 PM, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If we allow architectures to bring APs online in parallel, then we end
> up requiring rcu_cpu_starting() to be reentrant. But currently, the
> manipulation of rnp->ofl_seq is not thread-safe.
> 
> However, rnp->ofl_seq is also fairly much pointless anyway since both
> rcu_cpu_starting() and rcu_report_dead() hold rcu_state.ofl_lock for
> fairly much the whole time that rnp->ofl_seq is set to an odd number
> to indicate that an operation is in progress.
> 
> So drop rnp->ofl_seq completely, and use only rcu_state.ofl_lock.
> 
> This has a couple of minor complexities: lockdep will complain when we
> take rcu_state.ofl_lock, and currently accepts the 'excuse' of having
> an odd value in rnp->ofl_seq. So switch it to an arch_spinlock_t to
> avoid that false positive complaint. Since we're killing rnp->ofl_seq
> of course that 'excuse' has to be changed too, so make it check for
> arch_spin_is_locked(rcu_state.ofl_lock).
> 
> There's no arch_spin_lock_irqsave() so we have to manually save and
> restore local interrupts around the locking.
> 
> At Paul's request, make rcu_gp_init not just wait but *exclude* any
> CPU online/offline activity, which was fairly much true already by
> virtue of it holding rcu_state.ofl_lock.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   kernel/rcu/tree.c | 64 +++++++++++++++++++++++------------------------
>   kernel/rcu/tree.h |  4 +--
>   2 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index ef8d36f580fc..a1bb0b1229ed 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -91,7 +91,7 @@ static struct rcu_state rcu_state = {
>   	.abbr = RCU_ABBR,
>   	.exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
>   	.exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
> -	.ofl_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.ofl_lock),
> +	.ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
>   };
>   
>   /* Dump rcu_node combining tree at boot to verify correct setup. */
> @@ -1168,7 +1168,15 @@ bool rcu_lockdep_current_cpu_online(void)
>   	preempt_disable_notrace();
>   	rdp = this_cpu_ptr(&rcu_data);
>   	rnp = rdp->mynode;
> -	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
> +	/*
> +	 * Strictly, we care here about the case where the current CPU is
> +	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
> +	 * not being up to date. So arch_spin_is_locked() might have a
> +	 * false positive if it's held by some *other* CPU, but that's
> +	 * OK because that just means a false *negative* on the warning.
> +	 */
> +	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) ||
> +	    arch_spin_is_locked(&rcu_state.ofl_lock))
>   		ret = true;
>   	preempt_enable_notrace();
>   	return ret;
> @@ -1731,7 +1739,6 @@ static void rcu_strict_gp_boundary(void *unused)
>    */
>   static noinline_for_stack bool rcu_gp_init(void)
>   {
> -	unsigned long firstseq;
>   	unsigned long flags;
>   	unsigned long oldmask;
>   	unsigned long mask;
> @@ -1774,22 +1781,17 @@ static noinline_for_stack bool rcu_gp_init(void)
>   	 * of RCU's Requirements documentation.
>   	 */
>   	WRITE_ONCE(rcu_state.gp_state, RCU_GP_ONOFF);
> +	/* Exclude CPU hotplug operations. */
>   	rcu_for_each_leaf_node(rnp) {
> -		// Wait for CPU-hotplug operations that might have
> -		// started before this grace period did.
> -		smp_mb(); // Pair with barriers used when updating ->ofl_seq to odd values.
> -		firstseq = READ_ONCE(rnp->ofl_seq);
> -		if (firstseq & 0x1)
> -			while (firstseq == READ_ONCE(rnp->ofl_seq))
> -				schedule_timeout_idle(1);  // Can't wake unless RCU is watching.
> -		smp_mb(); // Pair with barriers used when updating ->ofl_seq to even values.
> -		raw_spin_lock(&rcu_state.ofl_lock);
> -		raw_spin_lock_irq_rcu_node(rnp);
> +		local_irq_save(flags);
> +		arch_spin_lock(&rcu_state.ofl_lock);
> +		raw_spin_lock_rcu_node(rnp);
>   		if (rnp->qsmaskinit == rnp->qsmaskinitnext &&
>   		    !rnp->wait_blkd_tasks) {
>   			/* Nothing to do on this leaf rcu_node structure. */
> -			raw_spin_unlock_irq_rcu_node(rnp);
> -			raw_spin_unlock(&rcu_state.ofl_lock);
> +			raw_spin_unlock_rcu_node(rnp);
> +			arch_spin_unlock(&rcu_state.ofl_lock);
> +			local_irq_restore(flags);
>   			continue;
>   		}
>   
> @@ -1824,8 +1826,9 @@ static noinline_for_stack bool rcu_gp_init(void)
>   				rcu_cleanup_dead_rnp(rnp);
>   		}
>   
> -		raw_spin_unlock_irq_rcu_node(rnp);
> -		raw_spin_unlock(&rcu_state.ofl_lock);
> +		raw_spin_unlock_rcu_node(rnp);
> +		arch_spin_unlock(&rcu_state.ofl_lock);
> +		local_irq_restore(flags);
>   	}
>   	rcu_gp_slow(gp_preinit_delay); /* Races with CPU hotplug. */
>   
> @@ -4233,7 +4236,7 @@ int rcutree_offline_cpu(unsigned int cpu)
>    */
>   void rcu_cpu_starting(unsigned int cpu)
>   {
> -	unsigned long flags;
> +	unsigned long flags, seq_flags;
>   	unsigned long mask;
>   	struct rcu_data *rdp;
>   	struct rcu_node *rnp;
> @@ -4246,11 +4249,11 @@ void rcu_cpu_starting(unsigned int cpu)
>   
>   	rnp = rdp->mynode;
>   	mask = rdp->grpmask;
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> +	local_irq_save(seq_flags);
> +	arch_spin_lock(&rcu_state.ofl_lock);
>   	rcu_dynticks_eqs_online();
>   	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().

Can we drop this smp_mb(),as arch_spin_lock(&rcu_state.ofl_lock) 
provides the ordering now?

> -	raw_spin_lock_irqsave_rcu_node(rnp, flags);
> +	raw_spin_lock_rcu_node(rnp);
>   	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
>   	newcpu = !(rnp->expmaskinitnext & mask);
>   	rnp->expmaskinitnext |= mask;
> @@ -4269,9 +4272,8 @@ void rcu_cpu_starting(unsigned int cpu)
>   	} else {
>   		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);

'flags' is uninitialized now?



Thanks
Neeraj

>   	}
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
> +	arch_spin_unlock(&rcu_state.ofl_lock);
> +	local_irq_restore(seq_flags);
>   	smp_mb(); /* Ensure RCU read-side usage follows above initialization. */
>   }
>   
> @@ -4285,7 +4287,7 @@ void rcu_cpu_starting(unsigned int cpu)
>    */
>   void rcu_report_dead(unsigned int cpu)
>   {
> -	unsigned long flags;
> +	unsigned long flags, seq_flags;
>   	unsigned long mask;
>   	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
>   	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
> @@ -4299,10 +4301,8 @@ void rcu_report_dead(unsigned int cpu)
>   
>   	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
>   	mask = rdp->grpmask;
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	raw_spin_lock(&rcu_state.ofl_lock);
> +	local_irq_save(seq_flags);
> +	arch_spin_lock(&rcu_state.ofl_lock);
>   	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
>   	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>   	rdp->rcu_ofl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> @@ -4313,10 +4313,8 @@ void rcu_report_dead(unsigned int cpu)
>   	}
>   	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);
>   	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> -	raw_spin_unlock(&rcu_state.ofl_lock);
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
> +	arch_spin_unlock(&rcu_state.ofl_lock);
> +	local_irq_restore(seq_flags);
>   
>   	rdp->cpu_started = false;
>   }
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 305cf6aeb408..aff4cc9303fb 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -56,8 +56,6 @@ struct rcu_node {
>   				/*  Initialized from ->qsmaskinitnext at the */
>   				/*  beginning of each grace period. */
>   	unsigned long qsmaskinitnext;
> -	unsigned long ofl_seq;	/* CPU-hotplug operation sequence count. */
> -				/* Online CPUs for next grace period. */
>   	unsigned long expmask;	/* CPUs or groups that need to check in */
>   				/*  to allow the current expedited GP */
>   				/*  to complete. */
> @@ -358,7 +356,7 @@ struct rcu_state {
>   	const char *name;			/* Name of structure. */
>   	char abbr;				/* Abbreviated name. */
>   
> -	raw_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
> +	arch_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
>   						/* Synchronize offline with */
>   						/*  GP pre-initialization. */
>   };
> 

Re: [PATCH 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On Fri, 2021-12-10 at 00:01 +0530, Neeraj Upadhyay wrote:
> > @@ -4246,11 +4249,11 @@ void rcu_cpu_starting(unsigned int cpu)
> >    
> >        rnp = rdp->mynode;
> >        mask = rdp->grpmask;
> > -     WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> > -     WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> > +     local_irq_save(seq_flags);
> > +     arch_spin_lock(&rcu_state.ofl_lock);
> >        rcu_dynticks_eqs_online();
> >        smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> 
> Can we drop this smp_mb(),as arch_spin_lock(&rcu_state.ofl_lock) 
> provides the ordering now?

Yes, thanks.

> > -     raw_spin_lock_irqsave_rcu_node(rnp, flags);
> > +     raw_spin_lock_rcu_node(rnp);
> >        WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
> >        newcpu = !(rnp->expmaskinitnext & mask);
> >        rnp->expmaskinitnext |= mask;
> > @@ -4269,9 +4272,8 @@ void rcu_cpu_starting(unsigned int cpu)
> >        } else {
> >                raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> 
> 'flags' is uninitialized now?

Ah yes, that suffered from the fact that saving the flags is pointless
because we know we already disabled interrupts... but
rcu_report_qs_rnp() *really* wants to be given some flags to restore.

Will fix that too; thanks again.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

[PATCH v1.1 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 8650 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

If we allow architectures to bring APs online in parallel, then we end
up requiring rcu_cpu_starting() to be reentrant. But currently, the
manipulation of rnp->ofl_seq is not thread-safe.

However, rnp->ofl_seq is also fairly much pointless anyway since both
rcu_cpu_starting() and rcu_report_dead() hold rcu_state.ofl_lock for
fairly much the whole time that rnp->ofl_seq is set to an odd number
to indicate that an operation is in progress.

So drop rnp->ofl_seq completely, and use only rcu_state.ofl_lock.

This has a couple of minor complexities: lockdep will complain when we
take rcu_state.ofl_lock, and currently accepts the 'excuse' of having
an odd value in rnp->ofl_seq. So switch it to an arch_spinlock_t to
avoid that false positive complaint. Since we're killing rnp->ofl_seq
of course that 'excuse' has to be changed too, so make it check for
arch_spin_is_locked(rcu_state.ofl_lock).

There's no arch_spin_lock_irqsave() so we have to manually save and
restore local interrupts around the locking.

At Paul's request based on Neeraj's analysis, make rcu_gp_init not just 
wait but *exclude* any CPU online/offline activity, which was fairly
much true already by virtue of it holding rcu_state.ofl_lock.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
If we're going to split the series up and let the various patches take
their own paths to Linus, I'll just repost this one alone as 'v1.1'.

 kernel/rcu/tree.c | 71 ++++++++++++++++++++++++-----------------------
 kernel/rcu/tree.h |  4 +--
 2 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index ef8d36f580fc..2e1ae611be98 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -91,7 +91,7 @@ static struct rcu_state rcu_state = {
 	.abbr = RCU_ABBR,
 	.exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
 	.exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
-	.ofl_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.ofl_lock),
+	.ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
 };
 
 /* Dump rcu_node combining tree at boot to verify correct setup. */
@@ -1168,7 +1168,15 @@ bool rcu_lockdep_current_cpu_online(void)
 	preempt_disable_notrace();
 	rdp = this_cpu_ptr(&rcu_data);
 	rnp = rdp->mynode;
-	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
+	/*
+	 * Strictly, we care here about the case where the current CPU is
+	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
+	 * not being up to date. So arch_spin_is_locked() might have a
+	 * false positive if it's held by some *other* CPU, but that's
+	 * OK because that just means a false *negative* on the warning.
+	 */
+	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) ||
+	    arch_spin_is_locked(&rcu_state.ofl_lock))
 		ret = true;
 	preempt_enable_notrace();
 	return ret;
@@ -1731,7 +1739,6 @@ static void rcu_strict_gp_boundary(void *unused)
  */
 static noinline_for_stack bool rcu_gp_init(void)
 {
-	unsigned long firstseq;
 	unsigned long flags;
 	unsigned long oldmask;
 	unsigned long mask;
@@ -1774,22 +1781,17 @@ static noinline_for_stack bool rcu_gp_init(void)
 	 * of RCU's Requirements documentation.
 	 */
 	WRITE_ONCE(rcu_state.gp_state, RCU_GP_ONOFF);
+	/* Exclude CPU hotplug operations. */
 	rcu_for_each_leaf_node(rnp) {
-		// Wait for CPU-hotplug operations that might have
-		// started before this grace period did.
-		smp_mb(); // Pair with barriers used when updating ->ofl_seq to odd values.
-		firstseq = READ_ONCE(rnp->ofl_seq);
-		if (firstseq & 0x1)
-			while (firstseq == READ_ONCE(rnp->ofl_seq))
-				schedule_timeout_idle(1);  // Can't wake unless RCU is watching.
-		smp_mb(); // Pair with barriers used when updating ->ofl_seq to even values.
-		raw_spin_lock(&rcu_state.ofl_lock);
-		raw_spin_lock_irq_rcu_node(rnp);
+		local_irq_save(flags);
+		arch_spin_lock(&rcu_state.ofl_lock);
+		raw_spin_lock_rcu_node(rnp);
 		if (rnp->qsmaskinit == rnp->qsmaskinitnext &&
 		    !rnp->wait_blkd_tasks) {
 			/* Nothing to do on this leaf rcu_node structure. */
-			raw_spin_unlock_irq_rcu_node(rnp);
-			raw_spin_unlock(&rcu_state.ofl_lock);
+			raw_spin_unlock_rcu_node(rnp);
+			arch_spin_unlock(&rcu_state.ofl_lock);
+			local_irq_restore(flags);
 			continue;
 		}
 
@@ -1824,8 +1826,9 @@ static noinline_for_stack bool rcu_gp_init(void)
 				rcu_cleanup_dead_rnp(rnp);
 		}
 
-		raw_spin_unlock_irq_rcu_node(rnp);
-		raw_spin_unlock(&rcu_state.ofl_lock);
+		raw_spin_unlock_rcu_node(rnp);
+		arch_spin_unlock(&rcu_state.ofl_lock);
+		local_irq_restore(flags);
 	}
 	rcu_gp_slow(gp_preinit_delay); /* Races with CPU hotplug. */
 
@@ -4246,11 +4249,10 @@ void rcu_cpu_starting(unsigned int cpu)
 
 	rnp = rdp->mynode;
 	mask = rdp->grpmask;
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
+	local_irq_save(flags);
+	arch_spin_lock(&rcu_state.ofl_lock);
 	rcu_dynticks_eqs_online();
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	raw_spin_lock_irqsave_rcu_node(rnp, flags);
+	raw_spin_lock_rcu_node(rnp);
 	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
 	newcpu = !(rnp->expmaskinitnext & mask);
 	rnp->expmaskinitnext |= mask;
@@ -4263,15 +4265,18 @@ void rcu_cpu_starting(unsigned int cpu)
 
 	/* An incoming CPU should never be blocking a grace period. */
 	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
+		/* rcu_report_qs_rnp() *really* wants some flags to restore */
+		unsigned long flags2;
+		local_irq_save(flags2);
+
 		rcu_disable_urgency_upon_qs(rdp);
 		/* Report QS -after- changing ->qsmaskinitnext! */
-		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
+		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags2);
 	} else {
-		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
+		raw_spin_unlock_rcu_node(rnp);
 	}
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
+	arch_spin_unlock(&rcu_state.ofl_lock);
+	local_irq_restore(flags);
 	smp_mb(); /* Ensure RCU read-side usage follows above initialization. */
 }
 
@@ -4285,7 +4290,7 @@ void rcu_cpu_starting(unsigned int cpu)
  */
 void rcu_report_dead(unsigned int cpu)
 {
-	unsigned long flags;
+	unsigned long flags, seq_flags;
 	unsigned long mask;
 	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
 	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
@@ -4299,10 +4304,8 @@ void rcu_report_dead(unsigned int cpu)
 
 	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
 	mask = rdp->grpmask;
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	raw_spin_lock(&rcu_state.ofl_lock);
+	local_irq_save(seq_flags);
+	arch_spin_lock(&rcu_state.ofl_lock);
 	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
 	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
 	rdp->rcu_ofl_gp_flags = READ_ONCE(rcu_state.gp_flags);
@@ -4313,10 +4316,8 @@ void rcu_report_dead(unsigned int cpu)
 	}
 	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
-	raw_spin_unlock(&rcu_state.ofl_lock);
-	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
-	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
-	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
+	arch_spin_unlock(&rcu_state.ofl_lock);
+	local_irq_restore(seq_flags);
 
 	rdp->cpu_started = false;
 }
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 305cf6aeb408..aff4cc9303fb 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -56,8 +56,6 @@ struct rcu_node {
 				/*  Initialized from ->qsmaskinitnext at the */
 				/*  beginning of each grace period. */
 	unsigned long qsmaskinitnext;
-	unsigned long ofl_seq;	/* CPU-hotplug operation sequence count. */
-				/* Online CPUs for next grace period. */
 	unsigned long expmask;	/* CPUs or groups that need to check in */
 				/*  to allow the current expedited GP */
 				/*  to complete. */
@@ -358,7 +356,7 @@ struct rcu_state {
 	const char *name;			/* Name of structure. */
 	char abbr;				/* Abbreviated name. */
 
-	raw_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
+	arch_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
 						/* Synchronize offline with */
 						/*  GP pre-initialization. */
 };
-- 
2.31.1


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH v1.1 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[Neeraj Upadhyay]

Hi David,

Few minor comments

On 12/10/2021 12:51 AM, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If we allow architectures to bring APs online in parallel, then we end
> up requiring rcu_cpu_starting() to be reentrant. But currently, the
> manipulation of rnp->ofl_seq is not thread-safe.
> 
> However, rnp->ofl_seq is also fairly much pointless anyway since both
> rcu_cpu_starting() and rcu_report_dead() hold rcu_state.ofl_lock for
> fairly much the whole time that rnp->ofl_seq is set to an odd number
> to indicate that an operation is in progress.
> 
> So drop rnp->ofl_seq completely, and use only rcu_state.ofl_lock.
> 
> This has a couple of minor complexities: lockdep will complain when we
> take rcu_state.ofl_lock, and currently accepts the 'excuse' of having
> an odd value in rnp->ofl_seq. So switch it to an arch_spinlock_t to
> avoid that false positive complaint. Since we're killing rnp->ofl_seq
> of course that 'excuse' has to be changed too, so make it check for
> arch_spin_is_locked(rcu_state.ofl_lock).
> 
> There's no arch_spin_lock_irqsave() so we have to manually save and
> restore local interrupts around the locking.
> 
> At Paul's request based on Neeraj's analysis, make rcu_gp_init not just
> wait but *exclude* any CPU online/offline activity, which was fairly
> much true already by virtue of it holding rcu_state.ofl_lock.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> If we're going to split the series up and let the various patches take
> their own paths to Linus, I'll just repost this one alone as 'v1.1'.
> 
>   kernel/rcu/tree.c | 71 ++++++++++++++++++++++++-----------------------
>   kernel/rcu/tree.h |  4 +--
>   2 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index ef8d36f580fc..2e1ae611be98 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -91,7 +91,7 @@ static struct rcu_state rcu_state = {
>   	.abbr = RCU_ABBR,
>   	.exp_mutex = __MUTEX_INITIALIZER(rcu_state.exp_mutex),
>   	.exp_wake_mutex = __MUTEX_INITIALIZER(rcu_state.exp_wake_mutex),
> -	.ofl_lock = __RAW_SPIN_LOCK_UNLOCKED(rcu_state.ofl_lock),
> +	.ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
>   };
>   
>   /* Dump rcu_node combining tree at boot to verify correct setup. */
> @@ -1168,7 +1168,15 @@ bool rcu_lockdep_current_cpu_online(void)
>   	preempt_disable_notrace();
>   	rdp = this_cpu_ptr(&rcu_data);
>   	rnp = rdp->mynode;
> -	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
> +	/*
> +	 * Strictly, we care here about the case where the current CPU is
> +	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
> +	 * not being up to date. So arch_spin_is_locked() might have a

Minor:

Is this comment right - "thus has an excuse for rdp->grpmask not being 
up to date"; shouldn't it be "thus has an excuse for rnp->qsmaskinitnext 
not being up to date"?

Also, arch_spin_is_locked() also handles the rcu_report_dead() case,
where raw_spin_unlock_irqrestore_rcu_node() can have a rcu_read_lock 
from lockdep path with CPU bits already cleared from rnp->qsmaskinitnext?



> +	 * false positive if it's held by some *other* CPU, but that's
> +	 * OK because that just means a false *negative* on the warning.
> +	 */
> +	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) ||
> +	    arch_spin_is_locked(&rcu_state.ofl_lock))
>   		ret = true;
>   	preempt_enable_notrace();
>   	return ret;
> @@ -1731,7 +1739,6 @@ static void rcu_strict_gp_boundary(void *unused)
>    */
>   static noinline_for_stack bool rcu_gp_init(void)
>   {
> -	unsigned long firstseq;
>   	unsigned long flags;
>   	unsigned long oldmask;
>   	unsigned long mask;
> @@ -1774,22 +1781,17 @@ static noinline_for_stack bool rcu_gp_init(void)
>   	 * of RCU's Requirements documentation.
>   	 */
>   	WRITE_ONCE(rcu_state.gp_state, RCU_GP_ONOFF);
> +	/* Exclude CPU hotplug operations. */
>   	rcu_for_each_leaf_node(rnp) {
> -		// Wait for CPU-hotplug operations that might have
> -		// started before this grace period did.
> -		smp_mb(); // Pair with barriers used when updating ->ofl_seq to odd values.
> -		firstseq = READ_ONCE(rnp->ofl_seq);
> -		if (firstseq & 0x1)
> -			while (firstseq == READ_ONCE(rnp->ofl_seq))
> -				schedule_timeout_idle(1);  // Can't wake unless RCU is watching.
> -		smp_mb(); // Pair with barriers used when updating ->ofl_seq to even values.
> -		raw_spin_lock(&rcu_state.ofl_lock);
> -		raw_spin_lock_irq_rcu_node(rnp);
> +		local_irq_save(flags);
> +		arch_spin_lock(&rcu_state.ofl_lock);
> +		raw_spin_lock_rcu_node(rnp);
>   		if (rnp->qsmaskinit == rnp->qsmaskinitnext &&
>   		    !rnp->wait_blkd_tasks) {
>   			/* Nothing to do on this leaf rcu_node structure. */
> -			raw_spin_unlock_irq_rcu_node(rnp);
> -			raw_spin_unlock(&rcu_state.ofl_lock);
> +			raw_spin_unlock_rcu_node(rnp);
> +			arch_spin_unlock(&rcu_state.ofl_lock);
> +			local_irq_restore(flags);
>   			continue;
>   		}
>   
> @@ -1824,8 +1826,9 @@ static noinline_for_stack bool rcu_gp_init(void)
>   				rcu_cleanup_dead_rnp(rnp);
>   		}
>   
> -		raw_spin_unlock_irq_rcu_node(rnp);
> -		raw_spin_unlock(&rcu_state.ofl_lock);
> +		raw_spin_unlock_rcu_node(rnp);
> +		arch_spin_unlock(&rcu_state.ofl_lock);
> +		local_irq_restore(flags);
>   	}
>   	rcu_gp_slow(gp_preinit_delay); /* Races with CPU hotplug. */
>   
> @@ -4246,11 +4249,10 @@ void rcu_cpu_starting(unsigned int cpu)
>   
>   	rnp = rdp->mynode;
>   	mask = rdp->grpmask;
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> +	local_irq_save(flags);
> +	arch_spin_lock(&rcu_state.ofl_lock);
>   	rcu_dynticks_eqs_online();
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	raw_spin_lock_irqsave_rcu_node(rnp, flags);
> +	raw_spin_lock_rcu_node(rnp);
>   	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext | mask);
>   	newcpu = !(rnp->expmaskinitnext & mask);
>   	rnp->expmaskinitnext |= mask;
> @@ -4263,15 +4265,18 @@ void rcu_cpu_starting(unsigned int cpu)
>   
>   	/* An incoming CPU should never be blocking a grace period. */
>   	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
> +		/* rcu_report_qs_rnp() *really* wants some flags to restore */
> +		unsigned long flags2;

Minor: checkpatch flags it "Missing a blank line after declarations"



Thanks
Neeraj

> +		local_irq_save(flags2);
> +
>   		rcu_disable_urgency_upon_qs(rdp);
>   		/* Report QS -after- changing ->qsmaskinitnext! */
> -		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
> +		rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags2);
>   	} else {
> -		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> +		raw_spin_unlock_rcu_node(rnp);
>   	}
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
> +	arch_spin_unlock(&rcu_state.ofl_lock);
> +	local_irq_restore(flags);
>   	smp_mb(); /* Ensure RCU read-side usage follows above initialization. */
>   }
>   
> @@ -4285,7 +4290,7 @@ void rcu_cpu_starting(unsigned int cpu)
>    */
>   void rcu_report_dead(unsigned int cpu)
>   {
> -	unsigned long flags;
> +	unsigned long flags, seq_flags;
>   	unsigned long mask;
>   	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
>   	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
> @@ -4299,10 +4304,8 @@ void rcu_report_dead(unsigned int cpu)
>   
>   	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
>   	mask = rdp->grpmask;
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(!(rnp->ofl_seq & 0x1));
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	raw_spin_lock(&rcu_state.ofl_lock);
> +	local_irq_save(seq_flags);
> +	arch_spin_lock(&rcu_state.ofl_lock);
>   	raw_spin_lock_irqsave_rcu_node(rnp, flags); /* Enforce GP memory-order guarantee. */
>   	rdp->rcu_ofl_gp_seq = READ_ONCE(rcu_state.gp_seq);
>   	rdp->rcu_ofl_gp_flags = READ_ONCE(rcu_state.gp_flags);
> @@ -4313,10 +4316,8 @@ void rcu_report_dead(unsigned int cpu)
>   	}
>   	WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);
>   	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> -	raw_spin_unlock(&rcu_state.ofl_lock);
> -	smp_mb(); // Pair with rcu_gp_cleanup()'s ->ofl_seq barrier().
> -	WRITE_ONCE(rnp->ofl_seq, rnp->ofl_seq + 1);
> -	WARN_ON_ONCE(rnp->ofl_seq & 0x1);
> +	arch_spin_unlock(&rcu_state.ofl_lock);
> +	local_irq_restore(seq_flags);
>   
>   	rdp->cpu_started = false;
>   }
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 305cf6aeb408..aff4cc9303fb 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -56,8 +56,6 @@ struct rcu_node {
>   				/*  Initialized from ->qsmaskinitnext at the */
>   				/*  beginning of each grace period. */
>   	unsigned long qsmaskinitnext;
> -	unsigned long ofl_seq;	/* CPU-hotplug operation sequence count. */
> -				/* Online CPUs for next grace period. */
>   	unsigned long expmask;	/* CPUs or groups that need to check in */
>   				/*  to allow the current expedited GP */
>   				/*  to complete. */
> @@ -358,7 +356,7 @@ struct rcu_state {
>   	const char *name;			/* Name of structure. */
>   	char abbr;				/* Abbreviated name. */
>   
> -	raw_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
> +	arch_spinlock_t ofl_lock ____cacheline_internodealigned_in_smp;
>   						/* Synchronize offline with */
>   						/*  GP pre-initialization. */
>   };
> 

Re: [PATCH v1.1 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]

On Fri, 2021-12-10 at 09:56 +0530, Neeraj Upadhyay wrote:
> > -	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
> > +	/*
> > +	 * Strictly, we care here about the case where the current CPU is
> > +	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
> > +	 * not being up to date. So arch_spin_is_locked() might have a
> 
> Minor:
> 
> Is this comment right - "thus has an excuse for rdp->grpmask not being 
> up to date"; shouldn't it be "thus has an excuse for rnp->qsmaskinitnext 
> not being up to date"?
> 
> Also, arch_spin_is_locked() also handles the rcu_report_dead() case,
> where raw_spin_unlock_irqrestore_rcu_node() can have a rcu_read_lock 
> from lockdep path with CPU bits already cleared from rnp->qsmaskinitnext?

Good point; thanks. How's this:

	/*
	 * Strictly, we care here about the case where the current CPU is in
	 * rcu_cpu_starting() or rcu_report_dead() and thus has an excuse for
	 * rdp->qsmaskinitnext not being up to date. So arch_spin_is_locked()
	 * might have a false positive if it's held by some *other* CPU, but
	 * that's OK because that just means a false *negative* on the
	 * warning.
	 */

> >  	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
> > +		/* rcu_report_qs_rnp() *really* wants some flags to restore */
> > +		unsigned long flags2;
> 
> Minor: checkpatch flags it "Missing a blank line after declarations"

Ack. Also fixed and pushed out to my parallel-5.16 branch at 
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.16

This commit is probably the only one that's strictly needed for that
parallel bringup, but for now I've kept my rcu boost thread mutex
patch, and added your two patches (with minor whitespace fixes). I
think the best option is to let Paul handle them all.

We'll do the final step of actually *enabling* the parallel bringup on
any given architecture only after the various fixes have made their way
in and we've done a proper review of the remaining code paths.



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH v1.1 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl_lock for exclusion

[Neeraj Upadhyay]

On 12/13/2021 2:27 PM, David Woodhouse wrote:
> On Fri, 2021-12-10 at 09:56 +0530, Neeraj Upadhyay wrote:
>>> -	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
>>> +	/*
>>> +	 * Strictly, we care here about the case where the current CPU is
>>> +	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
>>> +	 * not being up to date. So arch_spin_is_locked() might have a
>>
>> Minor:
>>
>> Is this comment right - "thus has an excuse for rdp->grpmask not being
>> up to date"; shouldn't it be "thus has an excuse for rnp->qsmaskinitnext
>> not being up to date"?
>>
>> Also, arch_spin_is_locked() also handles the rcu_report_dead() case,
>> where raw_spin_unlock_irqrestore_rcu_node() can have a rcu_read_lock
>> from lockdep path with CPU bits already cleared from rnp->qsmaskinitnext?
> 
> Good point; thanks. How's this:
> 
> 	/*
> 	 * Strictly, we care here about the case where the current CPU is in
> 	 * rcu_cpu_starting() or rcu_report_dead() and thus has an excuse for
> 	 * rdp->qsmaskinitnext not being up to date. So arch_spin_is_locked()
> 	 * might have a false positive if it's held by some *other* CPU, but
> 	 * that's OK because that just means a false *negative* on the
> 	 * warning.
> 	 */
> 

Looks good to me!



>>>   	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
>>> +		/* rcu_report_qs_rnp() *really* wants some flags to restore */
>>> +		unsigned long flags2;
>>
>> Minor: checkpatch flags it "Missing a blank line after declarations"
> 
> Ack. Also fixed and pushed out to my parallel-5.16 branch at
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.16
> > This commit is probably the only one that's strictly needed for that
> parallel bringup, but for now I've kept my rcu boost thread mutex
> patch, and added your two patches (with minor whitespace fixes). I
> think the best option is to let Paul handle them all.
> 

Thanks; the 4 RCU specific patches in that tree looks good to me.


Thanks
Neeraj

> We'll do the final step of actually *enabling* the parallel bringup on
> any given architecture only after the various fixes have made their way
> in and we've done a proper review of the remaining code paths.
> 
> 

[PATCH 03/11] rcu: Add mutex for rcu boost kthread spawning and affinity setting

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

As we handle parallel CPU bringup, we will need to take care to avoid
spawning multiple boost threads, or race conditions when setting their
affinity. Spotted by Paul McKenney.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 kernel/rcu/tree.c        |  1 +
 kernel/rcu/tree.h        |  3 +++
 kernel/rcu/tree_plugin.h | 10 ++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index a1bb0b1229ed..809855474b39 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -4527,6 +4527,7 @@ static void __init rcu_init_one(void)
 			init_waitqueue_head(&rnp->exp_wq[2]);
 			init_waitqueue_head(&rnp->exp_wq[3]);
 			spin_lock_init(&rnp->exp_lock);
+			mutex_init(&rnp->boost_kthread_mutex);
 		}
 	}
 
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index aff4cc9303fb..055e30b3e5e0 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -108,6 +108,9 @@ struct rcu_node {
 				/*  side effect, not as a lock. */
 	unsigned long boost_time;
 				/* When to start boosting (jiffies). */
+	struct mutex boost_kthread_mutex;
+				/* Exclusion for thread spawning and affinity */
+				/*  manipulation. */
 	struct task_struct *boost_kthread_task;
 				/* kthread that takes care of priority */
 				/*  boosting for this rcu_node structure. */
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 5199559fbbf0..3b4ee0933710 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1162,15 +1162,16 @@ static void rcu_spawn_one_boost_kthread(struct rcu_node *rnp)
 	struct sched_param sp;
 	struct task_struct *t;
 
+	mutex_lock(&rnp->boost_kthread_mutex);
 	if (rnp->boost_kthread_task || !rcu_scheduler_fully_active)
-		return;
+		goto out;
 
 	rcu_state.boost = 1;
 
 	t = kthread_create(rcu_boost_kthread, (void *)rnp,
 			   "rcub/%d", rnp_index);
 	if (WARN_ON_ONCE(IS_ERR(t)))
-		return;
+		goto out;
 
 	raw_spin_lock_irqsave_rcu_node(rnp, flags);
 	rnp->boost_kthread_task = t;
@@ -1178,6 +1179,9 @@ static void rcu_spawn_one_boost_kthread(struct rcu_node *rnp)
 	sp.sched_priority = kthread_prio;
 	sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
 	wake_up_process(t); /* get to TASK_INTERRUPTIBLE quickly. */
+
+ out:
+	mutex_unlock(&rnp->boost_kthread_mutex);
 }
 
 /*
@@ -1200,6 +1204,7 @@ static void rcu_boost_kthread_setaffinity(struct rcu_node *rnp, int outgoingcpu)
 		return;
 	if (!zalloc_cpumask_var(&cm, GFP_KERNEL))
 		return;
+	mutex_lock(&rnp->boost_kthread_mutex);
 	for_each_leaf_node_possible_cpu(rnp, cpu)
 		if ((mask & leaf_node_cpu_bit(rnp, cpu)) &&
 		    cpu != outgoingcpu)
@@ -1207,6 +1212,7 @@ static void rcu_boost_kthread_setaffinity(struct rcu_node *rnp, int outgoingcpu)
 	if (cpumask_weight(cm) == 0)
 		cpumask_setall(cm);
 	set_cpus_allowed_ptr(t, cm);
+	mutex_unlock(&rnp->boost_kthread_mutex);
 	free_cpumask_var(cm);
 }
 
-- 
2.31.1

Re: [PATCH 03/11] rcu: Add mutex for rcu boost kthread spawning and affinity setting

[Paul E. McKenney]

On Thu, Dec 09, 2021 at 03:09:30PM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> As we handle parallel CPU bringup, we will need to take care to avoid
> spawning multiple boost threads, or race conditions when setting their
> affinity. Spotted by Paul McKenney.

And again, if testing goes well and you don't get it there first, I
expect to push this during the v5.18 merge window.  In case you
would like to push this with the rest of this series:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  kernel/rcu/tree.c        |  1 +
>  kernel/rcu/tree.h        |  3 +++
>  kernel/rcu/tree_plugin.h | 10 ++++++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index a1bb0b1229ed..809855474b39 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4527,6 +4527,7 @@ static void __init rcu_init_one(void)
>  			init_waitqueue_head(&rnp->exp_wq[2]);
>  			init_waitqueue_head(&rnp->exp_wq[3]);
>  			spin_lock_init(&rnp->exp_lock);
> +			mutex_init(&rnp->boost_kthread_mutex);
>  		}
>  	}
>  
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index aff4cc9303fb..055e30b3e5e0 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -108,6 +108,9 @@ struct rcu_node {
>  				/*  side effect, not as a lock. */
>  	unsigned long boost_time;
>  				/* When to start boosting (jiffies). */
> +	struct mutex boost_kthread_mutex;
> +				/* Exclusion for thread spawning and affinity */
> +				/*  manipulation. */
>  	struct task_struct *boost_kthread_task;
>  				/* kthread that takes care of priority */
>  				/*  boosting for this rcu_node structure. */
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 5199559fbbf0..3b4ee0933710 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1162,15 +1162,16 @@ static void rcu_spawn_one_boost_kthread(struct rcu_node *rnp)
>  	struct sched_param sp;
>  	struct task_struct *t;
>  
> +	mutex_lock(&rnp->boost_kthread_mutex);
>  	if (rnp->boost_kthread_task || !rcu_scheduler_fully_active)
> -		return;
> +		goto out;
>  
>  	rcu_state.boost = 1;
>  
>  	t = kthread_create(rcu_boost_kthread, (void *)rnp,
>  			   "rcub/%d", rnp_index);
>  	if (WARN_ON_ONCE(IS_ERR(t)))
> -		return;
> +		goto out;
>  
>  	raw_spin_lock_irqsave_rcu_node(rnp, flags);
>  	rnp->boost_kthread_task = t;
> @@ -1178,6 +1179,9 @@ static void rcu_spawn_one_boost_kthread(struct rcu_node *rnp)
>  	sp.sched_priority = kthread_prio;
>  	sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
>  	wake_up_process(t); /* get to TASK_INTERRUPTIBLE quickly. */
> +
> + out:
> +	mutex_unlock(&rnp->boost_kthread_mutex);
>  }
>  
>  /*
> @@ -1200,6 +1204,7 @@ static void rcu_boost_kthread_setaffinity(struct rcu_node *rnp, int outgoingcpu)
>  		return;
>  	if (!zalloc_cpumask_var(&cm, GFP_KERNEL))
>  		return;
> +	mutex_lock(&rnp->boost_kthread_mutex);
>  	for_each_leaf_node_possible_cpu(rnp, cpu)
>  		if ((mask & leaf_node_cpu_bit(rnp, cpu)) &&
>  		    cpu != outgoingcpu)
> @@ -1207,6 +1212,7 @@ static void rcu_boost_kthread_setaffinity(struct rcu_node *rnp, int outgoingcpu)
>  	if (cpumask_weight(cm) == 0)
>  		cpumask_setall(cm);
>  	set_cpus_allowed_ptr(t, cm);
> +	mutex_unlock(&rnp->boost_kthread_mutex);
>  	free_cpumask_var(cm);
>  }
>  
> -- 
> 2.31.1
> 

[PATCH 04/11] cpu/hotplug: Add dynamic parallel bringup states before CPUHP_BRINGUP_CPU

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

If the platform registers these states, bring all CPUs to each registered
state in turn, before the final bringup to CPUHP_BRINGUP_CPU. This allows
the architecture to parallelise the slow asynchronous tasks like sending
INIT/SIPI and waiting for the AP to come to life.

There is a subtlety here: even with an empty CPUHP_BP_PARALLEL_DYN step,
this means that *all* CPUs are brought through the prepare states and to
CPUHP_BP_PREPARE_DYN before any of them are taken to CPUHP_BRINGUP_CPU
and then are allowed to run for themselves to CPUHP_ONLINE.

So any combination of prepare/start calls which depend on A-B ordering
for each CPU in turn, such as the X2APIC code which used to allocate a
cluster mask 'just in case' and store it in a global variable in the
prep stage, then potentially consume that preallocated structure from
the AP and set the global pointer to NULL to be reallocated in
CPUHP_X2APIC_PREPARE for the next CPU... would explode horribly.

We believe that X2APIC was the only such case, for x86. But this is why
it remains an architecture opt-in. For now.

Note that the new parallel stages do *not* yet bring each AP to the
CPUHP_BRINGUP_CPU state. The final loop in bringup_nonboot_cpus() is
untouched, bringing each AP in turn from the final PARALLEL_DYN state
(or all the way from CPUHP_OFFLINE) to CPUHP_BRINGUP_CPU and then
waiting for that AP to do its own processing and reach CPUHP_ONLINE
before releasing the next. Parallelising that part by bringing them all
to CPUHP_BRINGUP_CPU and then waiting for them all is an exercise for
the future.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 include/linux/cpuhotplug.h |  2 ++
 kernel/cpu.c               | 27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 773c83730906..45c327538321 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -131,6 +131,8 @@ enum cpuhp_state {
 	CPUHP_MIPS_SOC_PREPARE,
 	CPUHP_BP_PREPARE_DYN,
 	CPUHP_BP_PREPARE_DYN_END		= CPUHP_BP_PREPARE_DYN + 20,
+	CPUHP_BP_PARALLEL_DYN,
+	CPUHP_BP_PARALLEL_DYN_END		= CPUHP_BP_PARALLEL_DYN + 4,
 	CPUHP_BRINGUP_CPU,
 
 	/*
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 192e43a87407..1a46eb57d8f7 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1462,6 +1462,24 @@ int bringup_hibernate_cpu(unsigned int sleep_cpu)
 void bringup_nonboot_cpus(unsigned int setup_max_cpus)
 {
 	unsigned int cpu;
+	int n = setup_max_cpus - num_online_cpus();
+
+	/* ∀ parallel pre-bringup state, bring N CPUs to it */
+	if (n > 0) {
+		enum cpuhp_state st = CPUHP_BP_PARALLEL_DYN;
+
+		while (st <= CPUHP_BP_PARALLEL_DYN_END &&
+		       cpuhp_hp_states[st].name) {
+			int i = n;
+
+			for_each_present_cpu(cpu) {
+				cpu_up(cpu, st);
+				if (!--i)
+					break;
+			}
+			st++;
+		}
+	}
 
 	for_each_present_cpu(cpu) {
 		if (num_online_cpus() >= setup_max_cpus)
@@ -1829,6 +1847,10 @@ static int cpuhp_reserve_state(enum cpuhp_state state)
 		step = cpuhp_hp_states + CPUHP_BP_PREPARE_DYN;
 		end = CPUHP_BP_PREPARE_DYN_END;
 		break;
+	case CPUHP_BP_PARALLEL_DYN:
+		step = cpuhp_hp_states + CPUHP_BP_PARALLEL_DYN;
+		end = CPUHP_BP_PARALLEL_DYN_END;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -1853,14 +1875,15 @@ static int cpuhp_store_callbacks(enum cpuhp_state state, const char *name,
 	/*
 	 * If name is NULL, then the state gets removed.
 	 *
-	 * CPUHP_AP_ONLINE_DYN and CPUHP_BP_PREPARE_DYN are handed out on
+	 * CPUHP_AP_ONLINE_DYN and CPUHP_BP_P*_DYN are handed out on
 	 * the first allocation from these dynamic ranges, so the removal
 	 * would trigger a new allocation and clear the wrong (already
 	 * empty) state, leaving the callbacks of the to be cleared state
 	 * dangling, which causes wreckage on the next hotplug operation.
 	 */
 	if (name && (state == CPUHP_AP_ONLINE_DYN ||
-		     state == CPUHP_BP_PREPARE_DYN)) {
+		     state == CPUHP_BP_PREPARE_DYN ||
+		     state == CPUHP_BP_PARALLEL_DYN)) {
 		ret = cpuhp_reserve_state(state);
 		if (ret < 0)
 			return ret;
-- 
2.31.1

[PATCH 05/11] x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

If we want to do parallel CPU bringup, we're going to need to set this up
and leave it until all CPUs are done. Might as well use the RTC spinlock
to protect the refcount, as we need to take it anyway.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/smpboot.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ac2909f0cab3..99c705935f94 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -127,17 +127,22 @@ int arch_update_cpu_topology(void)
 	return retval;
 }
 
+
+static unsigned int smpboot_warm_reset_vector_count;
+
 static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0xa, 0xf);
+	if (!smpboot_warm_reset_vector_count++) {
+		CMOS_WRITE(0xa, 0xf);
+		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
+			start_eip >> 4;
+		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
+			start_eip & 0xf;
+	}
 	spin_unlock_irqrestore(&rtc_lock, flags);
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
-							start_eip >> 4;
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
-							start_eip & 0xf;
 }
 
 static inline void smpboot_restore_warm_reset_vector(void)
@@ -149,10 +154,12 @@ static inline void smpboot_restore_warm_reset_vector(void)
 	 * to default values.
 	 */
 	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0, 0xf);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	if (!--smpboot_warm_reset_vector_count) {
+		CMOS_WRITE(0, 0xf);
 
-	*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+		*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+	}
+	spin_unlock_irqrestore(&rtc_lock, flags);
 }
 
 static void init_freq_invariance(bool secondary, bool cppc_ready);
-- 
2.31.1

[PATCH 06/11] x86/smpboot: Split up native_cpu_up into separate phases

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

There are four logical parts to what native_cpu_up() does.

First it actually wakes the AP.

Second, it waits for the AP to make it as far as wait_for_master_cpu()
which sets that CPU's bit in cpu_initialized_mask, and sets the bit in
cpu_callout_mask to let the AP proceed through cpu_init().

Then, it waits for the AP to finish cpu_init() and get as far as the
smp_callin() call, which sets that CPU's bit in cpu_callin_mask.

Finally, it does the TSC synchronization and waits for the AP to actually
mark itself online in cpu_online_mask.

This commit should have no behavioural change, but merely splits those
phases out into separate functions so that future commits can make them
happen in parallel for all APs.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/smpboot.c | 136 +++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 54 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 99c705935f94..11ef434a93a1 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1080,9 +1080,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 {
 	/* start_ip had better be page-aligned! */
 	unsigned long start_ip = real_mode_header->trampoline_start;
-
 	unsigned long boot_error = 0;
-	unsigned long timeout;
 
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);
 	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
@@ -1135,55 +1133,70 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
 						     cpu0_nmi_registered);
 
-	if (!boot_error) {
-		/*
-		 * Wait 10s total for first sign of life from AP
-		 */
-		boot_error = -1;
-		timeout = jiffies + 10*HZ;
-		while (time_before(jiffies, timeout)) {
-			if (cpumask_test_cpu(cpu, cpu_initialized_mask)) {
-				/*
-				 * Tell AP to proceed with initialization
-				 */
-				cpumask_set_cpu(cpu, cpu_callout_mask);
-				boot_error = 0;
-				break;
-			}
-			schedule();
-		}
-	}
+	return boot_error;
+}
 
-	if (!boot_error) {
-		/*
-		 * Wait till AP completes initial initialization
-		 */
-		while (!cpumask_test_cpu(cpu, cpu_callin_mask)) {
-			/*
-			 * Allow other tasks to run while we wait for the
-			 * AP to come online. This also gives a chance
-			 * for the MTRR work(triggered by the AP coming online)
-			 * to be completed in the stop machine context.
-			 */
-			schedule();
-		}
+static int do_wait_cpu_cpumask(unsigned int cpu, const struct cpumask *mask)
+{
+	unsigned long timeout;
+
+	/*
+	 * Wait up to 10s for the CPU to report in.
+	 */
+	timeout = jiffies + 10*HZ;
+	while (time_before(jiffies, timeout)) {
+		if (cpumask_test_cpu(cpu, mask))
+			return 0;
+
+		schedule();
 	}
+	return -1;
+}
 
-	if (x86_platform.legacy.warm_reset) {
-		/*
-		 * Cleanup possible dangling ends...
-		 */
-		smpboot_restore_warm_reset_vector();
+static int do_wait_cpu_initialized(unsigned int cpu)
+{
+	/*
+	 * Wait for first sign of life from AP.
+	 */
+	if (do_wait_cpu_cpumask(cpu, cpu_initialized_mask))
+		return -1;
+
+	cpumask_set_cpu(cpu, cpu_callout_mask);
+	return 0;
+}
+
+static int do_wait_cpu_callin(unsigned int cpu)
+{
+	/*
+	 * Wait till AP completes initial initialization.
+	 */
+	return do_wait_cpu_cpumask(cpu, cpu_callin_mask);
+}
+
+static int do_wait_cpu_online(unsigned int cpu)
+{
+	unsigned long flags;
+
+	/*
+	 * Check TSC synchronization with the AP (keep irqs disabled
+	 * while doing so):
+	 */
+	local_irq_save(flags);
+	check_tsc_sync_source(cpu);
+	local_irq_restore(flags);
+
+	while (!cpu_online(cpu)) {
+		cpu_relax();
+		touch_nmi_watchdog();
 	}
 
-	return boot_error;
+	return 0;
 }
 
-int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
+int do_cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
 	int apicid = apic->cpu_present_to_apicid(cpu);
 	int cpu0_nmi_registered = 0;
-	unsigned long flags;
 	int err, ret = 0;
 
 	lockdep_assert_irqs_enabled();
@@ -1230,19 +1243,6 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 		goto unreg_nmi;
 	}
 
-	/*
-	 * Check TSC synchronization with the AP (keep irqs disabled
-	 * while doing so):
-	 */
-	local_irq_save(flags);
-	check_tsc_sync_source(cpu);
-	local_irq_restore(flags);
-
-	while (!cpu_online(cpu)) {
-		cpu_relax();
-		touch_nmi_watchdog();
-	}
-
 unreg_nmi:
 	/*
 	 * Clean up the nmi handler. Do this after the callin and callout sync
@@ -1254,6 +1254,34 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	return ret;
 }
 
+int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
+{
+	int ret;
+
+	ret = do_cpu_up(cpu, tidle);
+	if (ret)
+		return ret;
+
+	ret = do_wait_cpu_initialized(cpu);
+	if (ret)
+		return ret;
+
+	ret = do_wait_cpu_callin(cpu);
+	if (ret)
+		return ret;
+
+	ret = do_wait_cpu_online(cpu);
+
+	if (x86_platform.legacy.warm_reset) {
+		/*
+		 * Cleanup possible dangling ends...
+		 */
+		smpboot_restore_warm_reset_vector();
+	}
+
+	return ret;
+}
+
 /**
  * arch_disable_smp_support() - disables SMP support for x86 at runtime
  */
-- 
2.31.1

[PATCH 07/11] cpu/hotplug: Move idle_thread_get() to <linux/smpboot.h>

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Instead of relying purely on the special-case wrapper in bringup_cpu()
to pass the idle thread to __cpu_up(), expose idle_thread_get() so that
the architecture code can obtain it directly when necessary.

This will be useful when the existing __cpu_up() is split into multiple
phases, only *one* of which will actually need the idle thread.

If the architecture code is to register its new pre-bringup states with
the cpuhp core, having a special-case wrapper to pass extra arguments is
non-trivial and it's easier just to let the arch register its function
pointer to be invoked with the standard API.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 include/linux/smpboot.h | 7 +++++++
 kernel/smpboot.h        | 2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index 9d1bc65d226c..3862addcaa34 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -5,6 +5,13 @@
 #include <linux/types.h>
 
 struct task_struct;
+
+#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
+struct task_struct *idle_thread_get(unsigned int cpu);
+#else
+static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
+#endif
+
 /* Cookie handed to the thread_fn*/
 struct smpboot_thread_data;
 
diff --git a/kernel/smpboot.h b/kernel/smpboot.h
index 34dd3d7ba40b..60c609318ad6 100644
--- a/kernel/smpboot.h
+++ b/kernel/smpboot.h
@@ -5,11 +5,9 @@
 struct task_struct;
 
 #ifdef CONFIG_GENERIC_SMP_IDLE_THREAD
-struct task_struct *idle_thread_get(unsigned int cpu);
 void idle_thread_set_boot_cpu(void);
 void idle_threads_init(void);
 #else
-static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; }
 static inline void idle_thread_set_boot_cpu(void) { }
 static inline void idle_threads_init(void) { }
 #endif
-- 
2.31.1

[PATCH 08/11] x86/tsc: Avoid synchronizing TSCs with multiple CPUs in parallel

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

The TSC sync algorithm is only designed to do a 1:1 sync between the
source and target CPUs.

In order to enable parallel CPU bringup, serialize it by using an
atomic_t containing the number of the target CPU whose turn it is.

In future we should look at inventing a 1:many TSC synchronization
algorithm, perhaps falling back to 1:1 if a warp is observed but
doing them all in parallel for the common case where no adjustment
is needed. Or just avoiding the sync completely for cases like kexec
where we trust that they were in sync already.

This is perfectly sufficient for the short term though, until we get
those further optimisations.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/tsc_sync.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/tsc_sync.c b/arch/x86/kernel/tsc_sync.c
index 50a4515fe0ad..4ee247d89a49 100644
--- a/arch/x86/kernel/tsc_sync.c
+++ b/arch/x86/kernel/tsc_sync.c
@@ -202,6 +202,7 @@ bool tsc_store_and_check_tsc_adjust(bool bootcpu)
  * Entry/exit counters that make sure that both CPUs
  * run the measurement code at once:
  */
+static atomic_t tsc_sync_cpu = ATOMIC_INIT(-1);
 static atomic_t start_count;
 static atomic_t stop_count;
 static atomic_t skip_test;
@@ -326,6 +327,8 @@ void check_tsc_sync_source(int cpu)
 		atomic_set(&test_runs, 1);
 	else
 		atomic_set(&test_runs, 3);
+
+	atomic_set(&tsc_sync_cpu, cpu);
 retry:
 	/*
 	 * Wait for the target to start or to skip the test:
@@ -407,6 +410,10 @@ void check_tsc_sync_target(void)
 	if (unsynchronized_tsc())
 		return;
 
+	/* Wait for this CPU's turn */
+	while (atomic_read(&tsc_sync_cpu) != cpu)
+		cpu_relax();
+
 	/*
 	 * Store, verify and sanitize the TSC adjust register. If
 	 * successful skip the test.
-- 
2.31.1

Re: [PATCH 08/11] x86/tsc: Avoid synchronizing TSCs with multiple CPUs in parallel

[Peter Zijlstra]

On Thu, Dec 09, 2021 at 03:09:35PM +0000, David Woodhouse wrote:
> diff --git a/arch/x86/kernel/tsc_sync.c b/arch/x86/kernel/tsc_sync.c
> index 50a4515fe0ad..4ee247d89a49 100644
> --- a/arch/x86/kernel/tsc_sync.c
> +++ b/arch/x86/kernel/tsc_sync.c
> @@ -202,6 +202,7 @@ bool tsc_store_and_check_tsc_adjust(bool bootcpu)
>   * Entry/exit counters that make sure that both CPUs
>   * run the measurement code at once:
>   */
> +static atomic_t tsc_sync_cpu = ATOMIC_INIT(-1);
>  static atomic_t start_count;
>  static atomic_t stop_count;
>  static atomic_t skip_test;
> @@ -326,6 +327,8 @@ void check_tsc_sync_source(int cpu)
>  		atomic_set(&test_runs, 1);
>  	else
>  		atomic_set(&test_runs, 3);
> +
> +	atomic_set(&tsc_sync_cpu, cpu);
>  retry:
>  	/*
>  	 * Wait for the target to start or to skip the test:
> @@ -407,6 +410,10 @@ void check_tsc_sync_target(void)
>  	if (unsynchronized_tsc())
>  		return;
>  
> +	/* Wait for this CPU's turn */
> +	while (atomic_read(&tsc_sync_cpu) != cpu)
> +		cpu_relax();
> +
>  	/*
>  	 * Store, verify and sanitize the TSC adjust register. If
>  	 * successful skip the test.

This new atomic_t seems superfluous, there isn't any actual atomic
operation used.

Re: [PATCH 08/11] x86/tsc: Avoid synchronizing TSCs with multiple CPUs in parallel

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Thu, 2021-12-09 at 16:43 +0100, Peter Zijlstra wrote:
> On Thu, Dec 09, 2021 at 03:09:35PM +0000, David Woodhouse wrote:
> > diff --git a/arch/x86/kernel/tsc_sync.c b/arch/x86/kernel/tsc_sync.c
> > index 50a4515fe0ad..4ee247d89a49 100644
> > --- a/arch/x86/kernel/tsc_sync.c
> > +++ b/arch/x86/kernel/tsc_sync.c
> > @@ -202,6 +202,7 @@ bool tsc_store_and_check_tsc_adjust(bool bootcpu)
> >   * Entry/exit counters that make sure that both CPUs
> >   * run the measurement code at once:
> >   */
> > +static atomic_t tsc_sync_cpu = ATOMIC_INIT(-1);
> >  static atomic_t start_count;
> >  static atomic_t stop_count;
> >  static atomic_t skip_test;
> > @@ -326,6 +327,8 @@ void check_tsc_sync_source(int cpu)
> >  		atomic_set(&test_runs, 1);
> >  	else
> >  		atomic_set(&test_runs, 3);
> > +
> > +	atomic_set(&tsc_sync_cpu, cpu);
> >  retry:
> >  	/*
> >  	 * Wait for the target to start or to skip the test:
> > @@ -407,6 +410,10 @@ void check_tsc_sync_target(void)
> >  	if (unsynchronized_tsc())
> >  		return;
> >  
> > +	/* Wait for this CPU's turn */
> > +	while (atomic_read(&tsc_sync_cpu) != cpu)
> > +		cpu_relax();
> > +
> >  	/*
> >  	 * Store, verify and sanitize the TSC adjust register. If
> >  	 * successful skip the test.
> 
> This new atomic_t seems superfluous, there isn't any actual atomic
> operation used.

That's true; it could just be WRITE_ONCE/READ_ONCE. But the atomic is
fairly much equivalent and does no harm.

I see this one mostly as a placeholder — I'd still prefer to have a
decent 1:many TSC sync or at least a 1:many *check* falling back to 1:1
mode if anything actually needs to be adjusted. And/or just avoid the
TSC sync completely when it's not needed, like on kexec.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

[PATCH 09/11] x86/boot: Support parallel startup of secondary CPUs

[David Woodhouse]

From: Thomas Gleixner <tglx@linutronix.de>

To allow for parallel AP bringup, we need to avoid the use of global
variables for passing information to the APs, as well as preventing them
from all trying to use the same real-mode stack simultaneously.

So, introduce a 'lock' field in struct trampoline_header to use as a
simple bit-spinlock for the real-mode stack. That lock also protects
the global variables initial_gs, initial_stack and early_gdt_descr,
which can now be calculated...

So how do we calculate those addresses? Well, they they can all be found
from the per_cpu data for this CPU. Simples! Except... how does it know
what its CPU# is? OK, we export the cpuid_to_apicid[] array and it can
search it to find its APIC ID in there.

But now you whine at me that it doesn't even know its APIC ID? Well, if
it's a relatively modern CPU then the APIC ID is in CPUID leaf 0x0B so
we can use that. Otherwise... erm... OK, otherwise it can't have parallel
CPU bringup for now. We'll still use a global variable for those CPUs and
bring them up one at a time.

So add a global 'smpboot_control' field which either contains the APIC
ID, or a flag indicating that it can be found in CPUID.

Not-signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 arch/x86/include/asm/realmode.h      |  3 ++
 arch/x86/include/asm/smp.h           |  9 +++-
 arch/x86/kernel/acpi/sleep.c         |  1 +
 arch/x86/kernel/apic/apic.c          |  2 +-
 arch/x86/kernel/head_64.S            | 71 ++++++++++++++++++++++++++++
 arch/x86/kernel/smpboot.c            | 19 +++++++-
 arch/x86/realmode/init.c             |  3 ++
 arch/x86/realmode/rm/trampoline_64.S | 14 ++++++
 kernel/smpboot.c                     |  2 +-
 9 files changed, 119 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 5db5d083c873..e1cc4bc746bc 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -51,6 +51,7 @@ struct trampoline_header {
 	u64 efer;
 	u32 cr4;
 	u32 flags;
+	u32 lock;
 #endif
 };
 
@@ -64,6 +65,8 @@ extern unsigned long initial_stack;
 extern unsigned long initial_vc_handler;
 #endif
 
+extern u32 *trampoline_lock;
+
 extern unsigned char real_mode_blob[];
 extern unsigned char real_mode_relocs[];
 
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 81a0211a372d..ca807c29dc34 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -196,5 +196,12 @@ extern void nmi_selftest(void);
 #define nmi_selftest() do { } while (0)
 #endif
 
-#endif /* __ASSEMBLY__ */
+extern unsigned int smpboot_control;
+
+#endif /* !__ASSEMBLY__ */
+
+/* Control bits for startup_64 */
+#define	STARTUP_USE_APICID	0x10000
+#define	STARTUP_USE_CPUID_0B	0x20000
+
 #endif /* _ASM_X86_SMP_H */
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3f85fcae450c..9598ebf4f9d6 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -114,6 +114,7 @@ int x86_acpi_suspend_lowlevel(void)
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_rw(smp_processor_id());
 	initial_gs = per_cpu_offset(smp_processor_id());
+	smpboot_control = 0;
 #endif
 	initial_code = (unsigned long)wakeup_long64;
        saved_magic = 0x123456789abcdef0L;
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..5b20e051d84c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2335,7 +2335,7 @@ static int nr_logical_cpuids = 1;
 /*
  * Used to store mapping between logical CPU IDs and APIC IDs.
  */
-static int cpuid_to_apicid[] = {
+int cpuid_to_apicid[] = {
 	[0 ... NR_CPUS - 1] = -1,
 };
 
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index d8b3ebd2bb85..0249212e23d2 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -25,6 +25,7 @@
 #include <asm/export.h>
 #include <asm/nospec-branch.h>
 #include <asm/fixmap.h>
+#include <asm/smp.h>
 
 /*
  * We are not able to switch in one step to the final KERNEL ADDRESS SPACE
@@ -176,6 +177,64 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 1:
 	UNWIND_HINT_EMPTY
 
+	/*
+	 * Is this the boot CPU coming up? If so everything is available
+	 * in initial_gs, initial_stack and early_gdt_descr.
+	 */
+	movl	smpboot_control(%rip), %eax
+	testl	%eax, %eax
+	jz	.Lsetup_cpu
+
+	/*
+	 * Secondary CPUs find out the offsets via the APIC ID. For parallel
+	 * boot the APIC ID is retrieved from CPUID, otherwise it's encoded
+	 * in smpboot_control:
+	 * Bit 0-15	APICID if STARTUP_USE_CPUID_0B is not set
+	 * Bit 16 	Secondary boot flag
+	 * Bit 17	Parallel boot flag
+	 */
+	testl	$STARTUP_USE_CPUID_0B, %eax
+	jz	.Lsetup_AP
+
+	mov	$0x0B, %eax
+	xorl	%ecx, %ecx
+	cpuid
+	mov	%edx, %eax
+
+.Lsetup_AP:
+	/* EAX contains the APICID of the current CPU */
+	andl	$0xFFFF, %eax
+	xorl	%ecx, %ecx
+	leaq	cpuid_to_apicid(%rip), %rbx
+
+.Lfind_cpunr:
+	cmpl	(%rbx), %eax
+	jz	.Linit_cpu_data
+	addq	$4, %rbx
+	addq	$8, %rcx
+	jmp	.Lfind_cpunr
+
+.Linit_cpu_data:
+	/* Get the per cpu offset */
+	leaq	__per_cpu_offset(%rip), %rbx
+	addq	%rcx, %rbx
+	movq	(%rbx), %rbx
+	/* Save it for GS BASE setup */
+	movq	%rbx, initial_gs(%rip)
+
+	/* Calculate the GDT address */
+	movq	$gdt_page, %rcx
+	addq	%rbx, %rcx
+	movq	%rcx, early_gdt_descr_base(%rip)
+
+	/* Find the idle task stack */
+	movq	$idle_threads, %rcx
+	addq	%rbx, %rcx
+	movq	(%rcx), %rcx
+	movq	TASK_threadsp(%rcx), %rcx
+	movq	%rcx, initial_stack(%rip)
+
+.Lsetup_cpu:
 	/*
 	 * We must switch to a new descriptor in kernel space for the GDT
 	 * because soon the kernel won't have access anymore to the userspace
@@ -216,6 +275,14 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
 	 */
 	movq initial_stack(%rip), %rsp
 
+	/* Drop the realmode protection. For the boot CPU the pointer is NULL! */
+	movq	trampoline_lock(%rip), %rax
+	testq	%rax, %rax
+	jz	.Lsetup_idt
+	lock
+	btrl	$0, (%rax)
+
+.Lsetup_idt:
 	/* Setup and Load IDT */
 	pushq	%rsi
 	call	early_setup_idt
@@ -347,6 +414,7 @@ SYM_DATA(initial_vc_handler,	.quad handle_vc_boot_ghcb)
  * reliably detect the end of the stack.
  */
 SYM_DATA(initial_stack, .quad init_thread_union + THREAD_SIZE - FRAME_SIZE)
+SYM_DATA(trampoline_lock, .quad 0);
 	__FINITDATA
 
 	__INIT
@@ -572,6 +640,9 @@ SYM_DATA_END(level1_fixmap_pgt)
 SYM_DATA(early_gdt_descr,		.word GDT_ENTRIES*8-1)
 SYM_DATA_LOCAL(early_gdt_descr_base,	.quad INIT_PER_CPU_VAR(gdt_page))
 
+	.align 16
+SYM_DATA(smpboot_control,		.long 0)
+
 	.align 16
 /* This must match the first entry in level2_kernel_pgt */
 SYM_DATA(phys_base, .quad 0x0)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 11ef434a93a1..8fdf889acf5d 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1069,6 +1069,16 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
 	return 0;
 }
 
+static void setup_smpboot_control(unsigned int apicid)
+{
+#ifdef CONFIG_X86_64
+	if (boot_cpu_data.cpuid_level < 0x0B)
+		smpboot_control = apicid | STARTUP_USE_APICID;
+	else
+		smpboot_control = STARTUP_USE_CPUID_0B;
+#endif
+}
+
 /*
  * NOTE - on most systems this is a PHYSICAL apic ID, but on multiquad
  * (ie clustered apic addressing mode), this is a LOGICAL apic ID.
@@ -1083,9 +1093,14 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	unsigned long boot_error = 0;
 
 	idle->thread.sp = (unsigned long)task_pt_regs(idle);
-	early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
 	initial_code = (unsigned long)start_secondary;
-	initial_stack  = idle->thread.sp;
+
+	if (IS_ENABLED(CONFIG_X86_32)) {
+		early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
+		initial_stack  = idle->thread.sp;
+	}
+
+	setup_smpboot_control(apicid);
 
 	/* Enable the espfix hack for this CPU */
 	init_espfix_ap(cpu);
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index 4a3da7592b99..7dc2e817bd02 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -127,6 +127,9 @@ static void __init setup_real_mode(void)
 
 	trampoline_header->flags = 0;
 
+	trampoline_lock = &trampoline_header->lock;
+	*trampoline_lock = 0;
+
 	trampoline_pgd = (u64 *) __va(real_mode_header->trampoline_pgd);
 	trampoline_pgd[0] = trampoline_pgd_entry.pgd;
 	trampoline_pgd[511] = init_top_pgt[511].pgd;
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index cc8391f86cdb..801b03d3c5a8 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -49,6 +49,19 @@ SYM_CODE_START(trampoline_start)
 	mov	%ax, %es
 	mov	%ax, %ss
 
+	/*
+	 * Make sure only one CPU fiddles with the realmode stack
+	 */
+.Llock_rm:
+	btw	$0, tr_lock
+	jnc	2f
+	pause
+	jmp	.Llock_rm
+2:
+	lock
+	btsw	$0, tr_lock
+	jc	.Llock_rm
+
 	# Setup stack
 	movl	$rm_stack_end, %esp
 
@@ -192,6 +205,7 @@ SYM_DATA_START(trampoline_header)
 	SYM_DATA(tr_efer,		.space 8)
 	SYM_DATA(tr_cr4,		.space 4)
 	SYM_DATA(tr_flags,		.space 4)
+	SYM_DATA(tr_lock,		.space 4)
 SYM_DATA_END(trampoline_header)
 
 #include "trampoline_common.S"
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index f6bc0bc8a2aa..934e64ff4eed 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -25,7 +25,7 @@
  * For the hotplug case we keep the task structs around and reuse
  * them.
  */
-static DEFINE_PER_CPU(struct task_struct *, idle_threads);
+DEFINE_PER_CPU(struct task_struct *, idle_threads);
 
 struct task_struct *idle_thread_get(unsigned int cpu)
 {
-- 
2.31.1

Re: [PATCH 09/11] x86/boot: Support parallel startup of secondary CPUs

[Peter Zijlstra]

On Thu, Dec 09, 2021 at 03:09:36PM +0000, David Woodhouse wrote:

> +	lock
> +	btrl	$0, (%rax)


> +	lock
> +	btsw	$0, tr_lock

I find that really weird style, but whatever..

Re: [PATCH 09/11] x86/boot: Support parallel startup of secondary CPUs

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

On Thu, 2021-12-09 at 16:50 +0100, Peter Zijlstra wrote:
> On Thu, Dec 09, 2021 at 03:09:36PM +0000, David Woodhouse wrote:
> 
> > +	lock
> > +	btrl	$0, (%rax)
> 
> 
> > +	lock
> > +	btsw	$0, tr_lock
> 
> I find that really weird style, but whatever..

I think it's fairly gratuitous too. There's no reason we can't use btsl
even in .code16. I'll fix it.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

[PATCH 10/11] x86/smp: Bring up secondary CPUs in parallel

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Start by documenting the various synchronization points between the CPU
doing the bringup, and the target AP.

Then enable parallel bringup where we can manage it, which is in 64-bit
mode when the CPU provides its APIC ID in CPUID leaf 0x0b.

The main win here is by sending the INIT/SIPI to all CPUs first, then
coming back in a second round as do_wait_cpu_initialized() is called
from the x86/cpu:wait-init cpuhp stage to check that they have reached
cpu_init() and release them to proceed further.

This reduces the time taken for bringup on my 28-thread Haswell system
by about 60% on a boot from EFI (120ms to 49.5ms), and somewhat less
than that for kexec (100ms to 80ms). It isn't clear (yet) why kexec
is faster than boot for the serial bringup, while boot is faster then
kexec for the parallel bringup. Either way, the parallel bringup is
faster than serial; just by a smaller ratio in the kexec case.

Only using kexec on a 2-socket 96-way Skylake, it reduces the bringup
time from ~500ms to ~34ms.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/smpboot.c | 77 +++++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 8fdf889acf5d..dc62e28ede48 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -57,6 +57,7 @@
 #include <linux/pgtable.h>
 #include <linux/overflow.h>
 #include <linux/syscore_ops.h>
+#include <linux/smpboot.h>
 
 #include <asm/acpi.h>
 #include <asm/desc.h>
@@ -241,17 +242,33 @@ static void notrace start_secondary(void *unused)
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
 #endif
+	/*
+	 * Sync point with do_wait_cpu_initialized(). On boot, all secondary
+	 * CPUs reach this stage after receiving INIT/SIPI from do_cpu_up()
+	 * in the x86/cpu:kick cpuhp stage. At the start of cpu_init() they
+	 * will wait for do_wait_cpu_initialized() in the x86/cpu:wait-init
+	 * cpuhp stage to set their bit in smp_callout_mask to release them.
+	 */
 	cpu_init_secondary();
 	rcu_cpu_starting(raw_smp_processor_id());
 	x86_cpuinit.early_percpu_clock_init();
+
+	/*
+	 * Sync point with do_wait_cpu_callin(). The AP doesn't wait here
+	 * but just sets the bit to let the existing CPU (BSP) know that
+	 * it's got this far.
+	 */
 	smp_callin();
 
 	enable_start_cpu0 = 0;
 
 	/* otherwise gcc will move up smp_processor_id before the cpu_init */
 	barrier();
+
 	/*
-	 * Check TSC synchronization with the boot CPU:
+	 * Check TSC synchronization with the boot CPU (or whichever CPU
+	 * is controlling the bringup). It will do its part of this from
+	 * do_wait_cpu_online(), making it an implicit sync point.
 	 */
 	check_tsc_sync_target();
 
@@ -264,6 +281,7 @@ static void notrace start_secondary(void *unused)
 	 * half valid vector space.
 	 */
 	lock_vector_lock();
+	/* Sync point with do_wait_cpu_online() */
 	set_cpu_online(smp_processor_id(), true);
 	lapic_online();
 	unlock_vector_lock();
@@ -1168,6 +1186,13 @@ static int do_wait_cpu_cpumask(unsigned int cpu, const struct cpumask *mask)
 	return -1;
 }
 
+/*
+ * Bringup step two: Wait for the target AP to reach cpu_init_secondary()
+ * and thus wait_for_master_cpu(), then set cpu_callout_mask to allow it
+ * to proceed.  The AP will then proceed past setting its 'callin' bit
+ * and end up waiting in check_tsc_sync_target() until we reach
+ * do_wait_cpu_online() to tend to it.
+ */
 static int do_wait_cpu_initialized(unsigned int cpu)
 {
 	/*
@@ -1180,6 +1205,13 @@ static int do_wait_cpu_initialized(unsigned int cpu)
 	return 0;
 }
 
+/*
+ * Bringup step three: Wait for the target AP to reach smp_callin().
+ * The AP is not waiting for us here so we don't need to parallelise
+ * this step. Not entirely clear why we care about this, since we just
+ * proceed directly to TSC synchronization which is the next sync
+ * point with the AP anyway.
+ */
 static int do_wait_cpu_callin(unsigned int cpu)
 {
 	/*
@@ -1188,6 +1220,10 @@ static int do_wait_cpu_callin(unsigned int cpu)
 	return do_wait_cpu_cpumask(cpu, cpu_callin_mask);
 }
 
+/*
+ * Bringup step four: Synchronize the TSC and wait for the target AP
+ * to reach set_cpu_online() in start_secondary().
+ */
 static int do_wait_cpu_online(unsigned int cpu)
 {
 	unsigned long flags;
@@ -1200,6 +1236,12 @@ static int do_wait_cpu_online(unsigned int cpu)
 	check_tsc_sync_source(cpu);
 	local_irq_restore(flags);
 
+	/*
+	 * Wait for the AP to mark itself online. Not entirely
+	 * clear why we care, since the generic cpuhp code will
+	 * wait for it to each CPUHP_AP_ONLINE_IDLE before going
+	 * ahead with the rest of the bringup anyway.
+	 */
 	while (!cpu_online(cpu)) {
 		cpu_relax();
 		touch_nmi_watchdog();
@@ -1273,13 +1315,16 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 {
 	int ret;
 
-	ret = do_cpu_up(cpu, tidle);
-	if (ret)
-		return ret;
+	/* If parallel AP bringup isn't enabled, perform the first steps now. */
+	if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 0x0B) {
+		ret = do_cpu_up(cpu, tidle);
+		if (ret)
+			return ret;
 
-	ret = do_wait_cpu_initialized(cpu);
-	if (ret)
-		return ret;
+		ret = do_wait_cpu_initialized(cpu);
+		if (ret)
+			return ret;
+	}
 
 	ret = do_wait_cpu_callin(cpu);
 	if (ret)
@@ -1297,6 +1342,12 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	return ret;
 }
 
+/* Bringup step one: Send INIT/SIPI to the target AP */
+static int native_cpu_kick(unsigned int cpu)
+{
+	return do_cpu_up(cpu, idle_thread_get(cpu));
+}
+
 /**
  * arch_disable_smp_support() - disables SMP support for x86 at runtime
  */
@@ -1475,6 +1526,18 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 	smp_quirk_init_udelay();
 
 	speculative_store_bypass_ht_init();
+
+	/*
+	 * We can do 64-bit AP bringup in parallel if the CPU reports its
+	 * APIC ID in CPUID leaf 0x0B. Otherwise it's too hard.
+	 */
+	if (IS_ENABLED(CONFIG_X86_64) && boot_cpu_data.cpuid_level >= 0x0B) {
+		cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
+					  native_cpu_kick, NULL);
+
+		cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:wait-init",
+					  do_wait_cpu_initialized, NULL);
+	}
 }
 
 void arch_thaw_secondary_cpus_begin(void)
-- 
2.31.1

[PATCH 11/11] x86/kvm: Silence per-cpu pr_info noise about KVM clocks and steal time

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

I made the actual CPU bringup go nice and fast... and then Linux spends
half a minute printing stupid nonsense about clocks and steal time for
each of 256 vCPUs. Don't do that. Nobody cares.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/kvm.c      | 6 +++---
 arch/x86/kernel/kvmclock.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 59abbdad7729..a438217cbfac 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -313,7 +313,7 @@ static void kvm_register_steal_time(void)
 		return;
 
 	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
-	pr_info("stealtime: cpu %d, msr %llx\n", cpu,
+	pr_debug("stealtime: cpu %d, msr %llx\n", cpu,
 		(unsigned long long) slow_virt_to_phys(st));
 }
 
@@ -350,7 +350,7 @@ static void kvm_guest_cpu_init(void)
 
 		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
 		__this_cpu_write(apf_reason.enabled, 1);
-		pr_info("setup async PF for cpu %d\n", smp_processor_id());
+		pr_debug("setup async PF for cpu %d\n", smp_processor_id());
 	}
 
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) {
@@ -376,7 +376,7 @@ static void kvm_pv_disable_apf(void)
 	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
 	__this_cpu_write(apf_reason.enabled, 0);
 
-	pr_info("disable async PF for cpu %d\n", smp_processor_id());
+	pr_debug("disable async PF for cpu %d\n", smp_processor_id());
 }
 
 static void kvm_disable_steal_time(void)
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 462dd8e9b03d..a35cbf9107af 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -174,7 +174,7 @@ static void kvm_register_clock(char *txt)
 
 	pa = slow_virt_to_phys(&src->pvti) | 0x01ULL;
 	wrmsrl(msr_kvm_system_time, pa);
-	pr_info("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
+	pr_debug("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
 }
 
 static void kvm_save_sched_clock_state(void)
-- 
2.31.1

Re: [PATCH 11/11] x86/kvm: Silence per-cpu pr_info noise about KVM clocks and steal time

[Paolo Bonzini]

On 12/9/21 16:09, David Woodhouse wrote:
> From: David Woodhouse<dwmw@amazon.co.uk>
> 
> I made the actual CPU bringup go nice and fast... and then Linux spends
> half a minute printing stupid nonsense about clocks and steal time for
> each of 256 vCPUs. Don't do that. Nobody cares.

Queued this one, it makes sense even without the rest of the series.

Paolo

> Signed-off-by: David Woodhouse<dwmw@amazon.co.uk>
> ---
>   arch/x86/kernel/kvm.c      | 6 +++---
>   arch/x86/kernel/kvmclock.c | 2 +-
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 59abbdad7729..a438217cbfac 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -313,7 +313,7 @@ static void kvm_register_steal_time(void)
>   		return;
>   
>   	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
> -	pr_info("stealtime: cpu %d, msr %llx\n", cpu,
> +	pr_debug("stealtime: cpu %d, msr %llx\n", cpu,
>   		(unsigned long long) slow_virt_to_phys(st));
>   }
>   
> @@ -350,7 +350,7 @@ static void kvm_guest_cpu_init(void)
>   
>   		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
>   		__this_cpu_write(apf_reason.enabled, 1);
> -		pr_info("setup async PF for cpu %d\n", smp_processor_id());
> +		pr_debug("setup async PF for cpu %d\n", smp_processor_id());
>   	}
>   
>   	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) {
> @@ -376,7 +376,7 @@ static void kvm_pv_disable_apf(void)
>   	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
>   	__this_cpu_write(apf_reason.enabled, 0);
>   
> -	pr_info("disable async PF for cpu %d\n", smp_processor_id());
> +	pr_debug("disable async PF for cpu %d\n", smp_processor_id());
>   }
>   
>   static void kvm_disable_steal_time(void)
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 462dd8e9b03d..a35cbf9107af 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -174,7 +174,7 @@ static void kvm_register_clock(char *txt)
>   
>   	pa = slow_virt_to_phys(&src->pvti) | 0x01ULL;
>   	wrmsrl(msr_kvm_system_time, pa);
> -	pr_info("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
> +	pr_debug("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
>   }
>   
>   static void kvm_save_sched_clock_state(void)
> -- 2.31.1
>