[LTP] [PATCH v2 0/3] syscalls: add tgkill test-cases

[LTP] [PATCH v2 0/3] syscalls: add tgkill test-cases

[Sumit Garg]

This patch-set is inherited from Greg which adds test-cases for tgkill
syscall.

Changes in v2:
- Free allocated memory for threads.
- Remove redundant call to stop_threads().
- Apply reviewed-by tag.

Greg Hackmann (3):
  syscalls/tgkill01: add new test
  syscalls/tgkill02: add new test
  syscalls/tgkill03: add new test

 runtest/syscalls                            |   4 +
 testcases/kernel/syscalls/tgkill/.gitignore |   3 +
 testcases/kernel/syscalls/tgkill/Makefile   |  10 ++
 testcases/kernel/syscalls/tgkill/tgkill.h   |  22 +++++
 testcases/kernel/syscalls/tgkill/tgkill01.c | 141 ++++++++++++++++++++++++++++
 testcases/kernel/syscalls/tgkill/tgkill02.c |  80 ++++++++++++++++
 testcases/kernel/syscalls/tgkill/tgkill03.c | 122 ++++++++++++++++++++++++
 7 files changed, 382 insertions(+)
 create mode 100644 testcases/kernel/syscalls/tgkill/.gitignore
 create mode 100644 testcases/kernel/syscalls/tgkill/Makefile
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill.h
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill01.c
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill02.c
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill03.c

-- 
2.7.4

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Sumit Garg]

From: Greg Hackmann <ghackmann@google.com>

tgkill() delivers a signal to a specific thread.  Test this by
installing a SIGUSR1 handler which records the current pthread ID.
Start a number of threads in parallel, then one-by-one call tgkill(...,
tid, SIGUSR1) and check that the expected pthread ID was recorded.

Signed-off-by: Greg Hackmann <ghackmann@google.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Li Wang <liwang@redhat.com>
---
 runtest/syscalls                            |   2 +
 testcases/kernel/syscalls/tgkill/.gitignore |   1 +
 testcases/kernel/syscalls/tgkill/Makefile   |  10 ++
 testcases/kernel/syscalls/tgkill/tgkill.h   |  22 +++++
 testcases/kernel/syscalls/tgkill/tgkill01.c | 141 ++++++++++++++++++++++++++++
 5 files changed, 176 insertions(+)
 create mode 100644 testcases/kernel/syscalls/tgkill/.gitignore
 create mode 100644 testcases/kernel/syscalls/tgkill/Makefile
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill.h
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill01.c

diff --git a/runtest/syscalls b/runtest/syscalls
index d752dba..8cd99e0 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1399,6 +1399,8 @@ syslog10 syslog10
 syslog11 syslog11
 syslog12 syslog12
 
+tgkill01 tgkill01
+
 time01 time01
 time02 time02
 
diff --git a/testcases/kernel/syscalls/tgkill/.gitignore b/testcases/kernel/syscalls/tgkill/.gitignore
new file mode 100644
index 0000000..f4566fd
--- /dev/null
+++ b/testcases/kernel/syscalls/tgkill/.gitignore
@@ -0,0 +1 @@
+tgkill01
diff --git a/testcases/kernel/syscalls/tgkill/Makefile b/testcases/kernel/syscalls/tgkill/Makefile
new file mode 100644
index 0000000..a51080c
--- /dev/null
+++ b/testcases/kernel/syscalls/tgkill/Makefile
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2018 Google, Inc.
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
+
+LDLIBS			+= -pthread
diff --git a/testcases/kernel/syscalls/tgkill/tgkill.h b/testcases/kernel/syscalls/tgkill/tgkill.h
new file mode 100644
index 0000000..a7d96f4
--- /dev/null
+++ b/testcases/kernel/syscalls/tgkill/tgkill.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 Google, Inc.
+ */
+
+#ifndef TGKILL_H
+#define TGKILL_H
+
+#include "config.h"
+#include "lapi/syscalls.h"
+
+static inline int sys_tgkill(int tgid, int tid, int sig)
+{
+	return tst_syscall(__NR_tgkill, tgid, tid, sig);
+}
+
+static inline pid_t sys_gettid(void)
+{
+	return tst_syscall(__NR_gettid);
+}
+
+#endif /* TGKILL_H */
diff --git a/testcases/kernel/syscalls/tgkill/tgkill01.c b/testcases/kernel/syscalls/tgkill/tgkill01.c
new file mode 100644
index 0000000..acc9977
--- /dev/null
+++ b/testcases/kernel/syscalls/tgkill/tgkill01.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 Google, Inc.
+ *
+ * tgkill() delivers a signal to a specific thread.  Test this by installing
+ * a SIGUSR1 handler which records the current pthread ID.  Start a number
+ * of threads in parallel, then one-by-one call tgkill(..., tid, SIGUSR1)
+ * and check that the expected pthread ID was recorded.
+ */
+
+#include <pthread.h>
+#include <stdlib.h>
+
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
+#include "tgkill.h"
+
+struct thread_state {
+	pthread_t thread;
+	pid_t tid;
+};
+
+static char *str_threads;
+static int n_threads = 10;
+static struct thread_state *threads;
+
+static pthread_t sigusr1_thread;
+
+static int test_running;
+static pthread_cond_t test_running_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t test_running_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static void sigusr1_handler(int signum __attribute__((unused)))
+{
+	sigusr1_thread = pthread_self();
+}
+
+static void *thread_func(void *arg)
+{
+	struct thread_state *thread = arg;
+
+	/**
+	 * There is no standard way to map pthread -> tid, so we will have the
+	 * child stash its own tid then notify the parent that the stashed tid
+	 * is available.
+	 */
+	thread->tid = sys_gettid();
+
+	TST_CHECKPOINT_WAKE(0);
+
+	pthread_mutex_lock(&test_running_mutex);
+	while (test_running)
+		pthread_cond_wait(&test_running_cond, &test_running_mutex);
+	pthread_mutex_unlock(&test_running_mutex);
+
+	return arg;
+}
+
+static void start_thread(struct thread_state *thread)
+{
+	SAFE_PTHREAD_CREATE(&thread->thread, NULL, thread_func, thread);
+
+	TST_CHECKPOINT_WAIT(0);
+}
+
+static void stop_threads(void)
+{
+	int i;
+
+	test_running = 0;
+	pthread_cond_broadcast(&test_running_cond);
+
+	for (i = 0; i < n_threads; i++) {
+		if (threads[i].tid == -1)
+			continue;
+
+		SAFE_PTHREAD_JOIN(threads[i].thread, NULL);
+		threads[i].tid = -1;
+	}
+
+	if (threads)
+		free(threads);
+}
+
+static void run(void)
+{
+	int i;
+
+	for (i = 0; i < n_threads; i++)
+		threads[i].tid = -1;
+
+	test_running = 1;
+	for (i = 0; i < n_threads; i++)
+		start_thread(&threads[i]);
+
+	for (i = 0; i < n_threads; i++) {
+		sigusr1_thread = pthread_self();
+
+		TEST(sys_tgkill(getpid(), threads[i].tid, SIGUSR1));
+		if (TST_RET) {
+			tst_res(TFAIL | TTERRNO, "tgkill() failed");
+			return;
+		}
+
+		while (pthread_equal(sigusr1_thread, pthread_self()))
+			usleep(1000);
+
+		if (!pthread_equal(sigusr1_thread, threads[i].thread)) {
+			tst_res(TFAIL, "SIGUSR1 delivered to wrong thread");
+			return;
+		}
+	}
+
+	tst_res(TPASS, "SIGUSR1 delivered to correct threads");
+}
+
+static void setup(void)
+{
+	if (tst_parse_int(str_threads, &n_threads, 1, INT_MAX))
+		tst_brk(TBROK, "Invalid number of threads '%s'", str_threads);
+
+	threads = SAFE_MALLOC(sizeof(*threads) * n_threads);
+
+	struct sigaction sigusr1 = {
+		.sa_handler = sigusr1_handler,
+	};
+	SAFE_SIGACTION(SIGUSR1, &sigusr1, NULL);
+}
+
+static struct tst_option options[] = {
+	{"t:", &str_threads, "-t       Number of threads (default 10)"},
+	{NULL, NULL, NULL},
+};
+
+static struct tst_test test = {
+	.options = options,
+	.needs_checkpoints = 1,
+	.setup = setup,
+	.test_all = run,
+	.cleanup = stop_threads,
+};
-- 
2.7.4

[LTP] [PATCH v2 2/3] syscalls/tgkill02: add new test

[Sumit Garg]

From: Greg Hackmann <ghackmann@google.com>

tgkill() should fail with EAGAIN when RLIMIT_SIGPENDING is reached with
a real-time signal.  Test this by starting a child thread with SIGRTMIN
blocked and a limit of 0 pending signals, then attempting to deliver
SIGRTMIN from the parent thread.

Signed-off-by: Greg Hackmann <ghackmann@google.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Li Wang <liwang@redhat.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/tgkill/.gitignore |  1 +
 testcases/kernel/syscalls/tgkill/tgkill02.c | 80 +++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 8cd99e0..1f4a2da 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1400,6 +1400,7 @@ syslog11 syslog11
 syslog12 syslog12
 
 tgkill01 tgkill01
+tgkill02 tgkill02
 
 time01 time01
 time02 time02
diff --git a/testcases/kernel/syscalls/tgkill/.gitignore b/testcases/kernel/syscalls/tgkill/.gitignore
index f4566fd..42be2bb 100644
--- a/testcases/kernel/syscalls/tgkill/.gitignore
+++ b/testcases/kernel/syscalls/tgkill/.gitignore
@@ -1 +1,2 @@
 tgkill01
+tgkill02
diff --git a/testcases/kernel/syscalls/tgkill/tgkill02.c b/testcases/kernel/syscalls/tgkill/tgkill02.c
new file mode 100644
index 0000000..a121992
--- /dev/null
+++ b/testcases/kernel/syscalls/tgkill/tgkill02.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 Google, Inc.
+ *
+ * tgkill() should fail with EAGAIN when RLIMIT_SIGPENDING is reached with a
+ * real-time signal.  Test this by starting a child thread with SIGRTMIN
+ * blocked and a limit of 0 pending signals, then attempting to deliver
+ * SIGRTMIN from the parent thread.
+ */
+
+#include <pthread.h>
+#include <signal.h>
+
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
+#include "tgkill.h"
+
+static int test_running;
+static pthread_cond_t test_running_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t test_running_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static void *thread_func(void *arg)
+{
+	const struct rlimit sigpending = {
+		.rlim_cur = 0,
+		.rlim_max = 0,
+	};
+	sigset_t sigrtmin;
+	int err;
+	pid_t *tid = arg;
+
+	sigemptyset(&sigrtmin);
+	sigaddset(&sigrtmin, SIGRTMIN);
+
+	err = pthread_sigmask(SIG_BLOCK, &sigrtmin, NULL);
+	if (err)
+		tst_brk(TBROK, "pthread_sigmask() failed: %s",
+			tst_strerrno(err));
+
+	SAFE_SETRLIMIT(RLIMIT_SIGPENDING, &sigpending);
+	*tid = sys_gettid();
+
+	TST_CHECKPOINT_WAKE(0);
+
+	pthread_mutex_lock(&test_running_mutex);
+	while (test_running)
+		pthread_cond_wait(&test_running_cond, &test_running_mutex);
+	pthread_mutex_unlock(&test_running_mutex);
+
+	return arg;
+}
+
+static void run(void)
+{
+	pthread_t thread;
+	pid_t tid = -1;
+
+	test_running = 1;
+
+	SAFE_PTHREAD_CREATE(&thread, NULL, thread_func, &tid);
+
+	TST_CHECKPOINT_WAIT(0);
+
+	TEST(sys_tgkill(getpid(), tid, SIGRTMIN));
+	if (TST_RET && TST_ERR == EAGAIN)
+		tst_res(TPASS, "tgkill() failed with EAGAIN as expected");
+	else
+		tst_res(TFAIL | TTERRNO,
+			"tgkill() should have failed with EAGAIN");
+
+	test_running = 0;
+	pthread_cond_broadcast(&test_running_cond);
+
+	SAFE_PTHREAD_JOIN(thread, NULL);
+}
+
+static struct tst_test test = {
+	.needs_checkpoints = 1,
+	.test_all = run,
+};
-- 
2.7.4

[LTP] [PATCH v2 3/3] syscalls/tgkill03: add new test

[Sumit Garg]

From: Greg Hackmann <ghackmann@google.com>

Test simple tgkill() error cases.

Signed-off-by: Greg Hackmann <ghackmann@google.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Li Wang <liwang@redhat.com>
---
 runtest/syscalls                            |   1 +
 testcases/kernel/syscalls/tgkill/.gitignore |   1 +
 testcases/kernel/syscalls/tgkill/tgkill03.c | 122 ++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 testcases/kernel/syscalls/tgkill/tgkill03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 1f4a2da..b92244d 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1401,6 +1401,7 @@ syslog12 syslog12
 
 tgkill01 tgkill01
 tgkill02 tgkill02
+tgkill03 tgkill03
 
 time01 time01
 time02 time02
diff --git a/testcases/kernel/syscalls/tgkill/.gitignore b/testcases/kernel/syscalls/tgkill/.gitignore
index 42be2bb..a6d2299 100644
--- a/testcases/kernel/syscalls/tgkill/.gitignore
+++ b/testcases/kernel/syscalls/tgkill/.gitignore
@@ -1,2 +1,3 @@
 tgkill01
 tgkill02
+tgkill03
diff --git a/testcases/kernel/syscalls/tgkill/tgkill03.c b/testcases/kernel/syscalls/tgkill/tgkill03.c
new file mode 100644
index 0000000..3597d1b
--- /dev/null
+++ b/testcases/kernel/syscalls/tgkill/tgkill03.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 Google, Inc.
+ *
+ * Test simple tgkill() error cases.
+ */
+
+#include <pthread.h>
+#include <pwd.h>
+#include <sys/types.h>
+
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
+#include "tgkill.h"
+
+static pthread_t child_thread;
+
+static pid_t parent_tgid;
+static pid_t parent_tid;
+static pid_t child_tid;
+static pid_t defunct_tid;
+
+static const int invalid_pid = -1;
+
+static int test_running;
+static pthread_cond_t test_running_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t test_running_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static void *child_thread_func(void *arg)
+{
+	child_tid = sys_gettid();
+
+	TST_CHECKPOINT_WAKE(0);
+
+	pthread_mutex_lock(&test_running_mutex);
+	while (test_running)
+		pthread_cond_wait(&test_running_cond, &test_running_mutex);
+	pthread_mutex_unlock(&test_running_mutex);
+
+	return arg;
+}
+
+static void *defunct_thread_func(void *arg)
+{
+	defunct_tid = sys_gettid();
+
+	return arg;
+}
+
+static void setup(void)
+{
+	sigset_t sigusr1;
+	pthread_t defunct_thread;
+
+	sigemptyset(&sigusr1);
+	sigaddset(&sigusr1, SIGUSR1);
+	pthread_sigmask(SIG_BLOCK, &sigusr1, NULL);
+
+	parent_tgid = getpid();
+	parent_tid = sys_gettid();
+
+	test_running = 1;
+
+	SAFE_PTHREAD_CREATE(&child_thread, NULL, child_thread_func, NULL);
+
+	TST_CHECKPOINT_WAIT(0);
+
+	SAFE_PTHREAD_CREATE(&defunct_thread, NULL, defunct_thread_func, NULL);
+
+	SAFE_PTHREAD_JOIN(defunct_thread, NULL);
+}
+
+static void cleanup(void)
+{
+	test_running = 0;
+	pthread_cond_broadcast(&test_running_cond);
+
+	SAFE_PTHREAD_JOIN(child_thread, NULL);
+}
+
+static const struct testcase {
+	const char *desc;
+	const int *tgid;
+	const int *tid;
+	const int sig;
+	const int err;
+} testcases[] = {
+	{ "Invalid tgid", &invalid_pid, &parent_tid, SIGUSR1, EINVAL },
+	{ "Invalid tid", &parent_tgid, &invalid_pid, SIGUSR1, EINVAL },
+	{ "Invalid signal", &parent_tgid, &parent_tid, -1, EINVAL },
+	{ "Defunct thread ID", &parent_tgid, &defunct_tid, SIGUSR1, ESRCH },
+	{ "Valid tgkill call", &parent_tgid, &child_tid, SIGUSR1, 0 },
+};
+
+static void run(unsigned int i)
+{
+	const struct testcase *tc = &testcases[i];
+
+	TEST(sys_tgkill(*tc->tgid, *tc->tid, tc->sig));
+	if (tc->err) {
+		if (TST_RET < 0 && TST_ERR == tc->err)
+			tst_res(TPASS | TTERRNO, "%s failed as expected",
+				tc->desc);
+		else
+			tst_res(TFAIL | TTERRNO,
+				"%s should have failed with %s", tc->desc,
+				tst_strerrno(tc->err));
+	} else {
+		if (TST_RET == 0)
+			tst_res(TPASS, "%s succeeded", tc->desc);
+		else
+			tst_res(TFAIL | TTERRNO, "%s failed", tc->desc);
+	}
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(testcases),
+	.needs_checkpoints = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test = run,
+};
-- 
2.7.4

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Cyril Hrubis]

Hi!
The test itself looks good, my only concern is actually that checkpoints
cannot be used for the keeping the thread asleep during the test.
However I can easily add one function to the futex library:

TST_CHECKPOINT_SLEEP(id)

That would cause the thread to wait on the checkpoint until:

* we were woken up
* we timeouted

Which would basically loop on tst_checkpoint_wait() and retry in case of
EINTR.

Maybe it would be a good idea to retry on EINTR in the
TST_CHECKPOINT_WAIT(), then we could easily use that one here as well.
I'm not sure though if there are tests that depends on checkpoints being
interrupted by signals though, I would have to check.

For the second part we already have a function to wake all threads
waiting on checkpoint, but we have to specify exact number of threads to
wait for, which is there in order to avoid race coditions (i.e. thread
was not sleeping yet at the time we tried to wake it). So we would have
to count the number of threads we want to wake before the call to the
TST_CHECKPOINT_WAKE2().

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Sumit Garg]

On Thu, 14 Mar 2019 at 17:53, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> The test itself looks good, my only concern is actually that checkpoints
> cannot be used for the keeping the thread asleep during the test.
> However I can easily add one function to the futex library:
>
> TST_CHECKPOINT_SLEEP(id)
>
> That would cause the thread to wait on the checkpoint until:
>
> * we were woken up
> * we timeouted
>
> Which would basically loop on tst_checkpoint_wait() and retry in case of
> EINTR.
>

I am not sure how we would manage actual "msec_timeout" in case we get
EINTR and need to retry again as we may need to take care of elapsed
time till we receive asynchronous signal.

-Sumit

> Maybe it would be a good idea to retry on EINTR in the
> TST_CHECKPOINT_WAIT(), then we could easily use that one here as well.
> I'm not sure though if there are tests that depends on checkpoints being
> interrupted by signals though, I would have to check.
>
> For the second part we already have a function to wake all threads
> waiting on checkpoint, but we have to specify exact number of threads to
> wait for, which is there in order to avoid race coditions (i.e. thread
> was not sleeping yet at the time we tried to wake it). So we would have
> to count the number of threads we want to wake before the call to the
> TST_CHECKPOINT_WAKE2().
>
> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Cyril Hrubis]

Hi!
> I am not sure how we would manage actual "msec_timeout" in case we get
> EINTR and need to retry again as we may need to take care of elapsed
> time till we receive asynchronous signal.

I would have just restarted the timeout after we got signal, the worst case
that can happen is that in an unlikely case we will send a signals fast enough
so that the checkpoint will never timeout. But even then the test library will
timeout and would kill the process anyways.

Another option is to switch checkpoints so that they use absolute timeout and
pass clock_gettime() + msec_timeout as timeout.

I would go for something as simple as:

diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c
index 5455d0378..5e5b11496 100644
--- a/lib/tst_checkpoint.c
+++ b/lib/tst_checkpoint.c
@@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno,
 int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
 {
        struct timespec timeout;
+       int ret;
 
        if (id >= tst_max_futexes) {
                errno = EOVERFLOW;
@@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
        timeout.tv_sec = msec_timeout/1000;
        timeout.tv_nsec = (msec_timeout%1000) * 1000000;
 
-       return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
-                      tst_futexes[id], &timeout);
+       do {
+               ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
+                             tst_futexes[id], &timeout);
+       } while (ret == -1 && errno == EINTR);
+
+       return ret;
 }

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Li Wang]

On Thu, Mar 14, 2019 at 9:59 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > I am not sure how we would manage actual "msec_timeout" in case we get
> > EINTR and need to retry again as we may need to take care of elapsed
> > time till we receive asynchronous signal.
>
> I would have just restarted the timeout after we got signal, the worst case
> that can happen is that in an unlikely case we will send a signals fast
> enough
>

Maybe we can print something useful there at least for friendly debugging
if that unlikely case happens.

> so that the checkpoint will never timeout. But even then the test library
> will
> timeout and would kill the process anyways.
>
> Another option is to switch checkpoints so that they use absolute timeout
> and
> pass clock_gettime() + msec_timeout as timeout.
>
> I would go for something as simple as:
>
> diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c
> index 5455d0378..5e5b11496 100644
> --- a/lib/tst_checkpoint.c
> +++ b/lib/tst_checkpoint.c
> @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int
> lineno,
>  int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
>  {
>         struct timespec timeout;
> +       int ret;
>
>         if (id >= tst_max_futexes) {
>                 errno = EOVERFLOW;
> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int
> msec_timeout)
>         timeout.tv_sec = msec_timeout/1000;
>         timeout.tv_nsec = (msec_timeout%1000) * 1000000;
>
> -       return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> -                      tst_futexes[id], &timeout);
> +       do {
> +               ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> +                             tst_futexes[id], &timeout);

    if (ret == -1 && errno == EINTR)
        tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a
signal, retry again");

+       } while (ret == -1 && errno == EINTR);
> +
> +       return ret;
>  }
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190315/e104e07e/attachment-0001.html>

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Sumit Garg]

On Thu, 14 Mar 2019 at 19:29, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > I am not sure how we would manage actual "msec_timeout" in case we get
> > EINTR and need to retry again as we may need to take care of elapsed
> > time till we receive asynchronous signal.
>
> I would have just restarted the timeout after we got signal, the worst case
> that can happen is that in an unlikely case we will send a signals fast enough
> so that the checkpoint will never timeout. But even then the test library will
> timeout and would kill the process anyways.
>
> Another option is to switch checkpoints so that they use absolute timeout and
> pass clock_gettime() + msec_timeout as timeout.
>
> I would go for something as simple as:
>
> diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c
> index 5455d0378..5e5b11496 100644
> --- a/lib/tst_checkpoint.c
> +++ b/lib/tst_checkpoint.c
> @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno,
>  int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
>  {
>         struct timespec timeout;
> +       int ret;
>
>         if (id >= tst_max_futexes) {
>                 errno = EOVERFLOW;
> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
>         timeout.tv_sec = msec_timeout/1000;
>         timeout.tv_nsec = (msec_timeout%1000) * 1000000;
>
> -       return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> -                      tst_futexes[id], &timeout);
> +       do {
> +               ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> +                             tst_futexes[id], &timeout);
> +       } while (ret == -1 && errno == EINTR);
> +
> +       return ret;
>  }
>

Wouldn't this loop be more appropriate in
"tst_safe_checkpoint_wait()"? As at later stage we may have tests that
depends on checkpoints being interrupted by signals and could directly
use "tst_checkpoint_wait()".

-Sumit

> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Sumit Garg]

On Fri, 15 Mar 2019 at 13:15, Li Wang <liwang@redhat.com> wrote:
>
>
>
> On Thu, Mar 14, 2019 at 9:59 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>>
>> Hi!
>> > I am not sure how we would manage actual "msec_timeout" in case we get
>> > EINTR and need to retry again as we may need to take care of elapsed
>> > time till we receive asynchronous signal.
>>
>> I would have just restarted the timeout after we got signal, the worst case
>> that can happen is that in an unlikely case we will send a signals fast enough
>
>
> Maybe we can print something useful there at least for friendly debugging if that unlikely case happens.
>>
>> so that the checkpoint will never timeout. But even then the test library will
>> timeout and would kill the process anyways.
>>
>> Another option is to switch checkpoints so that they use absolute timeout and
>> pass clock_gettime() + msec_timeout as timeout.
>>
>> I would go for something as simple as:
>>
>> diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c
>> index 5455d0378..5e5b11496 100644
>> --- a/lib/tst_checkpoint.c
>> +++ b/lib/tst_checkpoint.c
>> @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno,
>>  int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
>>  {
>>         struct timespec timeout;
>> +       int ret;
>>
>>         if (id >= tst_max_futexes) {
>>                 errno = EOVERFLOW;
>> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
>>         timeout.tv_sec = msec_timeout/1000;
>>         timeout.tv_nsec = (msec_timeout%1000) * 1000000;
>>
>> -       return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
>> -                      tst_futexes[id], &timeout);
>> +       do {
>> +               ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
>> +                             tst_futexes[id], &timeout);
>
>     if (ret == -1 && errno == EINTR)
>         tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again");
>

I am not sure if this warning message is desired for test-cases which
needs to wait on checkpoints irrespective of signals like this
tgkill01 test-case.

-Sumit

>> +       } while (ret == -1 && errno == EINTR);
>> +
>> +       return ret;
>>  }
>>
>> --
>> Cyril Hrubis
>> chrubis@suse.cz
>
>
>
> --
> Regards,
> Li Wang

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Cyril Hrubis]

Hi!
> >>  int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
> >>  {
> >>         struct timespec timeout;
> >> +       int ret;
> >>
> >>         if (id >= tst_max_futexes) {
> >>                 errno = EOVERFLOW;
> >> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
> >>         timeout.tv_sec = msec_timeout/1000;
> >>         timeout.tv_nsec = (msec_timeout%1000) * 1000000;
> >>
> >> -       return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> >> -                      tst_futexes[id], &timeout);
> >> +       do {
> >> +               ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> >> +                             tst_futexes[id], &timeout);
> >
> >     if (ret == -1 && errno == EINTR)
> >         tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again");
> >
> 
> I am not sure if this warning message is desired for test-cases which
> needs to wait on checkpoints irrespective of signals like this
> tgkill01 test-case.

Agreed, it's not an error condition, it's just a coincidence that most
of the tests does not get signals when they sleep on futex, otherwise
thing would crash and burn. So I would argue that retrying on EINTR is
actually a bug fix rather than anything else.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Cyril Hrubis]

Hi!
> Wouldn't this loop be more appropriate in
> "tst_safe_checkpoint_wait()"? As at later stage we may have tests that
> depends on checkpoints being interrupted by signals and could directly
> use "tst_checkpoint_wait()".

The tst_checkpoint_wait() has a single use in the source tree and that
is testcases/lib/tst_checkpoint.c which is binary wrapper around
checkpoints so that we can use them in shell scripts as well, which is
pretty cool btw. And I think that we should retry on EINTR there as
well.

Also there does not seem to be test relying on checkpoints being
interrupted by signals and I would like to avoid this pattern ideally as
asynchronous events such as signals interrupting functions is something
rather counter intuitive.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Li Wang]

On Fri, Mar 15, 2019 at 6:09 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > >>  int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)
> > >>  {
> > >>         struct timespec timeout;
> > >> +       int ret;
> > >>
> > >>         if (id >= tst_max_futexes) {
> > >>                 errno = EOVERFLOW;
> > >> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned
> int msec_timeout)
> > >>         timeout.tv_sec = msec_timeout/1000;
> > >>         timeout.tv_nsec = (msec_timeout%1000) * 1000000;
> > >>
> > >> -       return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> > >> -                      tst_futexes[id], &timeout);
> > >> +       do {
> > >> +               ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,
> > >> +                             tst_futexes[id], &timeout);
> > >
> > >     if (ret == -1 && errno == EINTR)
> > >         tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted
> by a signal, retry again");
> > >
> >
> > I am not sure if this warning message is desired for test-cases which
> > needs to wait on checkpoints irrespective of signals like this
> > tgkill01 test-case.
>
> Agreed, it's not an error condition, it's just a coincidence that most
> of the tests does not get signals when they sleep on futex, otherwise
> thing would crash and burn. So I would argue that retrying on EINTR is
> actually a bug fix rather than anything else.
>

Okay, here I'm not insist to print the warning. But it's only use for hint
on that worst situation which you were mentioned. If the checkpoint got
signal leads to never timeout and test eventually killed by test lib. That
would hard to know what happened at that moment. My concern is the
situation is hard to reproduce later so just want to print more useful
messeges:).

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190315/5caa91e0/attachment-0001.html>

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Cyril Hrubis]

Hi!
> > > I am not sure if this warning message is desired for test-cases which
> > > needs to wait on checkpoints irrespective of signals like this
> > > tgkill01 test-case.
> >
> > Agreed, it's not an error condition, it's just a coincidence that most
> > of the tests does not get signals when they sleep on futex, otherwise
> > thing would crash and burn. So I would argue that retrying on EINTR is
> > actually a bug fix rather than anything else.
> >
> 
> Okay, here I'm not insist to print the warning. But it's only use for hint
> on that worst situation which you were mentioned. If the checkpoint got
> signal leads to never timeout and test eventually killed by test lib. That
> would hard to know what happened at that moment. My concern is the
> situation is hard to reproduce later so just want to print more useful
> messeges:).

As for now that's only a hypotetical corner case, someone would have to
send signals to such process sleeping on the checkpoint in a loop for
that to happen. The problem is that printing any messages when
checkpoint was interrupted by signal would lead to even greater
confusion for tests that actually have to send signals to such
processes.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Sumit Garg]

On Fri, 15 Mar 2019 at 15:48, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > Wouldn't this loop be more appropriate in
> > "tst_safe_checkpoint_wait()"? As at later stage we may have tests that
> > depends on checkpoints being interrupted by signals and could directly
> > use "tst_checkpoint_wait()".
>
> The tst_checkpoint_wait() has a single use in the source tree and that
> is testcases/lib/tst_checkpoint.c which is binary wrapper around
> checkpoints so that we can use them in shell scripts as well, which is
> pretty cool btw. And I think that we should retry on EINTR there as
> well.

Ah, I see.

>
> Also there does not seem to be test relying on checkpoints being
> interrupted by signals and I would like to avoid this pattern ideally as
> asynchronous events such as signals interrupting functions is something
> rather counter intuitive.
>

Ok got it. Should I pick up this change as separate patch in this
patch-set or would you like to commit it and then I would update
patch-set to use these checkpoints?

-Sumit

> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Li Wang]

On Fri, Mar 15, 2019 at 7:34 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > > I am not sure if this warning message is desired for test-cases which
> > > > needs to wait on checkpoints irrespective of signals like this
> > > > tgkill01 test-case.
> > >
> > > Agreed, it's not an error condition, it's just a coincidence that most
> > > of the tests does not get signals when they sleep on futex, otherwise
> > > thing would crash and burn. So I would argue that retrying on EINTR is
> > > actually a bug fix rather than anything else.
> > >
> >
> > Okay, here I'm not insist to print the warning. But it's only use for
> hint
> > on that worst situation which you were mentioned. If the checkpoint got
> > signal leads to never timeout and test eventually killed by test lib.
> That
> > would hard to know what happened at that moment. My concern is the
> > situation is hard to reproduce later so just want to print more useful
> > messeges:).
>
> As for now that's only a hypotetical corner case, someone would have to
> send signals to such process sleeping on the checkpoint in a loop for
> that to happen. The problem is that printing any messages when
> checkpoint was interrupted by signal would lead to even greater
> confusion for tests that actually have to send signals to such
> processes.
>

That's true. I'm ok to withdraw my suggestion.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190318/0369cce3/attachment.html>

[LTP] [PATCH v2 1/3] syscalls/tgkill01: add new test

[Cyril Hrubis]

Hi!
> Ok got it. Should I pick up this change as separate patch in this
> patch-set or would you like to commit it and then I would update
> patch-set to use these checkpoints?

I will send my patch to the ML soon, you can then rebase your work on
the top of that then I will commit the patches in right order.

-- 
Cyril Hrubis
chrubis@suse.cz