----- Original Message -----
> From: chru...@suse.cz
> To: "Jan Stancek" <jstan...@redhat.com>
> Cc: ltp-list@lists.sourceforge.net
> Sent: Wednesday, 10 September, 2014 5:18:05 PM
> Subject: Re: [LTP] [PATCH] add clock_gettime04
>
> Hi!
> > +#define SAFE_PTHREAD(fn, ...) do { \
> > + int ret = fn(__VA_ARGS__); \
> > + if (ret) { \
> > + TEST_ERRNO = ret; \
> > + tst_brkm(TBROK | TTERRNO, cleanup, #fn); \
> ^
> We have TRERRNO for the case of pthread
> functions.
Is it implemented? I don't see any lib function handling it:
$ grep "TRERRNO" -r .
./include/tst_res_flags.h:#define TRERRNO 0x300 /* Capture errno
information from TEST_RETURN to
> > + } \
> > +} while (0)
> > +
> > +char *TCID = "clock_gettime04";
> > +int TST_TOTAL = 1;
> > +
> > +static volatile sig_atomic_t done_testing;
> > +static int opt_testtime;
> > +static char *opt_testtimestr;
> > +static option_t options[] = {
> > + {"T:", &opt_testtime, &opt_testtimestr},
>
> There is no need to use both opt_testtime and opt_testtimestr, you can
> do just:
>
> if (opt_testtimestr) {
> ...
> }
>
> > + {NULL, NULL, NULL}
> > +};
> > +static pthread_mutex_t exit_mutex = PTHREAD_MUTEX_INITIALIZER;
> > +
> > +static void setup(void);
> > +static void cleanup(void);
> > +
> > +static void sigproc(int sig)
> > +{
> > + (void) sig;
> > + done_testing = 1;
> > +}
>
> Shouldn't we rather do:
>
> if (!done_testing)
> done_testing = 1;
>
> Because as far as I can see the alarm may stil fire between
> one of the threads set done_testing to 2 and the if (done_testing == 1)
> message.
Good point.
>
> > +int64_t get_thread_cpu_time(pthread_t thread)
> > +{
> > + clockid_t clockid;
> > + int ret;
> > + struct timespec tp;
> > +
> > + SAFE_PTHREAD(pthread_getcpuclockid, thread, &clockid);
> > +
> > + ret = clock_gettime(clockid, &tp);
> > + if (ret)
> > + tst_brkm(TBROK | TERRNO, cleanup, "clock_gettime: %d", ret);
> > +
> > + return (tp.tv_sec * NANOSECS_PER_SEC) + tp.tv_nsec;
> > +}
> > +
> > +static void *child(void *arg)
> > +{
> > + struct timespec tim;
> > + int64_t t, t_last = 0;
> > + int check_own_cpu_time = *(int *)arg;
> > +
> > + tim.tv_sec = 0;
> > + tim.tv_nsec = 5000;
> > +
> > + while (!done_testing) {
> > + nanosleep(&tim, NULL);
> > +
> > + if (!check_own_cpu_time)
> > + continue;
> > +
> > + t = get_thread_cpu_time(pthread_self());
> > + if (t < t_last) {
> > + tst_resm(TFAIL, "t: %" PRId64 " < t_last: %"
> > + PRId64, t, t_last);
> > + done_testing = 2;
> > + } else {
> > + t_last = t;
> > + }
> > + }
> > +
> > + SAFE_PTHREAD(pthread_mutex_lock, &exit_mutex);
> > + SAFE_PTHREAD(pthread_mutex_unlock, &exit_mutex);
> > +
> > + return NULL;
> > +}
> > +
> > +static void test(int seconds)
> > +{
> > + int i, child_check_cpu_time[] = { 0, 1 };
> > + pthread_t thread[THREAD_COUNT];
> > + clockid_t clockid[THREAD_COUNT];
> > + int64_t thread_time[THREAD_COUNT], cur_time;
> > +
> > + done_testing = 0;
> > + alarm(seconds);
> > +
> > + /* create children and make every 2nd child check its own
> > + * cpu time as well. */
> > + for (i = 0; i < THREAD_COUNT; i++) {
> > + SAFE_PTHREAD(pthread_create, &thread[i], NULL, child,
> > + &child_check_cpu_time[i % 2]);
> > + SAFE_PTHREAD(pthread_getcpuclockid, thread[i], &clockid[i]);
> > + tst_resm(TINFO, "thread no.: %d clockid: %d", i, clockid[i]);
> > + }
> > +
> > + for (i = 0; i < THREAD_COUNT; i++)
> > + thread_time[i] = get_thread_cpu_time(thread[i]);
> > +
> > + SAFE_PTHREAD(pthread_mutex_lock, &exit_mutex);
>
> Hmm, shouldn't this mutex be locked before we create the threads
> because otherwise some threads may have exited allready.
It should be at least before the for loop above. Thread exiting sooner
shouldn't be big issue, because we would never run while loop below.
Problem is using get_thread_cpu_time() on thread that already exited.
Locking it before we create threads is OK too, I'll move it there.
>
> > + /* check periodically that cpu time of each thread
> > + * doesn't go backwards */
> > + while (!done_testing) {
> > + for (i = 0; i < THREAD_COUNT; i++) {
> > + cur_time = get_thread_cpu_time(thread[i]);
> > + if (cur_time < thread_time[i]) {
> > + tst_resm(TFAIL, "cpu clock time went backwards "
> > + "thread no. %d, clock id: %d"
> > + " previous value: %" PRId64
> > + " current value: %" PRId64,
> > + i, clockid[i], thread_time[i],
> > + cur_time);
> > + done_testing = 2;
> > + break;
> > + }
> > + thread_time[i] = cur_time;
> > + }
> > + }
> > +
> > + /* allow children to exit now */
> > + SAFE_PTHREAD(pthread_mutex_unlock, &exit_mutex);
> > +
> > + for (i = 0; i < THREAD_COUNT; i++)
> > + SAFE_PTHREAD(pthread_join, thread[i], NULL);
> > +
> > + if (done_testing == 1)
> > + tst_resm(TPASS, "Test completed, cpu times of all "
> > + "thread clocks were monotonic");
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + int testtime = DEFAULT_TEST_TIME;
> > + const char *msg = NULL;
> > +
> > + msg = parse_opts(argc, argv, options, NULL);
> > + if (msg)
> > + tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
> > +
> > + if (opt_testtime) {
> > + testtime = atoi(opt_testtimestr);
> > + if (testtime <= 0) {
> > + tst_brkm(TBROK, NULL, "Invalid arg for -T: %s",
> > + opt_testtimestr);
> > + }
> > + }
> > +
> > + setup();
> > +
> > + test(testtime);
>
> Shouldn't the test(testtime) be still inside the for () loop with
> TEST_LOOPING() because otherwise the test will gladly accept and ignore
> the standard parameters (-i -I etc).
I thought the -T would cover both, but I can add it back, with some
check to break that loop if anything fails.
Regards,
Jan
>
> Otherwise it looks OK to me.
>
> --
> Cyril Hrubis
> chru...@suse.cz
>
------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
