[v2,1/3] Redesign TST_RETRY_FUNC()
diff mbox series

Message ID 20200207112236.16462-1-mdoucha@suse.cz
State Superseded
Headers show
Series
  • [v2,1/3] Redesign TST_RETRY_FUNC()
Related show

Commit Message

Martin Doucha Feb. 7, 2020, 11:22 a.m. UTC
The TST_RETRY_FUNC() macro requires a single return value that'll be considered
success. This cannot be used with system calls that e.g. return a new file
descriptor because the success value is somewhat unpredictable.

Redesign TST_RETRY_FUNC() to accept arbitrary macro/function ECHCK as the second
parameter for validating the FUNC return value.
- The loop will end succesfully if ECHCK(ret) evaluates to non-zero.
- The loop will fall through on timeout instead of calling tst_brk().
- errno will be cleared before every FUNC call.
- Add standard check macros for the most common call conventions:
  - TST_RETVAL_EQ0(x) - x == 0
  - TST_RETVAL_NOTNULL(x) - x != 0 or x != NULL
  - TST_RETVAL_GE0(x) - x >= 0

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1: Everything in this patch. Redesign TST_RETRY_FUNC() instead
of adding a copy with slightly different functionality.

 include/tst_common.h                        | 45 +++++++++++++++------
 testcases/kernel/syscalls/tgkill/tgkill03.c |  8 +++-
 2 files changed, 39 insertions(+), 14 deletions(-)

Comments

Martin Doucha Feb. 7, 2020, 11:29 a.m. UTC | #1
On 2/7/20 12:22 PM, Martin Doucha wrote:
> The TST_RETRY_FUNC() macro requires a single return value that'll be considered
> success. This cannot be used with system calls that e.g. return a new file
> descriptor because the success value is somewhat unpredictable.
> 
> Redesign TST_RETRY_FUNC() to accept arbitrary macro/function ECHCK as the second
> parameter for validating the FUNC return value.
> - The loop will end succesfully if ECHCK(ret) evaluates to non-zero.
> - The loop will fall through on timeout instead of calling tst_brk().
> - errno will be cleared before every FUNC call.
> - Add standard check macros for the most common call conventions:
>   - TST_RETVAL_EQ0(x) - x == 0
>   - TST_RETVAL_NOTNULL(x) - x != 0 or x != NULL
>   - TST_RETVAL_GE0(x) - x >= 0
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---

Sorry, I forgot to add this to the commit message:
Co-Developed-by: Cyril Hrubis <chrubis@suse.cz>

Please add it before pushing.
Li Wang Feb. 8, 2020, 6:35 a.m. UTC | #2
Hi Martin,

On Fri, Feb 7, 2020 at 7:22 PM Martin Doucha <mdoucha@suse.cz> wrote:

> The TST_RETRY_FUNC() macro requires a single return value that'll be
> considered
> success. This cannot be used with system calls that e.g. return a new file
> descriptor because the success value is somewhat unpredictable.
>
> Redesign TST_RETRY_FUNC() to accept arbitrary macro/function ECHCK as the
> second
> parameter for validating the FUNC return value.
> - The loop will end succesfully if ECHCK(ret) evaluates to non-zero.
> - The loop will fall through on timeout instead of calling tst_brk().
> - errno will be cleared before every FUNC call.
> - Add standard check macros for the most common call conventions:
>   - TST_RETVAL_EQ0(x) - x == 0
>   - TST_RETVAL_NOTNULL(x) - x != 0 or x != NULL
>   - TST_RETVAL_GE0(x) - x >= 0


Nice to see this enhancement! Few comments as below:

1. We need to update the doc/test-writing-guidelines.txt too.

2. Maybe better to let the shell version is consistent with this new?

3. I remember there were discussions to support enabling infinite loop
in TST_RETRY_FUNC, but not sure if it is possible to add in this patch, or
we can do that after your patch merged.
http://lists.linux.it/pipermail/ltp/2019-October/013896.html

...
>         sprintf(defunct_tid_path, "/proc/%d/task/%d", getpid(),
> defunct_tid);
> -       TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK), -1, 15);
> +       ret = TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK),
> +               CHECK_ENOENT, 15);
>

The test total timeout is set to 20 seconds, here reserve 15 seconds is too
much for the macro looping because doing exponential backoff in
15secs(1us+2us+4us+..) actually larger than the 20secs. So I suggest
raising the tst_test.timeout at the same time or set a smaller value to
MAX_DELAY.
Martin Doucha Feb. 17, 2020, 2:16 p.m. UTC | #3
On 2/8/20 7:35 AM, Li Wang wrote:
> 1. We need to update the doc/test-writing-guidelines.txt too.

Right. I'll resubmit in a moment.

> 2. Maybe better to let the shell version is consistent with this new?

That doesn't make much sense. Shell programs and functions have much
simpler call conventions than C functions. If you really need to test a
more complex result than a single return value in shell, writing a
wrapper function is much easier than writing a validator function.

In C, it's the other way around. Writing a wrapper function would often
be a ton of work compared to writing a simple retval validator macro.

> 3. I remember there were discussions to support enabling infinite loop
> in TST_RETRY_FUNC, but not sure if it is possible to add in this patch,
> or we can do that after your patch merged.
> http://lists.linux.it/pipermail/ltp/2019-October/013896.html

I'll leave that to someone else. Though I'd say that timeout of
(1ULL<<40) should be infinite enough for everybody. All we need to do is
change tst_delay_ and tst_max_delay_ type to unsigned long long.

>     ...
>             sprintf(defunct_tid_path, "/proc/%d/task/%d", getpid(),
>     defunct_tid);
>     -       TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK), -1,
>     15);
>     +       ret = TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK),
>     +               CHECK_ENOENT, 15);
> 
> 
> The test total timeout is set to 20 seconds, here reserve 15 seconds is
> too much for the macro looping because doing exponential backoff in
> 15secs(1us+2us+4us+..) actually larger than the 20secs. So I suggest
> raising the tst_test.timeout at the same time or set a smaller value to
> MAX_DELAY.

Actually, this entire retry loop will never take longer than 17 seconds.
The last single delay will be at most 8.4 seconds (2^23 microseconds)
long and the total combined delay before that will also take 8.4
seconds. The next delay would be 16.8 seconds which is too much so the
loop will end. The main test function takes only a few milliseconds so
there's no problem even in the worst case scenario.

I can change the delay to 9 seconds if you want. It'll make no
difference in practice but the code will be less confusing to humans.
Petr Vorel Feb. 18, 2020, 8:05 a.m. UTC | #4
Hi Martin,

> On 2/8/20 7:35 AM, Li Wang wrote:
> > 1. We need to update the doc/test-writing-guidelines.txt too.

> Right. I'll resubmit in a moment.
Thanks!

> > 2. Maybe better to let the shell version is consistent with this new?

> That doesn't make much sense. Shell programs and functions have much
> simpler call conventions than C functions. If you really need to test a
> more complex result than a single return value in shell, writing a
> wrapper function is much easier than writing a validator function.

> In C, it's the other way around. Writing a wrapper function would often
> be a ton of work compared to writing a simple retval validator macro.
+1.


Kind regards,
Petr
Li Wang Feb. 18, 2020, 8:10 a.m. UTC | #5
On Mon, Feb 17, 2020 at 10:16 PM Martin Doucha <mdoucha@suse.cz> wrote:

> On 2/8/20 7:35 AM, Li Wang wrote:
> > 1. We need to update the doc/test-writing-guidelines.txt too.
>
> Right. I'll resubmit in a moment.
>
> > 2. Maybe better to let the shell version is consistent with this new?
>
> That doesn't make much sense. Shell programs and functions have much
> simpler call conventions than C functions. If you really need to test a
> more complex result than a single return value in shell, writing a
> wrapper function is much easier than writing a validator function.
>

According to the default convention of LTP, we maintain two versions of LTP
APIs(C and Shell), we always keep them consistent for creating unified
tests.

But here I'm OK to reserve a difference for the TST_RETRY_FUNC macro
because it looks a bit complicated to achieve the same one in shell. If
anyone has different thought please let me know :).


> In C, it's the other way around. Writing a wrapper function would often
> be a ton of work compared to writing a simple retval validator macro.
>
> > 3. I remember there were discussions to support enabling infinite loop
> > in TST_RETRY_FUNC, but not sure if it is possible to add in this patch,
> > or we can do that after your patch merged.
> > http://lists.linux.it/pipermail/ltp/2019-October/013896.html
>
> I'll leave that to someone else. Though I'd say that timeout of
> (1ULL<<40) should be infinite enough for everybody. All we need to do is
> change tst_delay_ and tst_max_delay_ type to unsigned long long.
>
Right. It seems no special need to achieve the infinite loop so far.

>
> >     ...
> >             sprintf(defunct_tid_path, "/proc/%d/task/%d", getpid(),
> >     defunct_tid);
> >     -       TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK), -1,
> >     15);
> >     +       ret = TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path,
> R_OK),
> >     +               CHECK_ENOENT, 15);
> >
> >
> > The test total timeout is set to 20 seconds, here reserve 15 seconds is
> > too much for the macro looping because doing exponential backoff in
> > 15secs(1us+2us+4us+..) actually larger than the 20secs. So I suggest
> > raising the tst_test.timeout at the same time or set a smaller value to
> > MAX_DELAY.
>
> Actually, this entire retry loop will never take longer than 17 seconds.
> The last single delay will be at most 8.4 seconds (2^23 microseconds)
> long and the total combined delay before that will also take 8.4
> seconds. The next delay would be 16.8 seconds which is too much so the
> loop will end. The main test function takes only a few milliseconds so
> there's no problem even in the worst case scenario.
>
> I can change the delay to 9 seconds if you want. It'll make no
> difference in practice but the code will be less confusing to humans.
>
> --
> Martin Doucha   mdoucha@suse.cz
> QA Engineer for Software Maintenance
> SUSE LINUX, s.r.o.
> CORSO IIa
> Krizikova 148/34
> 186 00 Prague 8
> Czech Republic
>
>

Patch
diff mbox series

diff --git a/include/tst_common.h b/include/tst_common.h
index a0c06a3f7..5c09fea7f 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -26,35 +26,54 @@ 
 /**
  * TST_RETRY_FUNC() - Repeatedly retry a function with an increasing delay.
  * @FUNC - The function which will be retried
- * @ERET - The value returned from @FUNC on success
+ * @ECHCK - Function/macro for validating @FUNC return value
  *
- * This macro will call @FUNC in a loop with a delay between retries. If @FUNC
- * returns @ERET then the loop exits. The delay between retries starts at one
- * micro second and is then doubled each iteration until it exceeds one second
- * (the total time sleeping will be approximately one second as well). When the
- * delay exceeds one second tst_brk() is called.
+ * This macro will call @FUNC in a loop with a delay between retries.
+ * If ECHCK(ret) evaluates to non-zero, the loop ends. The delay between
+ * retries starts at one microsecond and is then doubled each iteration until
+ * it exceeds one second (the total time sleeping will be approximately one
+ * second as well). When the delay exceeds one second, the loop will end.
+ * The TST_RETRY_FUNC() macro returns the last value returned by @FUNC.
  */
-#define TST_RETRY_FUNC(FUNC, ERET) \
-	TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1)
+#define TST_RETRY_FUNC(FUNC, ECHCK) \
+	TST_RETRY_FN_EXP_BACKOFF(FUNC, ECHCK, 1)
 
-#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)	\
+#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ECHCK, MAX_DELAY)	\
 ({	unsigned int tst_delay_, tst_max_delay_;			\
+	typeof(FUNC) tst_ret_;						\
 	tst_delay_ = 1;							\
 	tst_max_delay_ = tst_multiply_timeout(MAX_DELAY * 1000000);	\
 	for (;;) {							\
-		typeof(FUNC) tst_ret_ = FUNC;				\
-		if (tst_ret_ == ERET)					\
+		errno = 0;						\
+		tst_ret_ = FUNC;					\
+		if (ECHCK(tst_ret_))					\
 			break;						\
 		if (tst_delay_ < tst_max_delay_) {			\
 			usleep(tst_delay_);				\
 			tst_delay_ *= 2;				\
 		} else {						\
-			tst_brk(TBROK, #FUNC" timed out");		\
+			break;						\
 		}							\
 	}								\
-	ERET;								\
+	tst_ret_;								\
 })
 
+/*
+ * Return value validation macros for TST_RETRY_FUNC():
+ * TST_RETVAL_EQ0() - Check that value is equal to zero
+ */
+#define TST_RETVAL_EQ0(x) (!(x))
+
+/*
+ * TST_RETVAL_NOTNULL() - Check that value is not equal to zero/NULL
+ */
+#define TST_RETVAL_NOTNULL(x) (!!(x))
+
+/*
+ * TST_RETVAL_GE0() - Check that value is greater than or equal to zero
+ */
+#define TST_RETVAL_GE0(x) ((x) >= 0)
+
 #define TST_BRK_SUPPORTS_ONLY_TCONF_TBROK(condition) \
 	do { ((void)sizeof(char[1 - 2 * !!(condition)])); } while (0)
 
diff --git a/testcases/kernel/syscalls/tgkill/tgkill03.c b/testcases/kernel/syscalls/tgkill/tgkill03.c
index 593a21726..0002f3278 100644
--- a/testcases/kernel/syscalls/tgkill/tgkill03.c
+++ b/testcases/kernel/syscalls/tgkill/tgkill03.c
@@ -14,6 +14,8 @@ 
 #include "tst_test.h"
 #include "tgkill.h"
 
+#define CHECK_ENOENT(x) ((x) == -1 && errno == ENOENT)
+
 static pthread_t child_thread;
 
 static pid_t parent_tgid;
@@ -44,6 +46,7 @@  static void setup(void)
 	sigset_t sigusr1;
 	pthread_t defunct_thread;
 	char defunct_tid_path[PATH_MAX];
+	int ret;
 
 	sigemptyset(&sigusr1);
 	sigaddset(&sigusr1, SIGUSR1);
@@ -59,7 +62,10 @@  static void setup(void)
 	SAFE_PTHREAD_CREATE(&defunct_thread, NULL, defunct_thread_func, NULL);
 	SAFE_PTHREAD_JOIN(defunct_thread, NULL);
 	sprintf(defunct_tid_path, "/proc/%d/task/%d", getpid(), defunct_tid);
-	TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK), -1, 15);
+	ret = TST_RETRY_FN_EXP_BACKOFF(access(defunct_tid_path, R_OK),
+		CHECK_ENOENT, 15);
+	if (!CHECK_ENOENT(ret))
+		tst_brk(TBROK, "Timeout, %s still exists", defunct_tid_path);
 }
 
 static void cleanup(void)