Re: [PATCH v2 04/11] stop_machine: Add NMI-based execution path

["Chang S. Bae" <chang.seok.bae@intel.com>]

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

Like others, I also checked the review bot:

  https://sashiko.dev/#/patchset/20260331014251.86353-1-chang.seok.bae@intel.com

I thought all of comments could be converged in this patch. My take is 
below.

On 3/30/2026 6:42 PM, Chang S. Bae wrote:
> 
> +struct nmi_stop {
> +	struct multi_stop_data	*data;
> +	int			ret;
> +	bool			done;

The intention was to make it clear at the waiting loop. But ->data == 
NULL check can substitute it and a single variable looks to make it more 
robust and simple.

> +bool noinstr stop_machine_nmi_handler(void)
> +{
> +	struct multi_stop_data *msdata = raw_cpu_read(nmi_stop.data);
> +	unsigned int cpu = smp_processor_id();
> +	int ret;
> +
> +	if (!msdata || !cpumask_test_and_clear_cpu(cpu, msdata->nmi_cpus))
> +		return false;

smp_processor_id() and cpumask_test_and_clear_cpu() are wrappers that 
could include instrumentation, so not suitable here. Instead, 
raw_smp_processor_id() and arch_test_and_clear_bit() are inner functions.


> +	/* Ensure the handler went through before reading the result */
> +	if (!wait_for_nmi_handler())
> +		return -ETIMEDOUT;

On error exit, the stop_data pointer should be cleaned up as well before 
it is freed later.


Attached is the diff addressing them.

[-- Attachment #2: patch4.diff --]
[-- Type: text/plain, Size: 2107 bytes --]

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 45ea62f1b2b5..5bd1105d1a11 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -730,7 +730,6 @@ int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 struct nmi_stop {
 	struct multi_stop_data	*data;
 	int			ret;
-	bool			done;
 };
 
 static DEFINE_PER_CPU(struct nmi_stop, nmi_stop);
@@ -743,10 +742,10 @@ static DEFINE_PER_CPU(struct nmi_stop, nmi_stop);
 bool noinstr stop_machine_nmi_handler(void)
 {
 	struct multi_stop_data *msdata = raw_cpu_read(nmi_stop.data);
-	unsigned int cpu = smp_processor_id();
+	unsigned int cpu = raw_smp_processor_id();
 	int ret;
 
-	if (!msdata || !cpumask_test_and_clear_cpu(cpu, msdata->nmi_cpus))
+	if (!msdata || !arch_test_and_clear_bit(cpu, cpumask_bits(msdata->nmi_cpus)))
 		return false;
 
 	/*
@@ -759,7 +758,6 @@ bool noinstr stop_machine_nmi_handler(void)
 	instrumentation_end();
 
 	raw_cpu_write(nmi_stop.ret,  ret);
-	raw_cpu_write(nmi_stop.done, true);
 	raw_cpu_write(nmi_stop.data, NULL);
 
 	return true;
@@ -770,10 +768,11 @@ static bool wait_for_nmi_handler(void)
 	/* Conservative timeout */
 	unsigned long timeout = USEC_PER_SEC;
 
-	while (!this_cpu_read(nmi_stop.done) && timeout--)
+	/* The handler clears up at the end */
+	while (this_cpu_read(nmi_stop.data) && timeout--)
 		udelay(1);
 
-	return this_cpu_read(nmi_stop.done);
+	return !this_cpu_read(nmi_stop.data);
 }
 
 static int nmi_stop_run(struct multi_stop_data *msdata)
@@ -783,12 +782,16 @@ static int nmi_stop_run(struct multi_stop_data *msdata)
 	 * self-NMI to execute the stop function from the NMI handler
 	 */
 	this_cpu_write(nmi_stop.data, msdata);
-	this_cpu_write(nmi_stop.done, false);
 	arch_send_self_nmi();
 
-	/* Ensure the handler went through before reading the result */
-	if (!wait_for_nmi_handler())
+	/*
+	 * Ensure the handler went through before reading the result.
+	 * Otherwise, make no stale state left behind.
+	 */
+	if (!wait_for_nmi_handler()) {
+		this_cpu_write(nmi_stop.data, NULL);
 		return -ETIMEDOUT;
+	}
 
 	return this_cpu_read(nmi_stop.ret);
 }