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

[LTP] [PATCH v2 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(),
based on kernel function of same name. Kernel's atomic_t is int based,
so let's match that.

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                  | 16 ++++++++--------
 4 files changed, 11 insertions(+), 11 deletions(-)

Changes in v2:
- fixed tst_atomic_inc return type
- fixed tst_exit printf flags

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..2ec9f91318ea 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 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..b8ec246414d1 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;
@@ -442,10 +442,10 @@ static void do_exit(void)
 	int ret = 0;
 
 	printf("\nSummary:\n");
-	printf("passed   %u\n", results->passed);
-	printf("failed   %u\n", results->failed);
-	printf("skipped  %u\n", results->skipped);
-	printf("warnings %u\n", results->warnings);
+	printf("passed   %d\n", results->passed);
+	printf("failed   %d\n", results->failed);
+	printf("skipped  %d\n", results->skipped);
+	printf("warnings %d\n", results->warnings);
 
 	if (results->failed)
 		ret |= TFAIL;
-- 
1.8.3.1

[LTP] [PATCH v2 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

Changes in v2:
- no changes. I kept single definition of tst_atomic_inc()
  out of all #ifdefs as opposed to making 2 definitions
  for both (!)HAVE_SYNC_ADD_AND_FETCH cases.
  Now that we have a more generic version (atomic_add_return),
  that is compiler/arch dependent, it seems more tidy to me to
  keep just one definition of atomic_inc.
  
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 2ec9f91318ea..046eb160ad28 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 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 v2 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)

As this is publicly exported interface I would prefer adding tst_ prefix
to the function name.

> +{
> +	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 int tst_atomic_inc(int *v)
>  {
> -	return __sync_add_and_fetch(v, 1);
> +	return atomic_add_return(1, v);
>  }

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 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 4.5 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 | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

Changes in v2:
- atomic_add_return for x86 simplified, with original kernel macro
  kept as comment.
- added comments were assembly inline was taken from

diff --git a/include/tst_atomic.h b/include/tst_atomic.h
index 046eb160ad28..e6b432d63ea1 100644
--- a/include/tst_atomic.h
+++ b/include/tst_atomic.h
@@ -21,11 +21,98 @@
 #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
+static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
+{
+	int __ret = i;
+
+	/*
+	 * taken from arch/x86/include/asm/cmpxchg.h
+	 * Since we always pass int sized parameter, we can simplify it
+	 * and cherry-pick only that specific case.
+	 *
+	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();
+	}
+	*/
+	asm volatile ("lock; xaddl %0, %1\n"
+		: "+r" (__ret), "+m" (*v) : : "memory", "cc");
+
+	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;
+
+	/* taken from arch/powerpc/include/asm/atomic.h */
+	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;
+
+	/* taken from arch/s390/include/asm/atomic.h */
+	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 v2 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x

[Cyril Hrubis]

Hi!
>  #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
> +static inline __attribute__((always_inline)) int atomic_add_return(int i, int *v)
> +{

Maybe we can make the ifdefs a bit more readable by indenting them with
spaces after the hash.

#else
# if defined(__i386__) || defined(__x86_64__)
#  define HAVE_ATOMIC_ADD_RETURN 1

...

# endif


Otherwise it looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 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: Thursday, 14 April, 2016 4:36:23 PM
> Subject: Re: [LTP] [PATCH v2 3/4] tst_atomic: add atomic_add_return for x86/64, ppc/64 and s390/x
> 
> Hi!
> >  #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
> > +static inline __attribute__((always_inline)) int atomic_add_return(int i,
> > int *v)
> > +{
> 
> Maybe we can make the ifdefs a bit more readable by indenting them with
> spaces after the hash.
> 
> #else
> # if defined(__i386__) || defined(__x86_64__)
> #  define HAVE_ATOMIC_ADD_RETURN 1
> 
> ...
> 
> # endif

I have re-arranged this in v3, so that there is less nesting,
and also dropped HAVE_ATOMIC_ADD_RETURN.

I also made some other small tweaks, changes are described
in each patch.

[LTP] [PATCH v2 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 | 61 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 lib/newlib_tests/test09.c

Changes in v2:
- removed .forks_child

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..0674754f3770
--- /dev/null
+++ b/lib/newlib_tests/test09.c
@@ -0,0 +1,61 @@
+/*
+ * 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,
+};
-- 
1.8.3.1

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

[Cyril Hrubis]

Hi!
Acked.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Cyril Hrubis]

Hi!
Looks good, acked.

-- 
Cyril Hrubis
chrubis@suse.cz