[RFC,v3,2/3] lib: introduce tst_timeout_remaining()

Which returns number of seconds remaining till timeout.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 include/tst_test.h        |   1 +
 lib/newlib_tests/test18   | Bin 0 -> 384032 bytes
 lib/newlib_tests/test18.c |  32 ++++++++++++++++++++++++++++++++
 lib/tst_test.c            |  21 +++++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100755 lib/newlib_tests/test18
 create mode 100644 lib/newlib_tests/test18.c

Hi!
> Which returns number of seconds remaining till timeout.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  include/tst_test.h        |   1 +
>  lib/newlib_tests/test18   | Bin 0 -> 384032 bytes
>  lib/newlib_tests/test18.c |  32 ++++++++++++++++++++++++++++++++
>  lib/tst_test.c            |  21 +++++++++++++++++++++
>  4 files changed, 54 insertions(+)
>  create mode 100755 lib/newlib_tests/test18
>  create mode 100644 lib/newlib_tests/test18.c
> 
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 98dacf3873ab..c0c9a7c7b995 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);
>  
> +unsigned int tst_timeout_remaining(void);
>  void tst_set_timeout(int timeout);
>  
>  #ifndef TST_NO_DEFAULT_MAIN
> diff --git a/lib/newlib_tests/test18 b/lib/newlib_tests/test18
> new file mode 100755
> index 000000000000..01dc1d7976fe
> Binary files /dev/null and b/lib/newlib_tests/test18 differ
> diff --git a/lib/newlib_tests/test18.c b/lib/newlib_tests/test18.c
> new file mode 100644
> index 000000000000..aba007d3689c
> --- /dev/null
> +++ b/lib/newlib_tests/test18.c
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2018, 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 will 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <stdlib.h>
> +#include "tst_test.h"
> +
> +static void run(void)
> +{
> +	unsigned int remaining = tst_timeout_remaining();

Maybe we should do something as:

	while (tst_timeout_remaining() > 2)
		sleep(1);

	tst_res(TPASS, ...);

And set timeout in tst_test to something as 10s, to really test the API.

> +	if (remaining >= 200)
> +		tst_res(TPASS, "Timeout remaining: %d", remaining);
> +	else
> +		tst_res(TFAIL, "Timeout remaining: %d", remaining);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +};
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 2f3d357d2fcc..75619fabffa4 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -47,6 +47,8 @@ static int iterations = 1;
>  static float duration = -1;
>  static pid_t main_pid, lib_pid;
>  static int mntpoint_mounted;
> +static clockid_t tst_clock;
> +static struct timespec tst_start_time;
>  
>  struct results {
>  	int passed;
> @@ -758,6 +760,7 @@ static void do_setup(int argc, char *argv[])
>  
>  	if (tst_test->sample)
>  		tst_test = tst_timer_test_setup(tst_test);
> +	tst_clock = tst_timer_find_clock();

I wonder if we really need this, we were running with CLOCK_MONOTONIC
timer in the testrun() for quite some time now and nobody complained so
far.

Well I guess that it would be nice to use CLOCK_MONOTONIC_COARSE for the
tst_timeout_remaining if available, which should save us some CPU since
it's supposed to be called in a loop.

>  	parse_opts(argc, argv);
>  
> @@ -992,6 +995,21 @@ static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
>  	}
>  }
>  
> +unsigned int tst_timeout_remaining(void)
> +{
> +	static struct timespec now;
> +	unsigned int elapsed;
> +
> +	if (tst_clock_gettime(tst_clock, &now))
> +		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> +
> +	elapsed = tst_timespec_diff_ms(now, tst_start_time) / 1000;
> +	if (results->timeout > elapsed)
> +		return results->timeout - elapsed;
> +
> +	return 0;
> +}

This is obviously correct.

>  void tst_set_timeout(int timeout)
>  {
>  	char *mul = getenv("LTP_TIMEOUT_MUL");
> @@ -1012,6 +1030,9 @@ void tst_set_timeout(int timeout)
>  		results->timeout = results->timeout * m + 0.5;
>  	}
>  
> +	if (tst_clock_gettime(tst_clock, &tst_start_time))
> +		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");

Looking into this, this will not work with the -i option, since the
timeout is restarted after each iteration in heartbeat_handler().
However clock_gettime() is supposedly signal-safe. So as far as I can
tell we have to take the timestamp in the heartbeat_handler() instead
and that should be it.

----- Original Message -----
> Hi!
> > +#include <stdlib.h>
> > +#include "tst_test.h"
> > +
> > +static void run(void)
> > +{
> > +	unsigned int remaining = tst_timeout_remaining();
> 
> Maybe we should do something as:
> 
> 	while (tst_timeout_remaining() > 2)
> 		sleep(1);
> 
> 	tst_res(TPASS, ...);

Yeah, I felt guilty adding more sleeps() :-).

> 
> And set timeout in tst_test to something as 10s, to really test the API.
> 
> > +	if (remaining >= 200)
> > +		tst_res(TPASS, "Timeout remaining: %d", remaining);
> > +	else
> > +		tst_res(TFAIL, "Timeout remaining: %d", remaining);
> > +}
> > +
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +};
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index 2f3d357d2fcc..75619fabffa4 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -47,6 +47,8 @@ static int iterations = 1;
> >  static float duration = -1;
> >  static pid_t main_pid, lib_pid;
> >  static int mntpoint_mounted;
> > +static clockid_t tst_clock;
> > +static struct timespec tst_start_time;
> >  
> >  struct results {
> >  	int passed;
> > @@ -758,6 +760,7 @@ static void do_setup(int argc, char *argv[])
> >  
> >  	if (tst_test->sample)
> >  		tst_test = tst_timer_test_setup(tst_test);
> > +	tst_clock = tst_timer_find_clock();
> 
> I wonder if we really need this, we were running with CLOCK_MONOTONIC
> timer in the testrun() for quite some time now and nobody complained so
> far.

I don't have strong opinion on this. It's fairly cheap to go through that list,
and we can be more courageous to change order later.

> 
> Well I guess that it would be nice to use CLOCK_MONOTONIC_COARSE for the
> tst_timeout_remaining if available, which should save us some CPU since
> it's supposed to be called in a loop.
> 
> >  	parse_opts(argc, argv);
> >  
> > @@ -992,6 +995,21 @@ static void sigint_handler(int sig
> > LTP_ATTRIBUTE_UNUSED)
> >  	}
> >  }
> >  
> > +unsigned int tst_timeout_remaining(void)
> > +{
> > +	static struct timespec now;
> > +	unsigned int elapsed;
> > +
> > +	if (tst_clock_gettime(tst_clock, &now))
> > +		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> > +
> > +	elapsed = tst_timespec_diff_ms(now, tst_start_time) / 1000;
> > +	if (results->timeout > elapsed)
> > +		return results->timeout - elapsed;
> > +
> > +	return 0;
> > +}
> 
> This is obviously correct.
> 
> >  void tst_set_timeout(int timeout)
> >  {
> >  	char *mul = getenv("LTP_TIMEOUT_MUL");
> > @@ -1012,6 +1030,9 @@ void tst_set_timeout(int timeout)
> >  		results->timeout = results->timeout * m + 0.5;
> >  	}
> >  
> > +	if (tst_clock_gettime(tst_clock, &tst_start_time))
> > +		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> 
> Looking into this, this will not work with the -i option, since the
> timeout is restarted after each iteration in heartbeat_handler().
> However clock_gettime() is supposedly signal-safe. So as far as I can
> tell we have to take the timestamp in the heartbeat_handler() instead
> and that should be it.

heartbeat() is called in tst_set_timeout() only for non-lib pids.
And testrun() calls it only after run_tests().

So I think it will have to be at both locations: anytime we call alarm(),
we'll need to re-initialize tst_start_time:

void timeout_restart(void)
{
    alarm(results->timeout);
    if (tst_clock_gettime(tst_clock, &tst_start_time))
        tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
}

and call it in tst_set_timeout() and heartbeat_handler()

---

What is your opinion on API? Absolute numbers vs ratio approach?

Regards,
Jan

Hi!
> > Maybe we should do something as:
> > 
> > 	while (tst_timeout_remaining() > 2)
> > 		sleep(1);
> > 
> > 	tst_res(TPASS, ...);
> 
> Yeah, I felt guilty adding more sleeps() :-).

Well in this case it's reasonable use... ;-)

> > And set timeout in tst_test to something as 10s, to really test the API.
> > 
> > > +	if (remaining >= 200)
> > > +		tst_res(TPASS, "Timeout remaining: %d", remaining);
> > > +	else
> > > +		tst_res(TFAIL, "Timeout remaining: %d", remaining);
> > > +}
> > > +
> > > +static struct tst_test test = {
> > > +	.test_all = run,
> > > +};
> > > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > > index 2f3d357d2fcc..75619fabffa4 100644
> > > --- a/lib/tst_test.c
> > > +++ b/lib/tst_test.c
> > > @@ -47,6 +47,8 @@ static int iterations = 1;
> > >  static float duration = -1;
> > >  static pid_t main_pid, lib_pid;
> > >  static int mntpoint_mounted;
> > > +static clockid_t tst_clock;
> > > +static struct timespec tst_start_time;
> > >  
> > >  struct results {
> > >  	int passed;
> > > @@ -758,6 +760,7 @@ static void do_setup(int argc, char *argv[])
> > >  
> > >  	if (tst_test->sample)
> > >  		tst_test = tst_timer_test_setup(tst_test);
> > > +	tst_clock = tst_timer_find_clock();
> > 
> > I wonder if we really need this, we were running with CLOCK_MONOTONIC
> > timer in the testrun() for quite some time now and nobody complained so
> > far.
> 
> I don't have strong opinion on this. It's fairly cheap to go through that list,
> and we can be more courageous to change order later.

We do use CLOCK_MONOTONIC in the tst_test.c unconditionally anyways, so
I wouldn't bother with this unless somebody complains.

> > Well I guess that it would be nice to use CLOCK_MONOTONIC_COARSE for the
> > tst_timeout_remaining if available, which should save us some CPU since
> > it's supposed to be called in a loop.
> > 
> > >  	parse_opts(argc, argv);
> > >  
> > > @@ -992,6 +995,21 @@ static void sigint_handler(int sig
> > > LTP_ATTRIBUTE_UNUSED)
> > >  	}
> > >  }
> > >  
> > > +unsigned int tst_timeout_remaining(void)
> > > +{
> > > +	static struct timespec now;
> > > +	unsigned int elapsed;
> > > +
> > > +	if (tst_clock_gettime(tst_clock, &now))
> > > +		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> > > +
> > > +	elapsed = tst_timespec_diff_ms(now, tst_start_time) / 1000;
> > > +	if (results->timeout > elapsed)
> > > +		return results->timeout - elapsed;
> > > +
> > > +	return 0;
> > > +}
> > 
> > This is obviously correct.
> > 
> > >  void tst_set_timeout(int timeout)
> > >  {
> > >  	char *mul = getenv("LTP_TIMEOUT_MUL");
> > > @@ -1012,6 +1030,9 @@ void tst_set_timeout(int timeout)
> > >  		results->timeout = results->timeout * m + 0.5;
> > >  	}
> > >  
> > > +	if (tst_clock_gettime(tst_clock, &tst_start_time))
> > > +		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> > 
> > Looking into this, this will not work with the -i option, since the
> > timeout is restarted after each iteration in heartbeat_handler().
> > However clock_gettime() is supposedly signal-safe. So as far as I can
> > tell we have to take the timestamp in the heartbeat_handler() instead
> > and that should be it.
> 
> heartbeat() is called in tst_set_timeout() only for non-lib pids.
> And testrun() calls it only after run_tests().
> 
> So I think it will have to be at both locations: anytime we call alarm(),
> we'll need to re-initialize tst_start_time:
> 
> void timeout_restart(void)
> {
>     alarm(results->timeout);
>     if (tst_clock_gettime(tst_clock, &tst_start_time))
>         tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> }
> 
> and call it in tst_set_timeout() and heartbeat_handler()

Ah, right. I guess that we primarily care about the test process, which
calls the heartbeat() function. I doubt that we will need this to be
working in the test library, so I guess that adding this to heartbeat()
function will suffice...

> ---
> 
> What is your opinion on API? Absolute numbers vs ratio approach?

Absolute value sounds better to me.

Hi Jan,

I'm OK to go this way, as you said that with the remaining seconds return,
we can do something more for testcase. And I also suggest Richard to have
try on using it in fzsync API.


On Tue, Aug 28, 2018 at 10:40 PM, Jan Stancek <jstancek@redhat.com> wrote:

> Which returns number of seconds remaining till timeout.
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  include/tst_test.h        |   1 +
>  lib/newlib_tests/test18   | Bin 0 -> 384032 bytes
>  lib/newlib_tests/test18.c |  32 ++++++++++++++++++++++++++++++++
>  lib/tst_test.c            |  21 +++++++++++++++++++++
>  4 files changed, 54 insertions(+)
>  create mode 100755 lib/newlib_tests/test18
>

This test18 is a binary file which should not be submitted.

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > Maybe we should do something as:
>> >
>> > 	while (tst_timeout_remaining() > 2)
>> > 		sleep(1);
>> >
>> > 	tst_res(TPASS, ...);
>>
>> Yeah, I felt guilty adding more sleeps() :-).
>
> Well in this case it's reasonable use... ;-)
>
>> > And set timeout in tst_test to something as 10s, to really test the API.
>> >
>> > > +	if (remaining >= 200)
>> > > +		tst_res(TPASS, "Timeout remaining: %d", remaining);
>> > > +	else
>> > > +		tst_res(TFAIL, "Timeout remaining: %d", remaining);
>> > > +}
>> > > +
>> > > +static struct tst_test test = {
>> > > +	.test_all = run,
>> > > +};
>> > > diff --git a/lib/tst_test.c b/lib/tst_test.c
>> > > index 2f3d357d2fcc..75619fabffa4 100644
>> > > --- a/lib/tst_test.c
>> > > +++ b/lib/tst_test.c
>> > > @@ -47,6 +47,8 @@ static int iterations = 1;
>> > >  static float duration = -1;
>> > >  static pid_t main_pid, lib_pid;
>> > >  static int mntpoint_mounted;
>> > > +static clockid_t tst_clock;
>> > > +static struct timespec tst_start_time;
>> > >
>> > >  struct results {
>> > >  	int passed;
>> > > @@ -758,6 +760,7 @@ static void do_setup(int argc, char *argv[])
>> > >
>> > >  	if (tst_test->sample)
>> > >  		tst_test = tst_timer_test_setup(tst_test);
>> > > +	tst_clock = tst_timer_find_clock();
>> >
>> > I wonder if we really need this, we were running with CLOCK_MONOTONIC
>> > timer in the testrun() for quite some time now and nobody complained so
>> > far.
>>
>> I don't have strong opinion on this. It's fairly cheap to go through that list,
>> and we can be more courageous to change order later.
>
> We do use CLOCK_MONOTONIC in the tst_test.c unconditionally anyways, so
> I wouldn't bother with this unless somebody complains.
>
>> > Well I guess that it would be nice to use CLOCK_MONOTONIC_COARSE for the
>> > tst_timeout_remaining if available, which should save us some CPU since
>> > it's supposed to be called in a loop.

I doubt we will notice the difference in speed, if there is any on a
modern CPU. On all the implementations I found in glibc
CLOCK_MONOTONIC(_RAW) appears to just read a register value and scale it
to the CPU frequency. It doesn't look like an expensive operation in LTP
terms. However in some scenarious we definitely need a high resolution
clock.

I think defining TST_DEFAULT_CLOCK as CLOCK_MONOTONIC(_RAW) and using it
everywhere should be fine, although I don't have anything against
returning it from an inline function either.

--
Thank you,
Richard.