[kvm-unit-tests PATCH v1 0/3] provide asm-generic spinlock

[kvm-unit-tests PATCH v1 0/3] provide asm-generic spinlock

[David Hildenbrand]

Let's introduce a simple generic version that should be sufficient for
our purposes.

David Hildenbrand (3):
  lib: provide generic spinlock
  ppc64: use asm-generic spinlock
  x86: use asm-generic spinlock

 lib/asm-generic/spinlock.h | 16 +++++++++++++++-
 lib/ppc64/asm/spinlock.h   |  9 ++-------
 lib/ppc64/spinlock.c       | 18 ------------------
 lib/x86/asm/spinlock.h     |  9 ++-------
 lib/x86/smp.c              | 16 ----------------
 powerpc/Makefile.ppc64     |  1 -
 6 files changed, 19 insertions(+), 50 deletions(-)
 delete mode 100644 lib/ppc64/spinlock.c

-- 
2.9.3

[kvm-unit-tests PATCH v1 1/3] lib: provide generic spinlock

[David Hildenbrand]

Let's provide a basic lock implementation that should work on most
architectures.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/asm-generic/spinlock.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/asm-generic/spinlock.h b/lib/asm-generic/spinlock.h
index 3141744..e8c3a58 100644
--- a/lib/asm-generic/spinlock.h
+++ b/lib/asm-generic/spinlock.h
@@ -1,4 +1,18 @@
 #ifndef _ASM_GENERIC_SPINLOCK_H_
 #define _ASM_GENERIC_SPINLOCK_H_
-#error need architecture specific asm/spinlock.h
+
+struct spinlock {
+    unsigned int v;
+};
+
+static inline void spin_lock(struct spinlock *lock)
+{
+	while (!__sync_bool_compare_and_swap(&lock->v, 0, 1));
+}
+
+static inline void spin_unlock(struct spinlock *lock)
+{
+	__sync_bool_compare_and_swap(&lock->v, 1, 0);
+}
+
 #endif
-- 
2.9.3

[kvm-unit-tests PATCH v1 2/3] ppc64: use asm-generic spinlock

[David Hildenbrand]

Since the ppc64 implementation is currently only a defunc dummy
implementation, let's replace it by the generic one.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/ppc64/asm/spinlock.h |  9 ++-------
 lib/ppc64/spinlock.c     | 18 ------------------
 powerpc/Makefile.ppc64   |  1 -
 3 files changed, 2 insertions(+), 26 deletions(-)
 delete mode 100644 lib/ppc64/spinlock.c

diff --git a/lib/ppc64/asm/spinlock.h b/lib/ppc64/asm/spinlock.h
index 002cdb1..da1d9d0 100644
--- a/lib/ppc64/asm/spinlock.h
+++ b/lib/ppc64/asm/spinlock.h
@@ -1,11 +1,6 @@
+#include <asm-generic/spinlock.h>
+
 #ifndef _ASMPPC64_SPINLOCK_H_
 #define _ASMPPC64_SPINLOCK_H_
 
-struct spinlock {
-	int v;
-};
-
-extern void spin_lock(struct spinlock *lock);
-extern void spin_unlock(struct spinlock *lock);
-
 #endif /* _ASMPPC64_SPINLOCK_H_ */
diff --git a/lib/ppc64/spinlock.c b/lib/ppc64/spinlock.c
deleted file mode 100644
index 1b26ee1..0000000
--- a/lib/ppc64/spinlock.c
+++ /dev/null
@@ -1,18 +0,0 @@
-/*
- * ppc64 (dummy) spinlock implementation
- *
- * This code is free software; you can redistribute it and/or modify it
- * under the terms of the GNU Library General Public License version 2.
- */
-
-#include <asm/spinlock.h>
-
-void spin_lock(struct spinlock *lock)
-{
-        lock->v = 1;
-}
-
-void spin_unlock(struct spinlock *lock)
-{
-        lock->v = 0;
-}
diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
index 3da3a83..17ece66 100644
--- a/powerpc/Makefile.ppc64
+++ b/powerpc/Makefile.ppc64
@@ -15,7 +15,6 @@ endif
 
 cstart.o = $(TEST_DIR)/cstart64.o
 reloc.o  = $(TEST_DIR)/reloc64.o
-cflatobjs += lib/ppc64/spinlock.o
 
 # ppc64 specific tests
 tests =
-- 
2.9.3

[kvm-unit-tests PATCH v1 3/3] x86: use asm-generic spinlock

[David Hildenbrand]

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/x86/asm/spinlock.h |  9 ++-------
 lib/x86/smp.c          | 16 ----------------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/lib/x86/asm/spinlock.h b/lib/x86/asm/spinlock.h
index 4b0cb33..e302b56 100644
--- a/lib/x86/asm/spinlock.h
+++ b/lib/x86/asm/spinlock.h
@@ -1,11 +1,6 @@
+#include <asm-generic/spinlock.h>
+
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
-struct spinlock {
-    int v;
-};
-
-void spin_lock(struct spinlock *lock);
-void spin_unlock(struct spinlock *lock);
-
 #endif
diff --git a/lib/x86/smp.c b/lib/x86/smp.c
index 1eb49f2..4bdbeae 100644
--- a/lib/x86/smp.c
+++ b/lib/x86/smp.c
@@ -43,22 +43,6 @@ asm (
 #endif
      );
 
-void spin_lock(struct spinlock *lock)
-{
-    int v = 1;
-
-    do {
-	asm volatile ("xchg %1, %0" : "+m"(lock->v), "+r"(v));
-    } while (v);
-    asm volatile ("" : : : "memory");
-}
-
-void spin_unlock(struct spinlock *lock)
-{
-    asm volatile ("" : : : "memory");
-    lock->v = 0;
-}
-
 int cpu_count(void)
 {
     return _cpu_count;
-- 
2.9.3

Re: [kvm-unit-tests PATCH v1 2/3] ppc64: use asm-generic spinlock

[Thomas Huth]

On 12.05.2017 12:20, David Hildenbrand wrote:
> Since the ppc64 implementation is currently only a defunc dummy
> implementation, let's replace it by the generic one.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/ppc64/asm/spinlock.h |  9 ++-------
>  lib/ppc64/spinlock.c     | 18 ------------------
>  powerpc/Makefile.ppc64   |  1 -
>  3 files changed, 2 insertions(+), 26 deletions(-)
>  delete mode 100644 lib/ppc64/spinlock.c
> 
> diff --git a/lib/ppc64/asm/spinlock.h b/lib/ppc64/asm/spinlock.h
> index 002cdb1..da1d9d0 100644
> --- a/lib/ppc64/asm/spinlock.h
> +++ b/lib/ppc64/asm/spinlock.h
> @@ -1,11 +1,6 @@
> +#include <asm-generic/spinlock.h>

In case you respin this series: I think it would be nicer to put the
#include within the header guards below.

>  #ifndef _ASMPPC64_SPINLOCK_H_
>  #define _ASMPPC64_SPINLOCK_H_
>  
> -struct spinlock {
> -	int v;
> -};
> -
> -extern void spin_lock(struct spinlock *lock);
> -extern void spin_unlock(struct spinlock *lock);
> -
>  #endif /* _ASMPPC64_SPINLOCK_H_ */

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 1/3] lib: provide generic spinlock

[Thomas Huth]

On 12.05.2017 12:20, David Hildenbrand wrote:
> Let's provide a basic lock implementation that should work on most
> architectures.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/asm-generic/spinlock.h | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/asm-generic/spinlock.h b/lib/asm-generic/spinlock.h
> index 3141744..e8c3a58 100644
> --- a/lib/asm-generic/spinlock.h
> +++ b/lib/asm-generic/spinlock.h
> @@ -1,4 +1,18 @@
>  #ifndef _ASM_GENERIC_SPINLOCK_H_
>  #define _ASM_GENERIC_SPINLOCK_H_
> -#error need architecture specific asm/spinlock.h
> +
> +struct spinlock {
> +    unsigned int v;
> +};
> +
> +static inline void spin_lock(struct spinlock *lock)
> +{
> +	while (!__sync_bool_compare_and_swap(&lock->v, 0, 1));
> +}
> +
> +static inline void spin_unlock(struct spinlock *lock)
> +{
> +	__sync_bool_compare_and_swap(&lock->v, 1, 0);
> +}
> +
>  #endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 1/3] lib: provide generic spinlock

[Radim Krčmář]

2017-05-12 12:20+0200, David Hildenbrand:
> Let's provide a basic lock implementation that should work on most
> architectures.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  lib/asm-generic/spinlock.h | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/asm-generic/spinlock.h b/lib/asm-generic/spinlock.h
> index 3141744..e8c3a58 100644
> --- a/lib/asm-generic/spinlock.h
> +++ b/lib/asm-generic/spinlock.h
> @@ -1,4 +1,18 @@
>  #ifndef _ASM_GENERIC_SPINLOCK_H_
>  #define _ASM_GENERIC_SPINLOCK_H_
> -#error need architecture specific asm/spinlock.h
> +
> +struct spinlock {
> +    unsigned int v;
> +};
> +
> +static inline void spin_lock(struct spinlock *lock)
> +{
> +	while (!__sync_bool_compare_and_swap(&lock->v, 0, 1));
> +}
> +
> +static inline void spin_unlock(struct spinlock *lock)
> +{
> +	__sync_bool_compare_and_swap(&lock->v, 1, 0);
> +}

x86 would be better with __sync_lock_test_and_set() and
__sync_lock_release() as they generate the same code we have now,
instead of two locked cmpxchgs.

GCC mentions that some targets might have problems with that, but they
seem to fall back to boolean value and compare-and-swap.

Any reason to avoid "while(__sync_lock_test_and_set(&lock->v, 1));"?

Thanks.

Re: [kvm-unit-tests PATCH v1 1/3] lib: provide generic spinlock

[Paolo Bonzini]

On 12/05/2017 18:43, Radim Krčmář wrote:
> 2017-05-12 12:20+0200, David Hildenbrand:
>> Let's provide a basic lock implementation that should work on most
>> architectures.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  lib/asm-generic/spinlock.h | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/asm-generic/spinlock.h b/lib/asm-generic/spinlock.h
>> index 3141744..e8c3a58 100644
>> --- a/lib/asm-generic/spinlock.h
>> +++ b/lib/asm-generic/spinlock.h
>> @@ -1,4 +1,18 @@
>>  #ifndef _ASM_GENERIC_SPINLOCK_H_
>>  #define _ASM_GENERIC_SPINLOCK_H_
>> -#error need architecture specific asm/spinlock.h
>> +
>> +struct spinlock {
>> +    unsigned int v;
>> +};
>> +
>> +static inline void spin_lock(struct spinlock *lock)
>> +{
>> +	while (!__sync_bool_compare_and_swap(&lock->v, 0, 1));
>> +}
>> +
>> +static inline void spin_unlock(struct spinlock *lock)
>> +{
>> +	__sync_bool_compare_and_swap(&lock->v, 1, 0);
>> +}
> 
> x86 would be better with __sync_lock_test_and_set() and
> __sync_lock_release() as they generate the same code we have now,
> instead of two locked cmpxchgs.

I agree these are a better match.

Paolo

> GCC mentions that some targets might have problems with that, but they
> seem to fall back to boolean value and compare-and-swap.
> 
> Any reason to avoid "while(__sync_lock_test_and_set(&lock->v, 1));"?
> 
> Thanks.
> 

Re: [kvm-unit-tests PATCH v1 1/3] lib: provide generic spinlock

[David Hildenbrand]

On 12.05.2017 18:43, Radim Krčmář wrote:
> 2017-05-12 12:20+0200, David Hildenbrand:
>> Let's provide a basic lock implementation that should work on most
>> architectures.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  lib/asm-generic/spinlock.h | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/asm-generic/spinlock.h b/lib/asm-generic/spinlock.h
>> index 3141744..e8c3a58 100644
>> --- a/lib/asm-generic/spinlock.h
>> +++ b/lib/asm-generic/spinlock.h
>> @@ -1,4 +1,18 @@
>>  #ifndef _ASM_GENERIC_SPINLOCK_H_
>>  #define _ASM_GENERIC_SPINLOCK_H_
>> -#error need architecture specific asm/spinlock.h
>> +
>> +struct spinlock {
>> +    unsigned int v;
>> +};
>> +
>> +static inline void spin_lock(struct spinlock *lock)
>> +{
>> +	while (!__sync_bool_compare_and_swap(&lock->v, 0, 1));
>> +}
>> +
>> +static inline void spin_unlock(struct spinlock *lock)
>> +{
>> +	__sync_bool_compare_and_swap(&lock->v, 1, 0);
>> +}
> 
> x86 would be better with __sync_lock_test_and_set() and
> __sync_lock_release() as they generate the same code we have now,
> instead of two locked cmpxchgs.

Both should work, however using your pair looks nicer and also seems to
work for x86, powerpc and s390x (at least GCC doesn't spit fire).

So I'll resend, thanks!

> 
> GCC mentions that some targets might have problems with that, but they
> seem to fall back to boolean value and compare-and-swap.
> 
> Any reason to avoid "while(__sync_lock_test_and_set(&lock->v, 1));"?

I haven't found anything speaking against it. There are even multiple
articles suggesting to either use __sync_lock_test_and_set or
__sync_bool_compare_and_swap in a loop for exactly that purpose.

> 
> Thanks.
> 


-- 

Thanks,

David