[LTP] [PATCH 1/4] tst_atomic: make tst_atomic_inc take a signed integer

[LTP] [PATCH 1/4] tst_atomic: make tst_atomic_inc take a signed integer

[Jan Stancek]

This is a preparation for upcoming patches, which add atomic_add_return(),
that takes signed integer.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 doc/test-writing-guidelines.txt | 2 +-
 include/tst_atomic.h            | 2 +-
 lib/newlib_tests/test08.c       | 2 +-
 lib/tst_test.c                  | 8 ++++----
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index d0b14084c90b..c0ef33afd5bb 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -780,7 +780,7 @@ the user supplied cleanup to the test library.
 
 static void cleanup(void)
 {
-	static unsigned int flag;
+	static int flag;
 
 	if (tst_atomic_inc(&flag) != 1)
 		pthread_exit(NULL);
diff --git a/include/tst_atomic.h b/include/tst_atomic.h
index 75c713d38d2a..40b2c581f19d 100644
--- a/include/tst_atomic.h
+++ b/include/tst_atomic.h
@@ -18,7 +18,7 @@
 #ifndef TST_ATOMIC_H__
 #define TST_ATOMIC_H__
 
-static inline unsigned int tst_atomic_inc(unsigned int *v)
+static inline unsigned int tst_atomic_inc(int *v)
 {
 	return __sync_add_and_fetch(v, 1);
 }
diff --git a/lib/newlib_tests/test08.c b/lib/newlib_tests/test08.c
index 8fefc182650d..0a2023119a71 100644
--- a/lib/newlib_tests/test08.c
+++ b/lib/newlib_tests/test08.c
@@ -35,7 +35,7 @@ static void setup(void)
 
 static void cleanup(void)
 {
-	static unsigned int flag;
+	static int flag;
 
 	/* Avoid subsequent threads to enter the cleanup */
 	if (tst_atomic_inc(&flag) != 1)
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 37265fe50951..66c0bc67127f 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -43,10 +43,10 @@ static float duration = -1;
 static pid_t main_pid;
 
 struct results {
-	unsigned int passed;
-	unsigned int skipped;
-	unsigned int failed;
-	unsigned int warnings;
+	int passed;
+	int skipped;
+	int failed;
+	int warnings;
 };
 
 static struct results *results;
-- 
1.8.3.1

[LTP] [PATCH 2/4] m4: add a check for __sync_add_and_fetch

[Jan Stancek]

Compilers older than gcc 4.1.2 do not provide __sync_add_and_fetch,
newer ones usually do, but it still may be missing for some targets.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 configure.ac                 |  1 +
 include/tst_atomic.h         | 14 +++++++++++++-
 m4/ltp-sync_add_and_fetch.m4 | 29 +++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)
 create mode 100644 m4/ltp-sync_add_and_fetch.m4

diff --git a/configure.ac b/configure.ac
index 2fb1ebc4a9c1..92ed5ec0e500 100644
--- a/configure.ac
+++ b/configure.ac
@@ -185,5 +185,6 @@ LTP_CHECK_KCMP_TYPE
 LTP_CHECK_PREADV
 LTP_CHECK_PWRITEV
 LTP_CHECK_EPOLL_PWAIT
+LTP_CHECK_SYNC_ADD_AND_FETCH
 
 AC_OUTPUT
diff --git a/include/tst_atomic.h b/include/tst_atomic.h
index 40b2c581f19d..b989c10cb23b 100644
--- a/include/tst_atomic.h
+++ b/include/tst_atomic.h
@@ -18,9 +18,21 @@
 #ifndef TST_ATOMIC_H__
 #define TST_ATOMIC_H__
 
+#include "config.h"
+
+#if HAVE_SYNC_ADD_AND_FETCH == 1
+static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
+{
+	return __sync_add_and_fetch(v, i);
+}
+#else
+#error Your compiler does not provide __sync_add_and_fetch and LTP\
+ implementation is missing for your architecture.
+#endif
+
 static inline unsigned int tst_atomic_inc(int *v)
 {
-	return __sync_add_and_fetch(v, 1);
+	return atomic_add_return(1, v);
 }
 
 #endif	/* TST_ATOMIC_H__ */
diff --git a/m4/ltp-sync_add_and_fetch.m4 b/m4/ltp-sync_add_and_fetch.m4
new file mode 100644
index 000000000000..b9e222589267
--- /dev/null
+++ b/m4/ltp-sync_add_and_fetch.m4
@@ -0,0 +1,29 @@
+dnl
+dnl Copyright (c) Linux Test Project, 2016
+dnl
+dnl This program is free software;  you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 2 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY;  without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+dnl the GNU General Public License for more details.
+dnl
+
+AC_DEFUN([LTP_CHECK_SYNC_ADD_AND_FETCH],[dnl
+	AC_MSG_CHECKING([for __sync_add_and_fetch])
+	AC_LINK_IFELSE([AC_LANG_SOURCE([
+int main(void) {
+	int i = 0;
+	return __sync_add_and_fetch(&i, 1);
+}])],[has_saac="yes"])
+
+if test "x$has_saac" = xyes; then
+	AC_DEFINE(HAVE_SYNC_ADD_AND_FETCH,1,[Define to 1 if you have __sync_add_and_fetch])
+	AC_MSG_RESULT(yes)
+else
+	AC_MSG_RESULT(no)
+fi
+])
-- 
1.8.3.1

[LTP] [PATCH 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x

[Jan Stancek]

In case __sync_add_and_fetch is not provided by compiler, we
try to supply our own implementation.

It has been taken from kernel sources, by compiling a small
piece of code (atomic_add_return()) with "gcc -E" and applying
some formatting to make it more readable/pretty.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 include/tst_atomic.h | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/include/tst_atomic.h b/include/tst_atomic.h
index b989c10cb23b..e842f5500876 100644
--- a/include/tst_atomic.h
+++ b/include/tst_atomic.h
@@ -21,11 +21,86 @@
 #include "config.h"
 
 #if HAVE_SYNC_ADD_AND_FETCH == 1
+#define HAVE_ATOMIC_ADD_RETURN 1
 static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
 {
 	return __sync_add_and_fetch(v, i);
 }
-#else
+
+#else /* HAVE_SYNC_ADD_AND_FETCH == 1 */
+
+#if defined(__i386__) || defined(__x86_64__)
+#define HAVE_ATOMIC_ADD_RETURN 1
+extern void __xadd_wrong_size(void);
+static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
+{
+	int __ret = i;
+
+	switch (sizeof(*v)) {
+	case 1:
+		asm volatile ("lock; xaddb %b0, %1\n"
+			: "+q" (__ret), "+m" (*v) : : "memory", "cc");
+		break;
+	case 2:
+		asm volatile ("lock; xaddw %w0, %1\n"
+			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
+		break;
+	case 4:
+		asm volatile ("lock; xaddl %0, %1\n"
+			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
+		break;
+	case 8:
+		asm volatile ("lock; xaddq %q0, %1\n"
+			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
+		break;
+	default:
+		__xadd_wrong_size();
+	}
+	return i + __ret;
+}
+#endif
+
+#if defined(__powerpc__) || defined(__powerpc64__)
+#define HAVE_ATOMIC_ADD_RETURN 1
+static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
+{
+	int t;
+
+	asm volatile(
+		"	sync\n"
+		"1:	lwarx	%0,0,%2		# atomic_add_return\n"
+		"	add %0,%1,%0\n"
+		"	stwcx.	%0,0,%2 \n"
+		"	bne-	1b\n"
+		"	sync\n"
+		: "=&r" (t)
+		: "r" (i), "r" (v)
+		: "cc", "memory");
+	return t;
+}
+#endif
+
+#if defined(__s390__) || defined(__s390x__)
+#define HAVE_ATOMIC_ADD_RETURN 1
+static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
+{
+	int old_val, new_val;
+
+	asm volatile(
+		"	l	%0,%2\n"
+		"0:	lr	%1,%0\n"
+		"	ar	%1,%3\n"
+		"	cs	%0,%1,%2\n"
+		"	jl	0b"
+		: "=&d" (old_val), "=&d" (new_val), "+Q" (*v)
+		: "d" (i)
+		: "cc", "memory");
+	return old_val + i;
+}
+#endif
+#endif /* HAVE_SYNC_ADD_AND_FETCH == 1 */
+
+#if !defined(HAVE_ATOMIC_ADD_RETURN)
 #error Your compiler does not provide __sync_add_and_fetch and LTP\
  implementation is missing for your architecture.
 #endif
-- 
1.8.3.1

[LTP] [PATCH 4/4] tst_atomic: add test for tst_atomic_inc

[Jan Stancek]

Spawn a lot of threads and run tst_atomic_inc in a loop.
At the end we expect value to be THREADS * ITERATIONS.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 lib/newlib_tests/Makefile |  1 +
 lib/newlib_tests/test09.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 lib/newlib_tests/test09.c

diff --git a/lib/newlib_tests/Makefile b/lib/newlib_tests/Makefile
index 5771712fab10..0e4eeb83b6d4 100644
--- a/lib/newlib_tests/Makefile
+++ b/lib/newlib_tests/Makefile
@@ -6,5 +6,6 @@ CFLAGS			+= -W -Wall
 LDLIBS			+= -lltp
 
 test08: CFLAGS+=-pthread
+test09: CFLAGS+=-pthread
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/lib/newlib_tests/test09.c b/lib/newlib_tests/test09.c
new file mode 100644
index 000000000000..7b063b55cea4
--- /dev/null
+++ b/lib/newlib_tests/test09.c
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2016 Linux Test Project
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/*
+ * Test that tst_atomic_inc works as expected.
+ */
+
+#include <pthread.h>
+#include "tst_test.h"
+
+#define THREADS 64
+#define ITERATIONS 100000
+
+static int atomic;
+
+static void *worker(void *id)
+{
+	int i;
+
+	(void) id;
+	for (i = 0; i < ITERATIONS; i++)
+		tst_atomic_inc(&atomic);
+
+	return NULL;
+}
+
+static void do_test(void)
+{
+	long i;
+	pthread_t threads[THREADS];
+
+	for (i = 0; i < THREADS; i++)
+		pthread_create(threads+i, NULL, worker, (void *)i);
+
+	for (i = 0; i < THREADS; i++) {
+		tst_res(TINFO, "Joining thread %li", i);
+		pthread_join(threads[i], NULL);
+	}
+
+	if (atomic == THREADS * ITERATIONS)
+		tst_res(TPASS, "Atomic working as expected");
+	else
+		tst_res(TFAIL, "Atomic does not have expected value");
+}
+
+static struct tst_test test = {
+	.tid = "test09",
+	.test_all = do_test,
+	.forks_child = 1
+};
-- 
1.8.3.1

[LTP] [PATCH 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x

[Cyril Hrubis]

Hi!
> +#if defined(__i386__) || defined(__x86_64__)
> +#define HAVE_ATOMIC_ADD_RETURN 1
> +extern void __xadd_wrong_size(void);
> +static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
> +{
> +	int __ret = i;
> +
> +	switch (sizeof(*v)) {
> +	case 1:
> +		asm volatile ("lock; xaddb %b0, %1\n"
> +			: "+q" (__ret), "+m" (*v) : : "memory", "cc");
> +		break;
> +	case 2:
> +		asm volatile ("lock; xaddw %w0, %1\n"
> +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> +		break;

Do we really need byte and word version? As far as I can tell int is 4
bytes on x86 and x86_64 and unlike kernel where this is a macro we
cannot pass anything else than int.

> +	case 4:
> +		asm volatile ("lock; xaddl %0, %1\n"
> +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> +		break;
> +	case 8:
> +		asm volatile ("lock; xaddq %q0, %1\n"
> +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> +		break;

The same goes for the quad version here.

> +	default:
> +		__xadd_wrong_size();

So this supposedly causes linker error by trying to link nonexistent
function, right?

I guess that we should either add nonexistent to the function name or
short commment with explanation.

Also it should start with tst_ in order to avoid teoretical collisions
with system functions.

> +	}
> +	return i + __ret;
> +}
> +#endif
> +
> +#if defined(__powerpc__) || defined(__powerpc64__)
> +#define HAVE_ATOMIC_ADD_RETURN 1
> +static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
> +{
> +	int t;
> +
> +	asm volatile(
> +		"	sync\n"
> +		"1:	lwarx	%0,0,%2		# atomic_add_return\n"
> +		"	add %0,%1,%0\n"
> +		"	stwcx.	%0,0,%2 \n"
> +		"	bne-	1b\n"
> +		"	sync\n"
> +		: "=&r" (t)
> +		: "r" (i), "r" (v)
> +		: "cc", "memory");
> +	return t;
> +}
> +#endif
> +
> +#if defined(__s390__) || defined(__s390x__)
> +#define HAVE_ATOMIC_ADD_RETURN 1
> +static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
> +{
> +	int old_val, new_val;
> +
> +	asm volatile(
> +		"	l	%0,%2\n"
> +		"0:	lr	%1,%0\n"
> +		"	ar	%1,%3\n"
> +		"	cs	%0,%1,%2\n"
> +		"	jl	0b"
> +		: "=&d" (old_val), "=&d" (new_val), "+Q" (*v)
> +		: "d" (i)
> +		: "cc", "memory");
> +	return old_val + i;
> +}
> +#endif
> +#endif /* HAVE_SYNC_ADD_AND_FETCH == 1 */
> +
> +#if !defined(HAVE_ATOMIC_ADD_RETURN)
>  #error Your compiler does not provide __sync_add_and_fetch and LTP\
>   implementation is missing for your architecture.
>  #endif

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/4] m4: add a check for __sync_add_and_fetch

[Cyril Hrubis]

Hi!
> +#if HAVE_SYNC_ADD_AND_FETCH == 1
> +static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
> +{
> +	return __sync_add_and_fetch(v, i);
> +}
> +#else
> +#error Your compiler does not provide __sync_add_and_fetch and LTP\
> + implementation is missing for your architecture.
> +#endif
> +
>  static inline unsigned int tst_atomic_inc(int *v)
>  {
> -	return __sync_add_and_fetch(v, 1);
> +	return atomic_add_return(1, v);
>  }

I guess that we can make it a bit easier by defining the
tst_atomic_inc() directly, i.e.

#if HAVE_SYNC_ADD_AND_FETCH
static inline int tst_atomic_inc(int *v)
{
	return __sync_add_and_fetch(v, 1);
}
#else
...


>  #endif	/* TST_ATOMIC_H__ */
> diff --git a/m4/ltp-sync_add_and_fetch.m4 b/m4/ltp-sync_add_and_fetch.m4
> new file mode 100644
> index 000000000000..b9e222589267
> --- /dev/null
> +++ b/m4/ltp-sync_add_and_fetch.m4
> @@ -0,0 +1,29 @@
> +dnl
> +dnl Copyright (c) Linux Test Project, 2016
> +dnl
> +dnl This program is free software;  you can redistribute it and/or modify
> +dnl it under the terms of the GNU General Public License as published by
> +dnl the Free Software Foundation; either version 2 of the License, or
> +dnl (at your option) any later version.
> +dnl
> +dnl This program is distributed in the hope that it will be useful,
> +dnl but WITHOUT ANY WARRANTY;  without even the implied warranty of
> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> +dnl the GNU General Public License for more details.
> +dnl
> +
> +AC_DEFUN([LTP_CHECK_SYNC_ADD_AND_FETCH],[dnl
> +	AC_MSG_CHECKING([for __sync_add_and_fetch])
> +	AC_LINK_IFELSE([AC_LANG_SOURCE([
> +int main(void) {
> +	int i = 0;
> +	return __sync_add_and_fetch(&i, 1);
> +}])],[has_saac="yes"])
> +
> +if test "x$has_saac" = xyes; then
> +	AC_DEFINE(HAVE_SYNC_ADD_AND_FETCH,1,[Define to 1 if you have __sync_add_and_fetch])
> +	AC_MSG_RESULT(yes)
> +else
> +	AC_MSG_RESULT(no)
> +fi
> +])
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/4] tst_atomic: make tst_atomic_inc take a signed integer

[Cyril Hrubis]

Hi!
> This is a preparation for upcoming patches, which add atomic_add_return(),
> that takes signed integer.

What is the problem with unsigned integer?

> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  doc/test-writing-guidelines.txt | 2 +-
>  include/tst_atomic.h            | 2 +-
>  lib/newlib_tests/test08.c       | 2 +-
>  lib/tst_test.c                  | 8 ++++----
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> index d0b14084c90b..c0ef33afd5bb 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -780,7 +780,7 @@ the user supplied cleanup to the test library.
>  
>  static void cleanup(void)
>  {
> -	static unsigned int flag;
> +	static int flag;
>  
>  	if (tst_atomic_inc(&flag) != 1)
>  		pthread_exit(NULL);
> diff --git a/include/tst_atomic.h b/include/tst_atomic.h
> index 75c713d38d2a..40b2c581f19d 100644
> --- a/include/tst_atomic.h
> +++ b/include/tst_atomic.h
> @@ -18,7 +18,7 @@
>  #ifndef TST_ATOMIC_H__
>  #define TST_ATOMIC_H__
>  
> -static inline unsigned int tst_atomic_inc(unsigned int *v)
> +static inline unsigned int tst_atomic_inc(int *v)
                    ^
		 This should be removed as well, right?

>  {
>  	return __sync_add_and_fetch(v, 1);
>  }
> diff --git a/lib/newlib_tests/test08.c b/lib/newlib_tests/test08.c
> index 8fefc182650d..0a2023119a71 100644
> --- a/lib/newlib_tests/test08.c
> +++ b/lib/newlib_tests/test08.c
> @@ -35,7 +35,7 @@ static void setup(void)
>  
>  static void cleanup(void)
>  {
> -	static unsigned int flag;
> +	static int flag;
>  
>  	/* Avoid subsequent threads to enter the cleanup */
>  	if (tst_atomic_inc(&flag) != 1)
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 37265fe50951..66c0bc67127f 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -43,10 +43,10 @@ static float duration = -1;
>  static pid_t main_pid;
>  
>  struct results {
> -	unsigned int passed;
> -	unsigned int skipped;
> -	unsigned int failed;
> -	unsigned int warnings;
> +	int passed;
> +	int skipped;
> +	int failed;
> +	int warnings;
>  };

We print these values with %u in do_exit(), that should be updated as
well.

>  static struct results *results;
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 4/4] tst_atomic: add test for tst_atomic_inc

[Cyril Hrubis]

Hi!
This is nice to have ;)

> +/*
> + * Copyright (c) 2016 Linux Test Project
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +/*
> + * Test that tst_atomic_inc works as expected.
> + */
> +
> +#include <pthread.h>
> +#include "tst_test.h"
> +
> +#define THREADS 64
> +#define ITERATIONS 100000
> +
> +static int atomic;
> +
> +static void *worker(void *id)
> +{
> +	int i;
> +
> +	(void) id;
> +	for (i = 0; i < ITERATIONS; i++)
> +		tst_atomic_inc(&atomic);
> +
> +	return NULL;
> +}
> +
> +static void do_test(void)
> +{
> +	long i;
> +	pthread_t threads[THREADS];
> +
> +	for (i = 0; i < THREADS; i++)
> +		pthread_create(threads+i, NULL, worker, (void *)i);
> +
> +	for (i = 0; i < THREADS; i++) {
> +		tst_res(TINFO, "Joining thread %li", i);
> +		pthread_join(threads[i], NULL);
> +	}
> +
> +	if (atomic == THREADS * ITERATIONS)
> +		tst_res(TPASS, "Atomic working as expected");
> +	else
> +		tst_res(TFAIL, "Atomic does not have expected value");
> +}
> +
> +static struct tst_test test = {
> +	.tid = "test09",
> +	.test_all = do_test,
> +	.forks_child = 1

Looks like the forks_child is copy&pasted from test08 which was
copy&pasted test07 which really forks a child. I will fix test08.c and
you should remove it from this one as well.
`
> +};

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/4] tst_atomic: make tst_atomic_inc take a signed integer

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 13 April, 2016 4:07:02 PM
> Subject: Re: [LTP] [PATCH 1/4] tst_atomic: make tst_atomic_inc take a signed integer
> 
> Hi!
> > This is a preparation for upcoming patches, which add atomic_add_return(),
> > that takes signed integer.
> 
> What is the problem with unsigned integer?

All atomic code in kernel is int based. I wanted to match that as closely
as possible in our code.

> 
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  doc/test-writing-guidelines.txt | 2 +-
> >  include/tst_atomic.h            | 2 +-
> >  lib/newlib_tests/test08.c       | 2 +-
> >  lib/tst_test.c                  | 8 ++++----
> >  4 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/doc/test-writing-guidelines.txt
> > b/doc/test-writing-guidelines.txt
> > index d0b14084c90b..c0ef33afd5bb 100644
> > --- a/doc/test-writing-guidelines.txt
> > +++ b/doc/test-writing-guidelines.txt
> > @@ -780,7 +780,7 @@ the user supplied cleanup to the test library.
> >  
> >  static void cleanup(void)
> >  {
> > -	static unsigned int flag;
> > +	static int flag;
> >  
> >  	if (tst_atomic_inc(&flag) != 1)
> >  		pthread_exit(NULL);
> > diff --git a/include/tst_atomic.h b/include/tst_atomic.h
> > index 75c713d38d2a..40b2c581f19d 100644
> > --- a/include/tst_atomic.h
> > +++ b/include/tst_atomic.h
> > @@ -18,7 +18,7 @@
> >  #ifndef TST_ATOMIC_H__
> >  #define TST_ATOMIC_H__
> >  
> > -static inline unsigned int tst_atomic_inc(unsigned int *v)
> > +static inline unsigned int tst_atomic_inc(int *v)
>                     ^
> 		 This should be removed as well, right?

Correct. I missed that.

> 
> >  {
> >  	return __sync_add_and_fetch(v, 1);
> >  }
> > diff --git a/lib/newlib_tests/test08.c b/lib/newlib_tests/test08.c
> > index 8fefc182650d..0a2023119a71 100644
> > --- a/lib/newlib_tests/test08.c
> > +++ b/lib/newlib_tests/test08.c
> > @@ -35,7 +35,7 @@ static void setup(void)
> >  
> >  static void cleanup(void)
> >  {
> > -	static unsigned int flag;
> > +	static int flag;
> >  
> >  	/* Avoid subsequent threads to enter the cleanup */
> >  	if (tst_atomic_inc(&flag) != 1)
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 37265fe50951..66c0bc67127f 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -43,10 +43,10 @@ static float duration = -1;
> >  static pid_t main_pid;
> >  
> >  struct results {
> > -	unsigned int passed;
> > -	unsigned int skipped;
> > -	unsigned int failed;
> > -	unsigned int warnings;
> > +	int passed;
> > +	int skipped;
> > +	int failed;
> > +	int warnings;
> >  };
> 
> We print these values with %u in do_exit(), that should be updated as
> well.

This as well.

> 
> >  static struct results *results;
> > --
> > 1.8.3.1
> > 
> > 
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 13 April, 2016 3:59:58 PM
> Subject: Re: [LTP] [PATCH 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x
> 
> Hi!
> > +#if defined(__i386__) || defined(__x86_64__)
> > +#define HAVE_ATOMIC_ADD_RETURN 1
> > +extern void __xadd_wrong_size(void);
> > +static inline __attribute__((always_inline)) int atomic_add_return(int i,
> > int *v)
> > +{
> > +	int __ret = i;
> > +
> > +	switch (sizeof(*v)) {
> > +	case 1:
> > +		asm volatile ("lock; xaddb %b0, %1\n"
> > +			: "+q" (__ret), "+m" (*v) : : "memory", "cc");
> > +		break;
> > +	case 2:
> > +		asm volatile ("lock; xaddw %w0, %1\n"
> > +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> > +		break;
> 
> Do we really need byte and word version? As far as I can tell int is 4
> bytes on x86 and x86_64 and unlike kernel where this is a macro we
> cannot pass anything else than int.

Not really, it's again case where I tried to preserve original kernel code.

> 
> > +	case 4:
> > +		asm volatile ("lock; xaddl %0, %1\n"
> > +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> > +		break;
> > +	case 8:
> > +		asm volatile ("lock; xaddq %q0, %1\n"
> > +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> > +		break;
> 
> The same goes for the quad version here.
> 
> > +	default:
> > +		__xadd_wrong_size();
> 
> So this supposedly causes linker error by trying to link nonexistent
> function, right?
> 
> I guess that we should either add nonexistent to the function name or
> short commment with explanation.

I can add both.

> 
> Also it should start with tst_ in order to avoid teoretical collisions
> with system functions.
> 
> > +	}
> > +	return i + __ret;
> > +}
> > +#endif
> > +
> > +#if defined(__powerpc__) || defined(__powerpc64__)
> > +#define HAVE_ATOMIC_ADD_RETURN 1
> > +static inline __attribute__((always_inline)) int atomic_add_return(int i,
> > int *v)
> > +{
> > +	int t;
> > +
> > +	asm volatile(
> > +		"	sync\n"
> > +		"1:	lwarx	%0,0,%2		# atomic_add_return\n"
> > +		"	add %0,%1,%0\n"
> > +		"	stwcx.	%0,0,%2 \n"
> > +		"	bne-	1b\n"
> > +		"	sync\n"
> > +		: "=&r" (t)
> > +		: "r" (i), "r" (v)
> > +		: "cc", "memory");
> > +	return t;
> > +}
> > +#endif
> > +
> > +#if defined(__s390__) || defined(__s390x__)
> > +#define HAVE_ATOMIC_ADD_RETURN 1
> > +static inline __attribute__((always_inline)) int atomic_add_return(int i,
> > int *v)
> > +{
> > +	int old_val, new_val;
> > +
> > +	asm volatile(
> > +		"	l	%0,%2\n"
> > +		"0:	lr	%1,%0\n"
> > +		"	ar	%1,%3\n"
> > +		"	cs	%0,%1,%2\n"
> > +		"	jl	0b"
> > +		: "=&d" (old_val), "=&d" (new_val), "+Q" (*v)
> > +		: "d" (i)
> > +		: "cc", "memory");
> > +	return old_val + i;
> > +}
> > +#endif
> > +#endif /* HAVE_SYNC_ADD_AND_FETCH == 1 */
> > +
> > +#if !defined(HAVE_ATOMIC_ADD_RETURN)
> >  #error Your compiler does not provide __sync_add_and_fetch and LTP\
> >   implementation is missing for your architecture.
> >  #endif
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x

[Cyril Hrubis]

Hi!
> > > +#if defined(__i386__) || defined(__x86_64__)
> > > +#define HAVE_ATOMIC_ADD_RETURN 1
> > > +extern void __xadd_wrong_size(void);
> > > +static inline __attribute__((always_inline)) int atomic_add_return(int i,
> > > int *v)
> > > +{
> > > +	int __ret = i;
> > > +
> > > +	switch (sizeof(*v)) {
> > > +	case 1:
> > > +		asm volatile ("lock; xaddb %b0, %1\n"
> > > +			: "+q" (__ret), "+m" (*v) : : "memory", "cc");
> > > +		break;
> > > +	case 2:
> > > +		asm volatile ("lock; xaddw %w0, %1\n"
> > > +			: "+r" (__ret), "+m" (*v) : : "memory", "cc");
> > > +		break;
> > 
> > Do we really need byte and word version? As far as I can tell int is 4
> > bytes on x86 and x86_64 and unlike kernel where this is a macro we
> > cannot pass anything else than int.

I would say that we should remove this part as it's effectively dead
code. But if you really think that we should preserve it I'm fine with
that as well.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/4] tst_atomic: make tst_atomic_inc take a signed integer

[Cyril Hrubis]

Hi!
> > > This is a preparation for upcoming patches, which add atomic_add_return(),
> > > that takes signed integer.
> > 
> > What is the problem with unsigned integer?
> 
> All atomic code in kernel is int based. I wanted to match that as closely
> as possible in our code.

I on x86 that should not matter as addition for numbers in two's
complement is just the same for both signed and unsigned numbers and the
instruction just sets both carry and overflow flags and
programmer/compiler just uses one of them depending on if the number is
supposed to be signed or unsigned.

But I'm not expert on powerpc assembly nor s390 assembly and I certainly
do not want to become one :). So let's follow what kernel does here.

-- 
Cyril Hrubis
chrubis@suse.cz