[LTP] [PATCH v2 1/4] lib: Allow user to easily get LTP_TIMEOUT_MUL value

[LTP] [PATCH v2 1/4] lib: Allow user to easily get LTP_TIMEOUT_MUL value

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V2:
* Remove code which performs extra loop if one thread is behind the other because
  it only works when thread A sets exit after it has finished looping.
* Scale the execution time by LTP_TIMEOUT_MUL

 include/tst_test.h |  1 +
 lib/tst_test.c     | 23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/tst_test.h b/include/tst_test.h
index 98dacf387..0ad9025f2 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -217,6 +217,7 @@ const char *tst_strsig(int sig);
  */
 const char *tst_strstatus(int status);
 
+float tst_timeout_mul(void);
 void tst_set_timeout(int timeout);
 
 #ifndef TST_NO_DEFAULT_MAIN
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 2f3d357d2..f760e05b3 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -992,26 +992,31 @@ static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
 	}
 }
 
-void tst_set_timeout(int timeout)
+float tst_timeout_mul(void)
 {
 	char *mul = getenv("LTP_TIMEOUT_MUL");
 
-	if (timeout == -1) {
-		tst_res(TINFO, "Timeout per run is disabled");
-		return;
-	}
-
-	results->timeout = timeout;
-
 	if (mul) {
 		float m = atof(mul);
 
 		if (m < 1)
 			tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
 
-		results->timeout = results->timeout * m + 0.5;
+		return m;
+	}
+
+	return 1;
+}
+
+void tst_set_timeout(int timeout)
+{
+	if (timeout == -1) {
+		tst_res(TINFO, "Timeout per run is disabled");
+		return;
 	}
 
+	results->timeout = timeout * tst_timeout_mul() + 0.5;
+
 	tst_res(TINFO, "Timeout per run is %uh %02um %02us",
 		results->timeout/3600, (results->timeout%3600)/60,
 		results->timeout % 60);
-- 
2.18.0

[LTP] [PATCH v2 2/4] tst_timer: Convert to new library

[Richard Palethorpe]

So we can use functions from new lib.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 lib/tst_timer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/tst_timer.c b/lib/tst_timer.c
index 53ff36777..d5e4c49d2 100644
--- a/lib/tst_timer.c
+++ b/lib/tst_timer.c
@@ -23,7 +23,9 @@
 
 #include <errno.h>
 
-#include "test.h"
+#define TST_NO_DEFAULT_MAIN
+
+#include "tst_test.h"
 #include "tst_timer.h"
 #include "tst_clocks.h"
 #include "lapi/posix_clocks.h"
@@ -59,13 +61,13 @@ void tst_timer_check(clockid_t clk_id)
 {
 	if (tst_clock_gettime(clk_id, &start_time)) {
 		if (errno == EINVAL) {
-			tst_brkm(TCONF, NULL,
+			tst_brk(TCONF,
 			         "Clock id %s(%u) not supported by kernel",
 				 clock_name(clk_id), clk_id);
 			return;
 		}
 
-		tst_brkm(TBROK | TERRNO, NULL, "tst_clock_gettime() failed");
+		tst_brk(TBROK | TERRNO, "tst_clock_gettime() failed");
 	}
 }
 
@@ -74,7 +76,7 @@ void tst_timer_start(clockid_t clk_id)
 	clock_id = clk_id;
 
 	if (tst_clock_gettime(clock_id, &start_time))
-		tst_resm(TWARN | TERRNO, "tst_clock_gettime() failed");
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
 int tst_timer_expired_ms(long long ms)
@@ -82,7 +84,7 @@ int tst_timer_expired_ms(long long ms)
 	struct timespec cur_time;
 
 	if (tst_clock_gettime(clock_id, &cur_time))
-		tst_resm(TWARN | TERRNO, "tst_clock_gettime() failed");
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 
 	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
 }
@@ -90,7 +92,7 @@ int tst_timer_expired_ms(long long ms)
 void tst_timer_stop(void)
 {
 	if (tst_clock_gettime(clock_id, &stop_time))
-		tst_resm(TWARN | TERRNO, "tst_clock_gettime() failed");
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
 struct timespec tst_timer_elapsed(void)
-- 
2.18.0

[LTP] [PATCH v2 3/4] tst_timer: Add tst_timer_state_ms function

[Richard Palethorpe]

Allow the user to discover the current timer state with a single
function. Useful for using a timer in a loop where we do not know if the timer
has already been started.

Also scale the expiration time by LTP_TIMEOUT_MUL so that the end user can
control the time tests take which use this API.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_timer.h |  7 +++++++
 lib/tst_timer.c     | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/tst_timer.h b/include/tst_timer.h
index 0fd7ed6cf..a6df53e82 100644
--- a/include/tst_timer.h
+++ b/include/tst_timer.h
@@ -34,6 +34,11 @@
 #include <sys/time.h>
 #include <time.h>
 
+#define TST_TIMER_NONE 0
+#define TST_TIMER_STARTED 1
+#define TST_TIMER_STOPPED (1 << 1)
+#define TST_TIMER_EXPIRED (1 << 2)
+
 /*
  * Converts timespec to microseconds.
  */
@@ -259,6 +264,8 @@ void tst_timer_start(clockid_t clk_id);
  */
 int tst_timer_expired_ms(long long ms);
 
+int tst_timer_state_ms(long long ms);
+
 /*
  * Marks timer end time.
  */
diff --git a/lib/tst_timer.c b/lib/tst_timer.c
index d5e4c49d2..182b21441 100644
--- a/lib/tst_timer.c
+++ b/lib/tst_timer.c
@@ -86,7 +86,19 @@ int tst_timer_expired_ms(long long ms)
 	if (tst_clock_gettime(clock_id, &cur_time))
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 
-	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
+	return tst_timespec_diff_ms(cur_time, start_time)
+		>= ms * tst_timeout_mul();
+}
+
+int tst_timer_state_ms(long long ms)
+{
+	int status = TST_TIMER_NONE;
+
+	status |= (start_time.tv_sec | start_time.tv_nsec) > 0;
+	status |= ((stop_time.tv_sec | stop_time.tv_nsec) > 0) << 1;
+	status |= tst_timer_expired_ms(ms) << 2;
+
+	return status;
 }
 
 void tst_timer_stop(void)
-- 
2.18.0

[LTP] [PATCH v2 4/4] fzsync: Limit execution time to prevent test timeouts

[Richard Palethorpe]

Use the tst_timer library to check how long the main test loop has been
running for and break the loop if it exceeds 60 seconds. This prevents an
overall test timeout which is reported as a failure.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_fuzzy_sync.h                      | 47 ++++++++++++-------
 lib/newlib_tests/test16.c                     |  3 +-
 testcases/cve/cve-2014-0196.c                 |  3 +-
 testcases/cve/cve-2016-7117.c                 |  3 +-
 testcases/cve/cve-2017-2671.c                 |  3 +-
 testcases/kernel/syscalls/inotify/inotify09.c |  3 +-
 6 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h
index bcc625e24..75866d73c 100644
--- a/include/tst_fuzzy_sync.h
+++ b/include/tst_fuzzy_sync.h
@@ -42,6 +42,7 @@
 #include <time.h>
 #include <math.h>
 #include "tst_atomic.h"
+#include "tst_timer.h"
 
 #ifndef CLOCK_MONOTONIC_RAW
 # define CLOCK_MONOTONIC_RAW CLOCK_MONOTONIC
@@ -265,9 +266,7 @@ static inline int tst_fzsync_pair_wait(struct tst_fzsync_pair *pair,
 			;
 	}
 
-	/* Only exit if we have done the same amount of work as the other thread */
-	return !tst_atomic_load(&pair->exit) ||
-	  tst_atomic_load(other_cntr) <= tst_atomic_load(our_cntr);
+	return !tst_atomic_load(&pair->exit);
 }
 
 static inline int tst_fzsync_wait_a(struct tst_fzsync_pair *pair)
@@ -280,6 +279,17 @@ static inline int tst_fzsync_wait_b(struct tst_fzsync_pair *pair)
 	return tst_fzsync_pair_wait(pair, &pair->b_cntr, &pair->a_cntr);
 }
 
+/**
+ * tst_fzsync_pair_exit - Signal that the other thread should exit
+ *
+ * Causes tst_fzsync_pair_wait() and tst_fzsync_pair_wait_update() to return
+ * 0.
+ */
+static inline void tst_fzsync_pair_exit(struct tst_fzsync_pair *pair)
+{
+	tst_atomic_store(1, &pair->exit);
+}
+
 /**
  * tst_fzsync_pair_wait_update_{a,b} - Wait and then recalculate
  *
@@ -301,10 +311,26 @@ static inline int tst_fzsync_wait_b(struct tst_fzsync_pair *pair)
 static inline int tst_fzsync_wait_update_a(struct tst_fzsync_pair *pair)
 {
 	static int loop_index;
+	int timer_state = tst_timer_state_ms(60000);
+	int exit = 0;
+
+	if (!(timer_state & TST_TIMER_STARTED)) {
+		tst_timer_start(CLOCK_MONOTONIC_RAW);
+	} else if (timer_state & TST_TIMER_EXPIRED) {
+		tst_res(TINFO,
+			"Exceeded fuzzy sync time limit, requesting exit");
+		exit = 1;
+	}
 
 	tst_fzsync_pair_wait(pair, &pair->a_cntr, &pair->b_cntr);
-	loop_index++;
-	tst_fzsync_pair_update(loop_index, pair);
+
+	if (exit) {
+		tst_fzsync_pair_exit(pair);
+	} else {
+		loop_index++;
+		tst_fzsync_pair_update(loop_index, pair);
+	}
+
 	return tst_fzsync_pair_wait(pair, &pair->a_cntr, &pair->b_cntr);
 }
 
@@ -313,14 +339,3 @@ static inline int tst_fzsync_wait_update_b(struct tst_fzsync_pair *pair)
 	tst_fzsync_pair_wait(pair, &pair->b_cntr, &pair->a_cntr);
 	return tst_fzsync_pair_wait(pair, &pair->b_cntr, &pair->a_cntr);
 }
-
-/**
- * tst_fzsync_pair_exit - Signal that the other thread should exit
- *
- * Causes tst_fzsync_pair_wait() and tst_fzsync_pair_wait_update() to return
- * 0.
- */
-static inline void tst_fzsync_pair_exit(struct tst_fzsync_pair *pair)
-{
-	tst_atomic_store(1, &pair->exit);
-}
diff --git a/lib/newlib_tests/test16.c b/lib/newlib_tests/test16.c
index d80bd5369..bb3d86e2d 100644
--- a/lib/newlib_tests/test16.c
+++ b/lib/newlib_tests/test16.c
@@ -65,7 +65,8 @@ static void run(void)
 	unsigned int i, j, fail = 0;
 
 	for (i = 0; i < LOOPS; i++) {
-		tst_fzsync_wait_update_a(&pair);
+		if (!tst_fzsync_wait_update_a(&pair))
+			break;
 		tst_fzsync_delay_a(&pair);
 		seq[seq_n] = 'A';
 		seq_n = i * 2 + 1;
diff --git a/testcases/cve/cve-2014-0196.c b/testcases/cve/cve-2014-0196.c
index d18108897..8d15909b7 100644
--- a/testcases/cve/cve-2014-0196.c
+++ b/testcases/cve/cve-2014-0196.c
@@ -118,7 +118,8 @@ static void run(void)
 		t.c_lflag |= ECHO;
 		tcsetattr(master_fd, TCSANOW, &t);
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 
 		tst_fzsync_delay_a(&fzsync_pair);
 		tst_fzsync_time_a(&fzsync_pair);
diff --git a/testcases/cve/cve-2016-7117.c b/testcases/cve/cve-2016-7117.c
index 3cb7efcdf..d68f62ecc 100644
--- a/testcases/cve/cve-2016-7117.c
+++ b/testcases/cve/cve-2016-7117.c
@@ -140,7 +140,8 @@ static void run(void)
 		if (socketpair(AF_LOCAL, SOCK_DGRAM, 0, (int *)socket_fds))
 			tst_brk(TBROK | TERRNO, "Socket creation failed");
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 		tst_fzsync_delay_a(&fzsync_pair);
 
 		stat = tst_syscall(__NR_recvmmsg,
diff --git a/testcases/cve/cve-2017-2671.c b/testcases/cve/cve-2017-2671.c
index b0471bfff..21dd2a754 100644
--- a/testcases/cve/cve-2017-2671.c
+++ b/testcases/cve/cve-2017-2671.c
@@ -115,7 +115,8 @@ static void run(void)
 		SAFE_CONNECT(sockfd,
 			     (struct sockaddr *)&iaddr, sizeof(iaddr));
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 		tst_fzsync_delay_a(&fzsync_pair);
 		connect(sockfd, (struct sockaddr *)&uaddr, sizeof(uaddr));
 		tst_fzsync_time_a(&fzsync_pair);
diff --git a/testcases/kernel/syscalls/inotify/inotify09.c b/testcases/kernel/syscalls/inotify/inotify09.c
index 475411311..52df38f24 100644
--- a/testcases/kernel/syscalls/inotify/inotify09.c
+++ b/testcases/kernel/syscalls/inotify/inotify09.c
@@ -102,7 +102,8 @@ static void verify_inotify(void)
 		if (wd < 0)
 			tst_brk(TBROK | TERRNO, "inotify_add_watch() failed.");
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 		tst_fzsync_delay_a(&fzsync_pair);
 
 		wd = myinotify_rm_watch(inotify_fd, wd);
-- 
2.18.0

[LTP] [PATCH v2 4/4] fzsync: Limit execution time to prevent test timeouts

[Li Wang]

Hi Richard,

Richard Palethorpe <rpalethorpe@suse.com> wrote:

Use the tst_timer library to check how long the main test loop has been
> running for and break the loop if it exceeds 60 seconds. This prevents an
>

This summary needs a minor adjustment for v2 too. Otherwise patch set looks
good to me.

Btw, I think the whole fuzzy synchronisation maybe a bit complicated to LTP
users, I read all of the fuzzy_sync functions and spent much time to learn
the code logic :). Of course, if we want to do some improvement work,
better in new divided patch then.

overall test timeout which is reported as a failure.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>

Reviewed-by: Li Wang <liwang@redhat.com>

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

[LTP] [PATCH v2 4/4] fzsync: Limit execution time to prevent test timeouts

[Richard Palethorpe]

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
> Richard Palethorpe <rpalethorpe@suse.com> wrote:
>
> Use the tst_timer library to check how long the main test loop has been
>> running for and break the loop if it exceeds 60 seconds. This prevents an
>>
>
> This summary needs a minor adjustment for v2 too. Otherwise patch set looks
> good to me.
>
> Btw, I think the whole fuzzy synchronisation maybe a bit complicated to LTP
> users, I read all of the fuzzy_sync functions and spent much time to learn
> the code logic :). Of course, if we want to do some improvement work,
> better in new divided patch then.

Yes, I agree and have some ideas about that (see comments in V1 patches
covering letter). Thanks for the feedback.

Maybe the implementation can detect some common usage mistakes, which I
think is the main issue here. For example if the user has a different
number of waits in one thread than the other, we should be able to
detect that. Then people can learn how to use it more through
experimentation which is much easier than reading the implementation or
even documentation.

>
> overall test timeout which is reported as a failure.
>>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>>
>
> Reviewed-by: Li Wang <liwang@redhat.com>


--
Thank you,
Richard.

[LTP] [PATCH v2 4/4] fzsync: Limit execution time to prevent test timeouts

[Richard Palethorpe]

Hello,

Richard Palethorpe <rpalethorpe@suse.com> writes:

>  /**
>   * tst_fzsync_pair_wait_update_{a,b} - Wait and then recalculate
>   *
> @@ -301,10 +311,26 @@ static inline int tst_fzsync_wait_b(struct tst_fzsync_pair *pair)
>  static inline int tst_fzsync_wait_update_a(struct tst_fzsync_pair *pair)
>  {
>  	static int loop_index;
> +	int timer_state = tst_timer_state_ms(60000);
> +	int exit = 0;
> +
> +	if (!(timer_state & TST_TIMER_STARTED)) {
> +		tst_timer_start(CLOCK_MONOTONIC_RAW);
> +	} else if (timer_state & TST_TIMER_EXPIRED) {
> +		tst_res(TINFO,
> +			"Exceeded fuzzy sync time limit, requesting exit");
> +		exit = 1;

This is not going to work with the -i argument or if the test author
also wants to use tst_timer. I'm going to have to do something
different.

--
Thank you,
Richard.

[LTP] [PATCH v2 4/4] fzsync: Limit execution time to prevent test timeouts

[Li Wang]

Richard Palethorpe <rpalethorpe@suse.de> wrote:

Hello,
>
> Richard Palethorpe <rpalethorpe@suse.com> writes:
>
> >  /**
> >   * tst_fzsync_pair_wait_update_{a,b} - Wait and then recalculate
> >   *
> > @@ -301,10 +311,26 @@ static inline int tst_fzsync_wait_b(struct
> tst_fzsync_pair *pair)
> >  static inline int tst_fzsync_wait_update_a(struct tst_fzsync_pair
> *pair)
> >  {
> >       static int loop_index;
> > +     int timer_state = tst_timer_state_ms(60000);
> > +     int exit = 0;
> > +
> > +     if (!(timer_state & TST_TIMER_STARTED)) {
> > +             tst_timer_start(CLOCK_MONOTONIC_RAW);
> > +     } else if (timer_state & TST_TIMER_EXPIRED) {
> > +             tst_res(TINFO,
> > +                     "Exceeded fuzzy sync time limit, requesting exit");
> > +             exit = 1;
>
> This is not going to work with the -i argument or if the test author
> also wants to use tst_timer. I'm going to have to do something
> different.


Yes, good catch. I didn't realized that when reviewing the patch.

Maybe we can try to leave the expired time judgement in fzync library and
without using any LTP timer API. But for the -i parameter, it seems hard to
set pair->exit to 0 at a proper time :(.

Now I'm thinking that why not to redesign/improve the fzync first and then
solve the timeout issue. It will be easier than current situation I guess.

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

[LTP] [PATCH v2 4/4] fzsync: Limit execution time to prevent test timeouts

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> Hello,
>>
>> Richard Palethorpe <rpalethorpe@suse.com> writes:
>>
>> >  /**
>> >   * tst_fzsync_pair_wait_update_{a,b} - Wait and then recalculate
>> >   *
>> > @@ -301,10 +311,26 @@ static inline int tst_fzsync_wait_b(struct
>> tst_fzsync_pair *pair)
>> >  static inline int tst_fzsync_wait_update_a(struct tst_fzsync_pair
>> *pair)
>> >  {
>> >       static int loop_index;
>> > +     int timer_state = tst_timer_state_ms(60000);
>> > +     int exit = 0;
>> > +
>> > +     if (!(timer_state & TST_TIMER_STARTED)) {
>> > +             tst_timer_start(CLOCK_MONOTONIC_RAW);
>> > +     } else if (timer_state & TST_TIMER_EXPIRED) {
>> > +             tst_res(TINFO,
>> > +                     "Exceeded fuzzy sync time limit, requesting exit");
>> > +             exit = 1;
>>
>> This is not going to work with the -i argument or if the test author
>> also wants to use tst_timer. I'm going to have to do something
>> different.
>
>
> Yes, good catch. I didn't realized that when reviewing the patch.
>
> Maybe we can try to leave the expired time judgement in fzync library and
> without using any LTP timer API. But for the -i parameter, it seems hard to
> set pair->exit to 0 at a proper time :(.

I have another patch set which allows multiple timers to be used with
tst_timer which I will post soon.

>
> Now I'm thinking that why not to redesign/improve the fzync first and then
> solve the timeout issue. It will be easier than current situation I guess.

Redesigning fzsync is much more difficult in my opinion. At least
cve-2016-7117[1] relies on the delay feature so I can't simply remove it
like I previously said.

The race condition happens on the exit from the recvmmesg and close
syscalls. Close takes a fraction of the time recvmmesg does to execute
so synchronising the two threads before making these syscalls does not
work. They need to be synchronised just as the syscalls are both
exiting. Which requires the close call to be delayed.

We currently do this by taking time stamps at the exit of each
systemcall and using the delay function to try and synchronise these two
points. So that we end up with a time line which looks like the
following (assuming everything worked perfectly):

wait_update        time_stamp
   ^                       ^
   | +--------+ +-------+  |
---|-|  delay |-| close |--|-- - -
   | +--------+ +-------+  |
   | +------------------+  |
---|-|     recvmmesg    |--|-- - -
   | +------------------+  |

With the updated API which I proposed we would get something like the
diagram below where the start of close and recvmmesg are synced. Because
recvmmesg takes so much longer to reach its exit code than close, the
race has an extremely low probability of happening.

start_race     end_race
   ^            ^
   | +-------+  |-------+ |
---|-| close |--| wait  |-|-- - -
   | +-------+  +-------+ |
   | +------------------+ |
---|-|     recvmmesg    |-|-- - -
   | +------------------+ |
                          ^
                         end_race

However this doesn't mean the new API is bad, it just means that it has
to detect that close happens much quicker than recvmmesg and introduce
some random delay before the close. This is non-trivial because we have
to calculate the delay (in spin cycles) from timestamps (in nanoseconds)
taken during start_race and end_race, plus the spin cycles spent waiting
in end_race. On some CPUs the clock's resolution may not be good enough
to get an accurate delta between any of the timestamps, so we will just
have the number cycles spent during wait to work with.

[1] This test is currently quite unreliable, but it at least works
sometimes.

--
Thank you,
Richard.