From mboxrd@z Thu Jan  1 00:00:00 1970
From: Cyril Hrubis <chrubis@suse.cz>
Date: Wed, 29 Aug 2018 16:35:07 +0200
Subject: [LTP] [PATCH RFC v3 2/3] lib: introduce tst_timeout_remaining()
In-Reply-To: <1455249520.43485695.1535549637276.JavaMail.zimbra@redhat.com>
References: <920f7a911d8b32c3c2e673a9c608ded2526a616d.1535466715.git.jstancek@redhat.com>
 <5730db8dee0c566014c99bc0d264326fe1c923cc.1535466715.git.jstancek@redhat.com>
 <20180829130845.GB30074@rei>
 <1455249520.43485695.1535549637276.JavaMail.zimbra@redhat.com>
Message-ID: <20180829143507.GD30074@rei>
List-Id: <ltp.lists.linux.it>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: ltp@lists.linux.it

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