[kvm-unit-tests PATCH v2 0/3] lib/on-cpus: A couple of fixes

[kvm-unit-tests PATCH v2 0/3] lib/on-cpus: A couple of fixes

[Andrew Jones]

While creating riscv SBI HSM tests a couple strange on-cpus behaviors
were observed. Fix 'em.

v2:
 - Added patch for barrier after func() [Alex]
 - Improved commit message for patch1 [Alex]

Andrew Jones (3):
  lib/on-cpus: Correct and simplify synchronization
  lib/on-cpus: Add barrier after func call
  lib/on-cpus: Fix on_cpumask

 lib/cpumask.h | 14 +++++++++++++
 lib/on-cpus.c | 56 ++++++++++++++++++++-------------------------------
 2 files changed, 36 insertions(+), 34 deletions(-)

-- 
2.47.0

[kvm-unit-tests PATCH v2 1/3] lib/on-cpus: Correct and simplify synchronization

[Andrew Jones]

get/put_on_cpu_info() were providing per-cpu locking for the per-cpu
on_cpu info, but it's difficult to reason that they're correct since
they use test_and_set/clear rather than a typical lock. Just revert
to a typical spinlock to simplify it. Also simplify the break case
for on_cpu_async() - we don't care if func is NULL, we only care
that the cpu is idle. And, finally, add a missing barrier to
on_cpu_async(). Before commit 018550041b38 ("arm/arm64: Remove
spinlocks from on_cpu_async") the spin_unlock() provided an implicit
barrier at the correct location, but moving the release to the more
logical location, below the setting of idle, lost it.

Fixes: 018550041b38 ("arm/arm64: Remove spinlocks from on_cpu_async")
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/on-cpus.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/lib/on-cpus.c b/lib/on-cpus.c
index 892149338419..f6072117fa1b 100644
--- a/lib/on-cpus.c
+++ b/lib/on-cpus.c
@@ -9,6 +9,7 @@
 #include <on-cpus.h>
 #include <asm/barrier.h>
 #include <asm/smp.h>
+#include <asm/spinlock.h>
 
 bool cpu0_calls_idle;
 
@@ -18,18 +19,7 @@ struct on_cpu_info {
 	cpumask_t waiters;
 };
 static struct on_cpu_info on_cpu_info[NR_CPUS];
-static cpumask_t on_cpu_info_lock;
-
-static bool get_on_cpu_info(int cpu)
-{
-	return !cpumask_test_and_set_cpu(cpu, &on_cpu_info_lock);
-}
-
-static void put_on_cpu_info(int cpu)
-{
-	int ret = cpumask_test_and_clear_cpu(cpu, &on_cpu_info_lock);
-	assert(ret);
-}
+static struct spinlock lock;
 
 static void __deadlock_check(int cpu, const cpumask_t *waiters, bool *found)
 {
@@ -81,18 +71,14 @@ void do_idle(void)
 	if (cpu == 0)
 		cpu0_calls_idle = true;
 
-	set_cpu_idle(cpu, true);
-	smp_send_event();
-
 	for (;;) {
+		set_cpu_idle(cpu, true);
+		smp_send_event();
+
 		while (cpu_idle(cpu))
 			smp_wait_for_event();
 		smp_rmb();
 		on_cpu_info[cpu].func(on_cpu_info[cpu].data);
-		on_cpu_info[cpu].func = NULL;
-		smp_wmb();
-		set_cpu_idle(cpu, true);
-		smp_send_event();
 	}
 }
 
@@ -110,17 +96,17 @@ void on_cpu_async(int cpu, void (*func)(void *data), void *data)
 
 	for (;;) {
 		cpu_wait(cpu);
-		if (get_on_cpu_info(cpu)) {
-			if ((volatile void *)on_cpu_info[cpu].func == NULL)
-				break;
-			put_on_cpu_info(cpu);
-		}
+		spin_lock(&lock);
+		if (cpu_idle(cpu))
+			break;
+		spin_unlock(&lock);
 	}
 
 	on_cpu_info[cpu].func = func;
 	on_cpu_info[cpu].data = data;
+	smp_wmb();
 	set_cpu_idle(cpu, false);
-	put_on_cpu_info(cpu);
+	spin_unlock(&lock);
 	smp_send_event();
 }
 
-- 
2.47.0

Re: [kvm-unit-tests PATCH v2 1/3] lib/on-cpus: Correct and simplify synchronization

[Eric Auger]

On 10/31/24 13:39, Andrew Jones wrote:
> get/put_on_cpu_info() were providing per-cpu locking for the per-cpu
> on_cpu info, but it's difficult to reason that they're correct since
> they use test_and_set/clear rather than a typical lock. Just revert
> to a typical spinlock to simplify it. Also simplify the break case
> for on_cpu_async() - we don't care if func is NULL, we only care
> that the cpu is idle. And, finally, add a missing barrier to
> on_cpu_async(). Before commit 018550041b38 ("arm/arm64: Remove
> spinlocks from on_cpu_async") the spin_unlock() provided an implicit
> barrier at the correct location, but moving the release to the more
> logical location, below the setting of idle, lost it.
>
> Fixes: 018550041b38 ("arm/arm64: Remove spinlocks from on_cpu_async")
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  lib/on-cpus.c | 36 +++++++++++-------------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/lib/on-cpus.c b/lib/on-cpus.c
> index 892149338419..f6072117fa1b 100644
> --- a/lib/on-cpus.c
> +++ b/lib/on-cpus.c
> @@ -9,6 +9,7 @@
>  #include <on-cpus.h>
>  #include <asm/barrier.h>
>  #include <asm/smp.h>
> +#include <asm/spinlock.h>
>  
>  bool cpu0_calls_idle;
>  
> @@ -18,18 +19,7 @@ struct on_cpu_info {
>  	cpumask_t waiters;
>  };
>  static struct on_cpu_info on_cpu_info[NR_CPUS];
> -static cpumask_t on_cpu_info_lock;
> -
> -static bool get_on_cpu_info(int cpu)
> -{
> -	return !cpumask_test_and_set_cpu(cpu, &on_cpu_info_lock);
> -}
> -
> -static void put_on_cpu_info(int cpu)
> -{
> -	int ret = cpumask_test_and_clear_cpu(cpu, &on_cpu_info_lock);
> -	assert(ret);
> -}
> +static struct spinlock lock;
>  
>  static void __deadlock_check(int cpu, const cpumask_t *waiters, bool *found)
>  {
> @@ -81,18 +71,14 @@ void do_idle(void)
>  	if (cpu == 0)
>  		cpu0_calls_idle = true;
>  
> -	set_cpu_idle(cpu, true);
> -	smp_send_event();
> -
>  	for (;;) {
> +		set_cpu_idle(cpu, true);
> +		smp_send_event();
> +
>  		while (cpu_idle(cpu))
>  			smp_wait_for_event();
>  		smp_rmb();
>  		on_cpu_info[cpu].func(on_cpu_info[cpu].data);
> -		on_cpu_info[cpu].func = NULL;
> -		smp_wmb();
> -		set_cpu_idle(cpu, true);
> -		smp_send_event();
>  	}
>  }
>  
> @@ -110,17 +96,17 @@ void on_cpu_async(int cpu, void (*func)(void *data), void *data)
>  
>  	for (;;) {
>  		cpu_wait(cpu);
> -		if (get_on_cpu_info(cpu)) {
> -			if ((volatile void *)on_cpu_info[cpu].func == NULL)
> -				break;
> -			put_on_cpu_info(cpu);
> -		}
> +		spin_lock(&lock);
> +		if (cpu_idle(cpu))
> +			break;
> +		spin_unlock(&lock);
>  	}
>  
>  	on_cpu_info[cpu].func = func;
>  	on_cpu_info[cpu].data = data;
> +	smp_wmb();
>  	set_cpu_idle(cpu, false);
> -	put_on_cpu_info(cpu);
> +	spin_unlock(&lock);
>  	smp_send_event();
>  }
>  

[kvm-unit-tests PATCH v2 2/3] lib/on-cpus: Add barrier after func call

[Andrew Jones]

It's reasonable for users of on_cpu() and on_cpumask() to assume
they can read data that 'func' has written when the call completes.
Ensure the caller doesn't observe a completion (target cpus are again
idle) without also being able to observe any writes which were made
by func(). Do so by adding barriers to implement the following

 target-cpu                                   calling-cpu
 ----------                                   -----------
 func() /* store something */                 /* wait for target to be idle */
 smp_wmb();                                   smp_rmb();
 set_cpu_idle(smp_processor_id(), true);      /* load what func() stored */

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/on-cpus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/on-cpus.c b/lib/on-cpus.c
index f6072117fa1b..356f284be61b 100644
--- a/lib/on-cpus.c
+++ b/lib/on-cpus.c
@@ -79,6 +79,7 @@ void do_idle(void)
 			smp_wait_for_event();
 		smp_rmb();
 		on_cpu_info[cpu].func(on_cpu_info[cpu].data);
+		smp_wmb(); /* pairs with the smp_rmb() in on_cpu() and on_cpumask() */
 	}
 }
 
@@ -145,12 +146,14 @@ void on_cpumask(const cpumask_t *mask, void (*func)(void *data), void *data)
 		smp_wait_for_event();
 	for_each_cpu(cpu, mask)
 		cpumask_clear_cpu(me, &on_cpu_info[cpu].waiters);
+	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
 }
 
 void on_cpu(int cpu, void (*func)(void *data), void *data)
 {
 	on_cpu_async(cpu, func, data);
 	cpu_wait(cpu);
+	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
 }
 
 void on_cpus(void (*func)(void *data), void *data)
-- 
2.47.0

Re: [kvm-unit-tests PATCH v2 2/3] lib/on-cpus: Add barrier after func call

[Andrew Jones]

On Thu, Oct 31, 2024 at 01:39:51PM +0100, Andrew Jones wrote:
> It's reasonable for users of on_cpu() and on_cpumask() to assume
> they can read data that 'func' has written when the call completes.
> Ensure the caller doesn't observe a completion (target cpus are again
> idle) without also being able to observe any writes which were made
> by func(). Do so by adding barriers to implement the following
> 
>  target-cpu                                   calling-cpu
>  ----------                                   -----------
>  func() /* store something */                 /* wait for target to be idle */
>  smp_wmb();                                   smp_rmb();
>  set_cpu_idle(smp_processor_id(), true);      /* load what func() stored */
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

I added a

Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>

to this patch.

> ---
>  lib/on-cpus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/on-cpus.c b/lib/on-cpus.c
> index f6072117fa1b..356f284be61b 100644
> --- a/lib/on-cpus.c
> +++ b/lib/on-cpus.c
> @@ -79,6 +79,7 @@ void do_idle(void)
>  			smp_wait_for_event();
>  		smp_rmb();
>  		on_cpu_info[cpu].func(on_cpu_info[cpu].data);
> +		smp_wmb(); /* pairs with the smp_rmb() in on_cpu() and on_cpumask() */
>  	}
>  }
>  
> @@ -145,12 +146,14 @@ void on_cpumask(const cpumask_t *mask, void (*func)(void *data), void *data)
>  		smp_wait_for_event();
>  	for_each_cpu(cpu, mask)
>  		cpumask_clear_cpu(me, &on_cpu_info[cpu].waiters);
> +	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
>  }
>  
>  void on_cpu(int cpu, void (*func)(void *data), void *data)
>  {
>  	on_cpu_async(cpu, func, data);
>  	cpu_wait(cpu);
> +	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
>  }
>  
>  void on_cpus(void (*func)(void *data), void *data)
> -- 
> 2.47.0
> 
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

Re: [kvm-unit-tests PATCH v2 2/3] lib/on-cpus: Add barrier after func call

[Eric Auger]

On 10/31/24 13:39, Andrew Jones wrote:
> It's reasonable for users of on_cpu() and on_cpumask() to assume
> they can read data that 'func' has written when the call completes.
> Ensure the caller doesn't observe a completion (target cpus are again
> idle) without also being able to observe any writes which were made
> by func(). Do so by adding barriers to implement the following
>
>  target-cpu                                   calling-cpu
>  ----------                                   -----------
>  func() /* store something */                 /* wait for target to be idle */
>  smp_wmb();                                   smp_rmb();
>  set_cpu_idle(smp_processor_id(), true);      /* load what func() stored */
>
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  lib/on-cpus.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/on-cpus.c b/lib/on-cpus.c
> index f6072117fa1b..356f284be61b 100644
> --- a/lib/on-cpus.c
> +++ b/lib/on-cpus.c
> @@ -79,6 +79,7 @@ void do_idle(void)
>  			smp_wait_for_event();
>  		smp_rmb();
>  		on_cpu_info[cpu].func(on_cpu_info[cpu].data);
> +		smp_wmb(); /* pairs with the smp_rmb() in on_cpu() and on_cpumask() */
>  	}
>  }
>  
> @@ -145,12 +146,14 @@ void on_cpumask(const cpumask_t *mask, void (*func)(void *data), void *data)
>  		smp_wait_for_event();
>  	for_each_cpu(cpu, mask)
>  		cpumask_clear_cpu(me, &on_cpu_info[cpu].waiters);
> +	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
>  }
>  
>  void on_cpu(int cpu, void (*func)(void *data), void *data)
>  {
>  	on_cpu_async(cpu, func, data);
>  	cpu_wait(cpu);
> +	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
>  }
>  
>  void on_cpus(void (*func)(void *data), void *data)

[kvm-unit-tests PATCH v2 3/3] lib/on-cpus: Fix on_cpumask

[Andrew Jones]

on_cpumask should wait until the cpus in the mask, not including
the calling cpu, are idle. Checking the weight against nr_cpus
minus 1 only works when the mask is the same as the present mask.

Fixes: d012cfd5d309 ("lib/on-cpus: Introduce on_cpumask and on_cpumask_async")
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/cpumask.h | 14 ++++++++++++++
 lib/on-cpus.c | 17 ++++++++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/cpumask.h b/lib/cpumask.h
index e1e92aacd1f1..37d360786573 100644
--- a/lib/cpumask.h
+++ b/lib/cpumask.h
@@ -58,6 +58,20 @@ static inline void cpumask_clear(cpumask_t *mask)
 	memset(mask, 0, sizeof(*mask));
 }
 
+/* true if src1 is a subset of src2 */
+static inline bool cpumask_subset(const struct cpumask *src1, const struct cpumask *src2)
+{
+	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
+	int i;
+
+	for (i = 0; i < BIT_WORD(nr_cpus); ++i) {
+		if (cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i])
+			return false;
+	}
+
+	return !lastmask || !((cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i]) & lastmask);
+}
+
 static inline bool cpumask_empty(const cpumask_t *mask)
 {
 	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
diff --git a/lib/on-cpus.c b/lib/on-cpus.c
index 356f284be61b..889b6bc8a186 100644
--- a/lib/on-cpus.c
+++ b/lib/on-cpus.c
@@ -127,24 +127,23 @@ void on_cpumask_async(const cpumask_t *mask, void (*func)(void *data), void *dat
 void on_cpumask(const cpumask_t *mask, void (*func)(void *data), void *data)
 {
 	int cpu, me = smp_processor_id();
+	cpumask_t tmp;
 
-	for_each_cpu(cpu, mask) {
-		if (cpu == me)
-			continue;
+	cpumask_copy(&tmp, mask);
+	cpumask_clear_cpu(me, &tmp);
+
+	for_each_cpu(cpu, &tmp)
 		on_cpu_async(cpu, func, data);
-	}
 	if (cpumask_test_cpu(me, mask))
 		func(data);
 
-	for_each_cpu(cpu, mask) {
-		if (cpu == me)
-			continue;
+	for_each_cpu(cpu, &tmp) {
 		cpumask_set_cpu(me, &on_cpu_info[cpu].waiters);
 		deadlock_check(me, cpu);
 	}
-	while (cpumask_weight(&cpu_idle_mask) < nr_cpus - 1)
+	while (!cpumask_subset(&tmp, &cpu_idle_mask))
 		smp_wait_for_event();
-	for_each_cpu(cpu, mask)
+	for_each_cpu(cpu, &tmp)
 		cpumask_clear_cpu(me, &on_cpu_info[cpu].waiters);
 	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
 }
-- 
2.47.0

Re: [kvm-unit-tests PATCH v2 3/3] lib/on-cpus: Fix on_cpumask

[Eric Auger]

Hi Drew,

On 10/31/24 13:39, Andrew Jones wrote:
> on_cpumask should wait until the cpus in the mask, not including
> the calling cpu, are idle. Checking the weight against nr_cpus
> minus 1 only works when the mask is the same as the present mask.
>
> Fixes: d012cfd5d309 ("lib/on-cpus: Introduce on_cpumask and on_cpumask_async")
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

looks good to me as well

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  lib/cpumask.h | 14 ++++++++++++++
>  lib/on-cpus.c | 17 ++++++++---------
>  2 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/lib/cpumask.h b/lib/cpumask.h
> index e1e92aacd1f1..37d360786573 100644
> --- a/lib/cpumask.h
> +++ b/lib/cpumask.h
> @@ -58,6 +58,20 @@ static inline void cpumask_clear(cpumask_t *mask)
>  	memset(mask, 0, sizeof(*mask));
>  }
>  
> +/* true if src1 is a subset of src2 */
> +static inline bool cpumask_subset(const struct cpumask *src1, const struct cpumask *src2)
> +{
> +	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
> +	int i;
> +
> +	for (i = 0; i < BIT_WORD(nr_cpus); ++i) {
> +		if (cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i])
> +			return false;
> +	}
> +
> +	return !lastmask || !((cpumask_bits(src1)[i] & ~cpumask_bits(src2)[i]) & lastmask);
> +}
> +
>  static inline bool cpumask_empty(const cpumask_t *mask)
>  {
>  	unsigned long lastmask = BIT_MASK(nr_cpus) - 1;
> diff --git a/lib/on-cpus.c b/lib/on-cpus.c
> index 356f284be61b..889b6bc8a186 100644
> --- a/lib/on-cpus.c
> +++ b/lib/on-cpus.c
> @@ -127,24 +127,23 @@ void on_cpumask_async(const cpumask_t *mask, void (*func)(void *data), void *dat
>  void on_cpumask(const cpumask_t *mask, void (*func)(void *data), void *data)
>  {
>  	int cpu, me = smp_processor_id();
> +	cpumask_t tmp;
>  
> -	for_each_cpu(cpu, mask) {
> -		if (cpu == me)
> -			continue;
> +	cpumask_copy(&tmp, mask);
> +	cpumask_clear_cpu(me, &tmp);
> +
> +	for_each_cpu(cpu, &tmp)
>  		on_cpu_async(cpu, func, data);
> -	}
>  	if (cpumask_test_cpu(me, mask))
>  		func(data);
>  
> -	for_each_cpu(cpu, mask) {
> -		if (cpu == me)
> -			continue;
> +	for_each_cpu(cpu, &tmp) {
>  		cpumask_set_cpu(me, &on_cpu_info[cpu].waiters);
>  		deadlock_check(me, cpu);
>  	}
> -	while (cpumask_weight(&cpu_idle_mask) < nr_cpus - 1)
> +	while (!cpumask_subset(&tmp, &cpu_idle_mask))
>  		smp_wait_for_event();
> -	for_each_cpu(cpu, mask)
> +	for_each_cpu(cpu, &tmp)
>  		cpumask_clear_cpu(me, &on_cpu_info[cpu].waiters);
>  	smp_rmb(); /* pairs with the smp_wmb() in do_idle() */
>  }

Re: [kvm-unit-tests PATCH v2 0/3] lib/on-cpus: A couple of fixes

[Andrew Jones]

On Thu, Oct 31, 2024 at 01:39:49PM +0100, Andrew Jones wrote:
> While creating riscv SBI HSM tests a couple strange on-cpus behaviors
> were observed. Fix 'em.
> 
> v2:
>  - Added patch for barrier after func() [Alex]
>  - Improved commit message for patch1 [Alex]
> 
> Andrew Jones (3):
>   lib/on-cpus: Correct and simplify synchronization
>   lib/on-cpus: Add barrier after func call
>   lib/on-cpus: Fix on_cpumask
> 
>  lib/cpumask.h | 14 +++++++++++++
>  lib/on-cpus.c | 56 ++++++++++++++++++++-------------------------------
>  2 files changed, 36 insertions(+), 34 deletions(-)
> 
> -- 
> 2.47.0
>

Merged to master through riscv/queue.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 0/3] lib/on-cpus: A couple of fixes

[Alexandru Elisei]

Hi Drew,

On Mon, Nov 11, 2024 at 03:36:30PM +0100, Andrew Jones wrote:
> On Thu, Oct 31, 2024 at 01:39:49PM +0100, Andrew Jones wrote:
> > While creating riscv SBI HSM tests a couple strange on-cpus behaviors
> > were observed. Fix 'em.
> > 
> > v2:
> >  - Added patch for barrier after func() [Alex]
> >  - Improved commit message for patch1 [Alex]
> > 
> > Andrew Jones (3):
> >   lib/on-cpus: Correct and simplify synchronization
> >   lib/on-cpus: Add barrier after func call
> >   lib/on-cpus: Fix on_cpumask
> > 
> >  lib/cpumask.h | 14 +++++++++++++
> >  lib/on-cpus.c | 56 ++++++++++++++++++++-------------------------------
> >  2 files changed, 36 insertions(+), 34 deletions(-)
> > 
> > -- 
> > 2.47.0
> >
> 
> Merged to master through riscv/queue.

Sorry, I was busy with something else and I didn't have the time to review
the series until now.

I had a look and it looks good to me.

Thanks,
Alex