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

Cyril Hrubis
2018-08-29T14:35:07Z

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.

--
Cyril Hrubis
chrubis@suse.cz