On Tue, 6 May 2014 18:20:09 +0200
<[email protected]> wrote:
> Hi!
> > +Dependencies
> > +------------
> > +CPU partitioning is performed by the partrt tool, which is
> > available as a git +submodule. Before building, do the following
> > from the LTP root directory to +include rt-tools in the build:
> > +
> > + git submodule update utils/rt-tools
>
> I think that you have to do git submodule init prior to this (at least
> if you do this right after you cloned git repository).
Ok.
>
> > +/* Verbosity level chosen from program command line.
> > + * 0 = No extra information from called sub-programs
> > + * 1 = Request verbose information from called sub-programs
> > + * 2 = Request more verbose inormation from called sub-programs.
> > + */
> > +static int verbose;
>
> The verbosity level doesn't seem to be used anywhere (apart from being
> set while parsing command line options).
Hmm, I thought I had something left, but whatever it was it's no longer
there. So you're right, I could just as well rip it out completely.
> > + if (WIFSIGNALED(status))
> > + tst_brkm(TBROK, cleanup,
> > + "%s: Child terminated unexpectedly due to
> > signal nr %d",
> > + cmd,
> > + WTERMSIG(status));
>
> I think that it's more readbale to add the curly braces around block
> that spawns to multiple lines even if it's technically one function
> call.
Ok.
> > +/*
> > + * Convert string to unsigned long.
> > + * Error prefix expressed as string.
> > + */
> > +static unsigned long str_to_ulong(
> > + const char *str,
> > + const char *err_prefix
> > + )
>
> Why not just:
>
> static unsigned long str_to_ulong(const char *str, const char
> *err_prefix)
>
> As far as I can the line fits into 80 chars.
Yep, it did. Fixing.
> > + /* Launch command */
> > + errno = 0;
> > + file = popen(cmd_buf, "r");
> > + if (file == NULL) {
> > + if (errno == 0)
> > + tst_brkm(TBROK, cleanup, "%s: popen(): Out
> > of memory",
> > + cmd_buf);
> > + else
> > + tst_brkm(TBROK | TERRNO, cleanup,
> > + "%s: popen() failed", cmd_buf);
> > + }
>
> This may be worth of SAFE_POPEN(), I can add it if you want.
Go ahead. I'll update my code when you're done.
> > + /* Move child to RT partition */
> > + SAFE_ASPRINTF(cleanup, &tid_str, "%ld", (long) tid);
> > + SAFE_WRITE(cleanup, 1, cpuset_tasks_fd, tid_str,
> > strlen(tid_str)); +
> > + free(tid_str);
> > + SAFE_CLOSE(cleanup, cpuset_tasks_fd);
>
> SAFE_FILE_PRINTF(cleanup, CPUSET_RT_TASKS_FILE, "%ld",
> (long)tid) ?
>
> Or is there a good reason to use open(), asprintf(), write()
> and close() instead?
Well, printf() by itself will do no file updates, but I replaced
open(), asprintf(), write(), free() and close() with fopen(), fprintf()
and fclose().
There was no SAFE_FPRINTF(), so I used ordinary fprintf().
> > + if (cleanup_entered)
> > + return; /* Called from cleanup() */
> > +
> > + cleanup_entered = 1;
>
> Why is this still needed? It shouldn't be if only parent process uses
> the tst_* interface.
This is because of shell() and assert_exit_status() functions not taking
cleanup parameter. I made sure they do, and scrapped the recursion
inhibitor.
> > + for (nr_matches = fscanf(stream, "%d-%d", &range_first,
> > &range_last);
> > + nr_matches > 0;
> > + nr_matches = fscanf(stream, ",%d-%d", &range_first,
> > &range_last)) {
> > + if (nr_matches == 1)
> > + range_last = range_first;
> > +
> > + /* Set all bits in range */
> > + for (bit = range_first; bit <= range_last; bit++)
> > + mask |= (1 << bit);
>
> Are you sure that this would not overflow?
>
> I guess that it depends on how you have
> partitioned your CPUs.
As far as I know there is no API stable and architecture portable way of
determining how many CPUs a machine has, which makes this whole
business a bit tricky. The problem starts with the partrt tool, which
should actually support arbitrarily long masks, but this hasn't
happened.
This is on the TODO list, so currently the test only guarantees 32
CPUs. Should probably be documented somewhere...
> > +static void tools_check(void)
> > +{
> > + const char *env_path = getenv("PATH");
> > +
> > + if (env_path == NULL)
> > + env_path = "";
> > +
> > + tool_available("partrt", env_path);
> > + tool_available("list2mask", env_path);
> > + tool_available("count_ticks", env_path);
>
> We have tst_get_path() that could be used as:
>
> char buf[2048];
>
> if (!tst_get_path("partrt", buf, sizeof(buf)))
> tst_brkm(TCONF, cleanup, "Tool partrt not available");
>
That would simplify the implementation of tool_available(). Thanks!
> > + tst_resm(TINFO, "Execution started: %s",
> > ctime(¤t_time));
> > + tst_resm(TINFO, "Execution ends : %s",
> > ctime(&time_finished));
>
> What is this information good for? As far as I can see it says how
> long it took to prepare for the actual test.
It's most useful for manual runs when you're wondering when the test
will finish. The first one is more a sanity check that you agree what
the current time is...
> > +static void usage(void)
> > +{
> > + printf(
> > + "Usage: %s [options]\n"
> > + " %s --help\n"
> > + "Test whether a CPU can be isolated and made
> > tickless even under load.\n"
> > + "\n"
> > + " -h Display this usage text and exit.\n"
> > + " -v <level> Set verbose level, 0 = default.\n"
> > + " -d <secs> Number of seconds to run the test.\n"
> > + ,
> > + appname, appname
> > + );
> > +
> > + exit(1);
>
> The library exits after usage is printed, the exit(1) here will not be
> reached.
Ok.
> > +int main(int argc, char *argv[])
> > +{
> > + long duration = 0;
> > + int lc;
> > + unsigned int nr_children;
> > + char *error_msg;
> > + char *verbose_str = NULL;
> > + char *duration_str = NULL;
> > +
> > + const option_t options[] = {
> > + {"v:", NULL, &verbose_str},
> > + {"d:", NULL, &duration_str},
> > + {NULL, NULL, NULL}
> > + };
> > +
> > + verbose = 0;
> > + appname = basename(argv[0]);
> > +
> > + error_msg = parse_opts(argc, argv, options, usage);
> > + if (error_msg != NULL)
> > + tst_brkm(TBROK, NULL, "Error parsing command line:
> > %s",
> > + error_msg);
> > +
> > + if (verbose_str != NULL)
> > + verbose = str_to_ulong(verbose_str, "-v");
> > +
> > + if (duration_str == NULL)
> > + tst_brkm(TBROK, cleanup,
> > + "No duration specified, nothing to do");
> > +
> > + duration = (long) str_to_ulong(duration_str, "-d");
> > +
> > + tst_resm(TINFO, "%s: Compiled %s %s", __FILE__, __DATE__,
> > __TIME__);
>
> I would say that this is not very useful information, or is it?
During development it's vital, has saved me many hours. Anyone hacking
the file might find it equally useful, and I don't see the harm of
keeping it even for the majority that doesn't care.
>
> > + nr_children = setup_children();
> > +
> > + for (lc = 0; TEST_LOOPING(lc); lc++)
> > + test(duration, nr_children);
> > +
> > + cleanup();
> > + tst_exit();
> > +
> > + /* Should not end up here */
> > + return 1;
>
> Remove the return from here. The tst_exit() is marked as
> __attribute__((noreturn)) and the compiler knows that you cannot get
> here.
Ok.
> > diff --git a/utils/Makefile b/utils/Makefile
> > index 1508b35..8c2fc7e 100644
> > --- a/utils/Makefile
> > +++ b/utils/Makefile
> > @@ -22,7 +22,11 @@ top_srcdir ?= ..
> >
> > include $(top_srcdir)/include/mk/env_pre.mk
> >
> > +ifneq ($(wildcard rt-tools),)
> > +MAKE_TARGETS += ffsb partrt list2mask count_ticks
> > +else
> > MAKE_TARGETS += ffsb
> > +endif
>
> This would be better as:
>
> MAKE_TARGETS += ffsb
>
> ifneq ($(wildcard rt-tools),)
> MAKE_TARGETS += partrt list2mask count_ticks
> endif
>
> because otherwise we would need to add any new tool into two places
> which is prone to mistakes.
Agree.
Regards
Mats Liljegren
------------------------------------------------------------------------------
Is your legacy SCM system holding you back? Join Perforce May 7 to find out:
• 3 signs your SCM is hindering your productivity
• Requirements for releasing software faster
• Expert tips and advice for migrating your SCM now
http://p.sf.net/sfu/perforce
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
