From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jan Stancek <jstancek@redhat.com>
Date: Thu, 30 Aug 2018 08:17:06 -0400 (EDT)
Subject: [LTP] [PATCH v4 1/2] lib: introduce tst_timeout_remaining()
In-Reply-To: <20180830120256.GD6363@rei.lan>
References: <5ad739995335bda348f07fc4d3faeec501e00b19.1535618962.git.jstancek@redhat.com>
 <20180830104219.GA6363@rei.lan>
 <706273620.43655984.1535626905479.JavaMail.zimbra@redhat.com>
 <20180830112246.GB6363@rei.lan> <20180830113034.GC6363@rei.lan>
 <1443598375.43662526.1535630056840.JavaMail.zimbra@redhat.com>
 <20180830120256.GD6363@rei.lan>
Message-ID: <1490142235.43668138.1535631426014.JavaMail.zimbra@redhat.com>
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


----- Original Message -----
> Hi!
> > > > Fair enough, also the alarm() in the test library pid is set before we
> > > > run the test setup, so if the test setup would take a few seconds we
> > > > will be off with the calculation. Although that could be fixed by
> > > > calling heartbeat before we run the loop in testrun(), which I guess
> > > > should be done anyway. That in turn would allow for your patch to have
> > > > the clock_gettime only in the heartbeat function, right?
> > 
> > Correct. We could replace it with call to hearbeat():
> > 
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -929,9 +929,7 @@ static void testrun(void)
> >         unsigned long long stop_time = 0;
> >         int cont = 1;
> >  
> > -       if (tst_clock_gettime(CLOCK_MONOTONIC, &tst_start_time))
> > -               tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
> > -
> > +       heartbeat();
> >         add_paths();
> >         do_test_setup();
> >  
> > but then we should get rid of extra alarm() call in tst_set_timeout(),
> > because new hearbeat() call will do it anyway (it will send signal,
> > and handler in lib will call alarm()):
> > 
> > @@ -1038,9 +1036,7 @@ void tst_set_timeout(int timeout)
> >                 results->timeout/3600, (results->timeout%3600)/60,
> >                 results->timeout % 60);
> >  
> > -       if (getpid() == lib_pid)
> > -               alarm(results->timeout);
> > -       else
> > +       if (getpid() != lib_pid)
> >                 heartbeat();
> >  }
> 
> That would mean that the test library will not timeout unless the child
> process managed to send it a signal, so I would like to keep it there,
> since it's more robust that way.

Ok. So do we stay with v4 (with updated elapsed line) or should I
replace tst_clock_gettime in testrun() with call to heartbeat?

> 
> Well, we may change this, so that the first alarm in the test library
> runs with something as 30 seconds, which should be enough before we get
> the heartbeat() from the child that would reset the alarm with a correct
> timemout.
> 
> > > 
> > > Actually we would have to do the heartbeat before and after the setup.
> > 
> > Why after setup? Doesn't the time spent in setup count towards test time?
> 
> Actually it does, the setup + first iteration of the test share the
> timeout, since the first call to alarm(timeout) in the test library
> happens before we call the test setup. Subsequent iterations does not
> run the setup at all, so the whole timeout applies only to the actuall
> test function.
> 
> This haven't been a problem since our tests are usually very fast, but
> it would probably be better if we do heartbeat() before and after
> do_test_setup().

I like it more now, but as you said, it probably doesn't matter, since
setup is usually quick anyway.

> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>