On Tue, 22 Apr 2014 17:47:11 +0200
<chru...@suse.cz> wrote:
> Hi!
> > +top_srcdir ?= ../../../..
> > +
> > +include $(top_srcdir)/include/mk/testcases.mk
> > +
> > +CFLAGS += -D_GNU_SOURCE -Werror
>
> We usually define _GNU_SOURCE on the top of the C source and please
> remove the -Werror when you are done with testing (these flags should
> be controlled by the top level configure only).
As for D_GNU_SOURCE, I'm not sure why I left it there. I actually knew
it was already set, so why I have it there is somewhat of a mystery
for me. Thanks for pointing it out!
As for -Werror, I can remove it, but not without shedding some tears...
This was my way of assuring that no one who contributed to my code will
be able to slip in warnings unnoticed. To turn things around: The
-Werror is a way of saying "Now this code has once compiled without
warnings, and from now on it is not ok to introduce new warnings".
You might think that it is never ok to introduce warnings, but when I
compile I get some warnings. I didn't want this to happen to my code as
well.
> > +LDLIBS += -lrt
> > +
> > +MAKE_TARGETS := test_partrt_nohz_full
> > +
> > +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> > diff --git
> > a/testcases/kernel/partrt/nohz_full/test_partrt_nohz_full.c
> > b/testcases/kernel/partrt/nohz_full/test_partrt_nohz_full.c new
> > file mode 100644 index 0000000..750ab45 --- /dev/null
> > +++ b/testcases/kernel/partrt/nohz_full/test_partrt_nohz_full.c
> > @@ -0,0 +1,636 @@
> > +/*
> > + * Copyright (C) 2014, Enea AB.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License as
> > published by
> > + * the Free Software Foundation; either version 2 of the License,
> > or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> > + * the GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > 02110-1301 USA
> > + */
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <getopt.h>
> > +#include <sys/types.h>
> > +#include <sys/wait.h>
> > +#include <sys/syscall.h>
> > +#include <sys/stat.h>
> > +#include <stdarg.h>
> > +#include <inttypes.h>
> > +#include <limits.h>
> > +#include <time.h>
> > +#include <sched.h>
> > +#include <fcntl.h>
> > +
> > +#include <test.h>
> > +#include <safe_macros.h>
> > +
> > +/* Name of test case */
> > +const char *TCID = "nohz_full";
>
> This should be 1:1 with the test binary name. I would wote for naming
> both 'partrt_nohz_full' or similar.
I agree.
> > +/* Number of expected "TPASS" */
> > +const int TST_TOTAL = 1;
>
> No useless comments like this please (see paragraph 1.4 in Test
> Writing Guidelines).
Hmm, must have been some copy and paste. I'm usually bad at producing
comments... ;-)
> > +/* Used for RT load */
> > +volatile int dummy_value;
> > +
> > +/* When true (1), all children will terminate */
> > +static volatile int time_is_up = 0;
> > +
> > +/* Verbosity level chosen from program command line.
> > + * 0 = Only errors and status reported
> > + * 1 = Show info() messages
> > + * 2 = Request verbose information from called sub-programs
> > + * 3 = Request more verbose inormation from called sub-programs.
> > + */
> > +static int verbose = 0;
> > +
> > +/* Name of application as given in argv[0] */
> > +static const char *appname;
> > +
> > +/* RT load function prototype */
> > +typedef void (child_func)(void);
> > +
> > +/* Report error a'la printf() and terminate test */
> > +#define err(MSG...) tst_brkm(TBROK, cleanup, MSG)
> > +
> > +/* Report test failed. Will not terminate test execution. */
> > +#define fail(MSG...) tst_resm(TFAIL, MSG)
> > +
> > +/* Report test passed. Will not termiunate test execution. */
> > +#define pass(MSG...) tst_resm(TPASS, MSG)
> > +
> > +/* Print information if verbose has been selected */
> > +#define info(MSG...) do { if (verbose > 0) tst_resm(TINFO, MSG); }
> > while (0)
>
> Defining such macros is no no (see paragraph 1.1 in Test Writing
> Guidelines).
If I drop verbose option and always have info messages printed, I could
actually drop these macros entirely. They actually arised from the time
when I thought I needed to do more, but looking at them now, they seem
a bit pointless.
>
> > +/* Amount of stack for RT load threads */
> > +#define STACK_SZ (64 * 1024)
> > +
> > +static void cleanup(void);
> > +
> > +/*
> > + * Check exit status value.
> > + * If any signs of error is found, report error and abort testing.
> > + */
> > +static void assert_exit_status(const char *cmd, int status)
> > +{
> > + if (WIFSIGNALED(status))
> > + err("%s: Child terminated unexpectedly due to
> > signal nr %d",
> > + cmd,
> > + WTERMSIG(status));
> > + if (!WIFEXITED(status))
> > + err("%s: Child terminated unexpectedly",
> > + cmd);
> > + if (WEXITSTATUS(status) != 0)
> > + err("%s: Child exited with exit status %d",
> > + cmd,
> > + WEXITSTATUS(status));
> > +}
> > +
> > +/*
> > + * Generate a load that is guaranteed not to provoke ticks.
> > + */
> > +static void load_rt(void)
> > +{
> > + while (!time_is_up) {
> > + dummy_value = rand();
> > + }
> > +}
> > +
> > +#define READ 0
> > +#define WRITE 1
>
> Use either 0 and 1 or STDIN_FILENO and STDOUT_FILENO
I think STDIN_FILENO and STDOUT_FILENO was an excellent idea, I will
use them instead of inventing something myself.
>
> > +/*
> > + * Execute command, supplying data to the commands stdin using
> > infp if infp
> > + * is non-NULL, and supplying data output from the commands stdout
> > using
> > + * outfp if outfp is non-NULL.
> > + * The command returns the pid of the command executed.
> > + */
> > +static pid_t
> > +pid_popen(const char *cmd, FILE **infp, FILE **outfp)
> > +{
> > + int p_stdin[2], p_stdout[2];
> > + pid_t pid;
> > +
> > + if (((infp != NULL) && (pipe(p_stdin) != 0)) ||
> > + ((outfp != NULL) && (pipe(p_stdout) != 0)))
> > + err("%s: Failed executing command: pipe(): %s",
> > + cmd, strerror(errno));
> > +
> > + pid = fork();
> > +
> > + if (pid < 0)
> > + err("%s: Failed executing command: fork(): %s",
> > + cmd, strerror(errno));
> > +
> > + if (pid == 0) {
> > + if ((infp != NULL) && (dup2(p_stdin[READ], READ)
> > == -1))
> > + err("%s: pid %lu: dup() to stdin failed:
> > %s",
> > + cmd, (unsigned long) pid,
> > strerror(errno));
> > + if ((outfp != NULL) && (dup2(p_stdout[WRITE],
> > WRITE) == -1))
> > + err("%s: pid %lu: dup() to stdout failed:
> > %s",
> > + cmd, (unsigned long) pid,
> > strerror(errno)); +
> > + /* Close all unneeded descriptors */
> > + if (
> > + ((infp != NULL) &&
> > + ((close(p_stdin[READ]) == -1) ||
> > + (close(p_stdin[WRITE]) ==
> > -1))) ||
> > + ((outfp != NULL) &&
> > + ((close(p_stdout[READ]) == -1) ||
> > + (close(p_stdout[WRITE]) ==
> > -1))))
> > + err("%s: pid %lu: Child: close() of pipe
> > failed: %s",
> > + cmd, (unsigned long) pid,
> > strerror(errno));
>
> This is too ugly.
>
> > + execl("/bin/sh", "sh", "-c", cmd, NULL);
> > +
> > + err("%s: pid %lu: exec() error: %s",
> > + cmd, (unsigned long) pid, strerror(errno));
> > + }
> > +
> > + if (((infp != NULL) && (close(p_stdin[READ]) == -1)) ||
> > + ((outfp != NULL) && (close(p_stdout[WRITE]) ==
> > -1)))
> > + err("%s: pid %lu: Parent: close() of pipe failed:
> > %s",
> > + cmd, (unsigned long) pid, strerror(errno));
> > +
> > + if (infp != NULL) {
> > + (*infp) = fdopen(p_stdin[WRITE], "w");
> > + if (*infp == NULL)
> > + err("%s: pid %lu: fdopen() of stdin
> > failed: %s",
> > + cmd, (unsigned long) pid,
> > strerror(errno));
> > + }
> > +
> > + if (outfp != NULL) {
> > + (*outfp) = fdopen(p_stdout[READ], "r");
> > + if (*outfp == NULL)
> > + err("%s: pid %lu: fdopen() of stdout
> > failed: %s",
> > + cmd, (unsigned long) pid,
> > strerror(errno));
> > + }
> > +
> > + return pid;
> > +}
>
> Did you just reinvented popen() with additional parameters that does
> not get used? Because the infp is always NULL. Okay this returns pid
> that can be used for wait() but even then the code is too complex for
> what it does.
>
> Please do not implement functionality that is not used. This only
> overcomplicates testcases and creates code that will break sooner or
> later.
That's what you get for trying to make your functions generic... ;-)
I agree, there's no point in code that is never used. And by the way, I
invented a popen() that can return a pid. So it's not identical to
popen().
> >
+/*
> > + * Wait for a command with given pid to finish, and assert that
> > the command
> > + * returned success (0).
> > + */
> > +static void
> > +pid_wait(pid_t pid)
> > +{
> > + int status;
> > + char prefix[64];
> > +
> > + snprintf(prefix, sizeof (prefix), "Pid %lu", (unsigned
> > long) pid); +
> > + if (wait(&status) == -1)
> > + err("%s: wait() failed: %s", prefix,
> > strerror(errno));
>
> Use SAFE_WAIT() instead.
There have been cases when I've avoided SAFE_* macros because they
didn't print the scope (i.e. PID). Is it a better idea to always print
the PID using gettid()? This could be quite useful for test cases that
has several threads/processes. On the other hand, it produce clutters
for test cases that does not... But I guess that is a less problem,
especially since this is intended for the error path only.
>
> > + assert_exit_status(prefix, status);
>
> You should pass the pid directly instead of the prefix, see below.
Good idea.
>
> > +}
> > +
> > +/*
> > + * Execute given command using shell, ensure success (0) is
> > returned.
> > + */
> > +static void shell(const char *cmd, ...)
> > +{
> > + char *cmd_buf;
> > + va_list va;
> > +
> > + va_start(va, cmd);
> > + if (vasprintf(&cmd_buf, cmd, va) == -1)
> > + err("%s: Not valid printf format, or out of
> > memory", cmd);
> > + va_end(va);
> > +
> > + info("%s: Executing", cmd_buf);
> > + pid_wait(pid_popen(cmd_buf, NULL, NULL));
> > + info("%s: Returns success", cmd_buf);
> > +
> > + free(cmd_buf);
> > +}
> > +
> > +/*
> > + * Execute given command using shell, ensure success (0) is
> > returned.
> > + * Only reports errors.
> > + */
> > +static void shell_silent(const char *cmd, ...)
> > +{
> > + char *cmd_buf;
> > + va_list va;
> > +
> > + va_start(va, cmd);
> > + if (vasprintf(&cmd_buf, cmd, va) == -1)
> > + err("%s: Not valid printf format, or out of
> > memory", cmd);
> > + va_end(va);
> > +
> > + pid_wait(pid_popen(cmd_buf, NULL, NULL));
> > +
> > + free(cmd_buf);
> > +}
>
> Moreove these two functions are almost duplicate for no good reason.
>
> More generally several shell_* wrappers with sligtly different
> parameters polute the code.
You're thinking about have another parameter telling whether the
function is allowed to produce output or not?
It is quite critical that the right thing is done, so it must be easy
to see what variant is being used. This is the reason for the current
design.
>
> > +/*
> > + * Convert string to unsigned long.
> > + * Error prefix expressed as string.
> > + */
> > +static unsigned long str_to_ulong(
> > + const char *str,
> > + const char *err_prefix,
> > + int base
> > + )
> > +{
> > + char *endptr;
> > + unsigned long val;
> > + val = strtoull(str, &endptr, base);
> > + if (endptr == str)
> > + err("%s: %s: Expected unsigned decimal value",
> > err_prefix, str);
> > + if (*endptr != '\0')
> > + err("%s: %s: Garbage character '%c' found after
> > decimal value",
> > + err_prefix, str, *endptr);
> > +
> > + return val;
> > +}
>
> Use SAFE_STRTOUL() instead.
Good catch.
>
> > +/*
> > + * Call a command using shell.
> > + * Command line expressed as va_list.
> > + * Returns the command's first line of output.
> > + */
> > +static char * shell_str(char *dest, int size, const char *cmd, ...)
> > +{
> > + int len;
> > + char *cmd_buf;
> > + FILE *file;
> > + pid_t pid;
> > + va_list va;
> > +
> > + /* Build command line */
> > + va_start(va, cmd);
> > + if (vasprintf(&cmd_buf, cmd, va) == -1)
> > + err("%s: Not valid printf format, or out of
> > memory", cmd);
> > + va_end(va);
> > +
> > + /* Launch command */
> > + pid = pid_popen(cmd_buf, NULL, &file);
> > + info("%s: Executing, pid %lu", cmd_buf, (unsigned long)
> > pid); +
> > + /* Read commands stdout */
> > + if (fgets(dest, size, file) == NULL) {
> > + if (feof(file))
> > + err("%s: pid %lu: Expected output from the
> > command, but got nothing",
> > + cmd_buf, (unsigned long) pid);
> > + else
> > + err("%s: pid %lu: Could not read command
> > output: %s",
> > + cmd_buf, (unsigned long) pid,
> > strerror(errno));
> > + }
> > +
> > + /* Get rid of terminating newline, if any */
> > + len = strlen(dest);
> > + if (dest[len-1] == '\n')
> > + dest[len-1] = '\0';
> > +
> > + /* Wait until command execution finish.
> > + * Main reason for doing this even though we've got what
> > we want is for
> > + * better error detection. The alternative could be to let
> > cleanup()
> > + * handle it if process hasn't finished by then. */
> > + pid_wait(pid);
> > +
> > + info("%s: Returns: %s", cmd_buf, dest);
> > +
> > + free(cmd_buf);
> > +
> > + return dest;
> > +}
>
> Reinventing the wheel?
>
> This could be implemented with just va_args, popen(), fgets() and
> pclose(). No need to roll custom functions for the function at all.
Hmm, I might actually be able to do without child PID by using gettid()
in my error messages... I have to think about this and see if I could
simplify things.
>
> > +/*
> > + * Call a command using shell.
> > + * Command line expressed similar to printf().
> > + * Expects that the command outputs a value as the first line of
> > output
> > + * from stdout.
> > + * Asserts that the command returned success (0).
> > + */
> > +static unsigned long shell_int(int base, const char *cmd, ...)
> > +{
> > + va_list va;
> > + unsigned long val;
> > + char val_str[64];
> > + char *cmd_buf;
> > +
> > + va_start(va, cmd);
> > + if (vasprintf(&cmd_buf, cmd, va) == -1)
> > + err("%s: Not valid printf format, or out of
> > memory", cmd);
> > + va_end(va);
> > + val = str_to_ulong(
> > + shell_str(val_str, sizeof (val_str), "%s", cmd),
> > cmd, base); +
> > + return val;
> > +}
>
> The cmd_buf is not used nor freed, the additional parameters are
> ignored etc.
>
> If you are going to write shell_int() by using shell_str you must
> create vashell_str() first and build both shell_str and shell_int on
> the top of it. But given that this function is used only once I would
> strongy sugest to use shell_str() and SAFE_STRTOUL() in the caller.
I agree. That would simplify things. Sometimes my ambition to make
things general get a bit out of hand...
>
> > +/*
> > + * Struct used for synchronization between child_entry() and
> > launch_child().
> > + */
> > +struct child_params {
> > + /* Function to run */
> > + child_func *func;
> > +
> > + /* cpuset partition to run in */
> > + const char *cpu_partition;
> > +
> > + /* CPU ID to run on. Must be one of the CPUs in chosen
> > cpu_partition */
> > + int cpu;
> > +
> > + /* Written by child_entry() when no more activities that
> > might provoke
> > + * ticks are expected. */
> > + volatile int started;
> > +};
> > +
> > +/*
> > + * Child tramponline function for clone() function.
> > + */
> > +static int child_entry(void *arg)
> > +{
> > + const int child_tid = syscall(SYS_gettid);
> > + struct child_params *child_params = arg;
> > +
> > + /* Try to be quiet, or else we might provoke ticks */
> > + shell_silent("partrt move %d %s > /dev/null",
> > + child_tid, child_params->cpu_partition);
> > + if (child_params->cpu >= 0)
> > + shell_silent("taskset -p %#lx %d > /dev/null",
> > + (unsigned long) 1 << child_params->cpu,
> > + child_tid);
>
> You must not call tst_* interface from the child process and by
> extension you cannot call shell_silent() from here. See paragraph
> 2.2.7 in the Test Writing Guidelines.
Seems like I could use system() rather than shell_silent() here, not
sure why I didn't. Must think about it to make sure there wasn't a
gotcha there somewhere...
>
> > + /* Signal parent that this child now enters RT load
> > function
> > + * (i.e. no more ticks are expected) */
> > + child_params->started = 1;
> > +
> > + child_params->func();
> > + return 0;
> > +}
> > +
> > +/*
> > + * Start a new thread executing the given function, on the given
> > cpuset
> > + * partition and CPU ID. Note that the CPU ID must be one of the
> > + * available CPUs in the cpuset partition.
> > + */
> > +static void launch_child(
> > + child_func *func,
> > + const char *cpu_partition,
> > + int cpu)
>
> This should fit to one line with less than 80 chars.
I can try and see.
>
> > +{
> > + char *stack = SAFE_MALLOC(cleanup, STACK_SZ);
> > + int tid;
> > + struct child_params child_params = {
> > + func, cpu_partition, cpu, 0
> > + };
> > +
> > + tid = ltp_clone(CLONE_VM, child_entry, &child_params,
> > STACK_SZ, stack);
>
> Is there any reason to use clone instead of fork() of
> pthread_create()?
Yep, fork() will produce a process, but I want a thread. But I also
wanted the child PID, which is why I didn't use pthread_create().
>
> > + if (tid == -1)
> > + err ("clone(): %s", strerror(errno));
> ^
> no spaces between function names and brackets
>
> Have you used checkpatch.pl as suggested in the Test Writing
> Guidelines?
No, but that was my intention before sending the "real" patch. This
current round was RFC since I wanted to know if this is something that
could be upstreamed before I do the final polishing on it.
>
> > + info("tid %d: Starting %s load on cpu %d", tid,
> > cpu_partition, cpu);
> > + while (!child_params.started)
> > + sched_yield();
>
> I do not like the bussy loop here much (We have a pipe base child
> parent synchronization code in the LTP library, see paragraph 2.2.7
> in the Test Writing Guidelines) but given that all the childs do is
> bussy loop...
I'm not sure why this is a problem. Using shared memory seems like a
simpler design that using FIFO dependent on sharing the same current
working directory. I also wonder whether tst_checkpoint.h is dependent
on having different processes rather than different threads to
synchronize.
The whole point with this is to make sure the children are up and
running (i.e. no shell commands left to be done at startup). And this
was the simplest design I could think of to ensure this.
>
> > +}
> > +
> > +/*
> > + * Perform test cleanup.
> > + * Run both when testing is finished, as well as when test is
> > terminated
> > + * abnormally.
> > + */
> > +static void cleanup(void)
> > +{
> > + /* Prohibits multiple execution of cleanup function */
> > + static volatile int cleanup_entered = 0;
>
> This is only one reason why you cannot call tst_ interface from the
> child processes. Remove this and fix the rest of code.
Ok.
>
> > + pid_t pid;
> > + int status;
> > +
> > + if (!__sync_bool_compare_and_swap(&cleanup_entered, 0, 1,
> > + cleanup_entered)) {
> > + info("Cleanup: Already cleaning, exiting");
> > + return;
> > + }
> > +
> > + info("Cleanup: Terminating children");
> > +
> > + time_is_up = 1;
> > +
> > + do {
> > + pid = wait(&status);
> > + if (pid != -1) {
> > + char cmd[64];
> > + snprintf(cmd, sizeof (cmd), "Pid %lu",
> > + (unsigned long) pid);
> > + assert_exit_status(cmd, status);
>
> I do not understand why you pass "Pid %lu"
> string instead of the pid. It's more messy
> this way.
Ok.
>
> > + info("Cleanup: %s: Has terminated
> > successfully", cmd);
> > + }
> > + } while (pid != -1);
> > +
> > + if (errno != ECHILD)
> > + err("Cleanup: wait() failed: %s", strerror(errno));
> > +
> > + info("Cleanup: Done");
> > +}
> > +
> > +static unsigned long determine_nohz_mask(void)
> > +{
> > + const char *nohz_cpus_filename =
> > "/sys/fs/cgroup/cpuset/rt/cpuset.cpus";
> > + int file = open(nohz_cpus_filename, O_RDONLY);
>
> this is not file but fd (file descriptor)
Where the fd represents a file... Whatever. I can change the name.
>
> > + char buf[256] = {0};
> > + char *curr;
> > + int range_first;
> > + int range_last;
> > + unsigned long mask = 0;
> > +
> > + if (file == -1)
> > + err("%s: Open failed, have you run 'partrt
> > create'?",
> > + nohz_cpus_filename);
> > +
> > + SAFE_READ(cleanup, 0, file, buf, sizeof (buf) - 1);
> > +
> > + curr = buf;
> > + while (*curr != '\0') {
> > + char *curr_next;
> > + int bit;
> > +
> > + range_first = range_last = strtoull(curr,
> > &curr_next, 10);
> > + if (curr == curr_next)
> > + err("%s: %s: Cannot parse CPU list at %s",
> > + nohz_cpus_filename, buf, curr);
> > + curr = curr_next;
> > + if (*curr == '-') {
> > + curr++;
> > + range_last = strtoull(curr, &curr_next,
> > 10);
> > + if (curr == curr_next)
> > + err("%s: %s: Cannot parse CPU list
> > at %s",
> > + nohz_cpus_filename, buf,
> > curr);
> > + curr = curr_next;
> > + }
> > +
> > + /* Set all bits in range */
> > + for (bit = range_first; bit <= range_last; bit++) {
> > + mask |= (1 << bit);
> > + }
> > +
> > + if ((*curr == ',') || (*curr == '\n') || (*curr ==
> > '\r'))
> > + curr++;
>
> You may simplify this part with fmemopen() and sscanf() (which deals
> with whitespaces and conversions).
>
> Once you have FILE * handle it should be as simple as:
> (beweare untested)
>
> while (sscanf("%d-%d[^0-9]*", &range_first, &range_last) ==
> 2) { //modify bitmask
> }
If going this path, I might as well open the file using SAFE_FOPEN()
followed by fscanf(). I'll see if I can get that to work, it would
simplify the code.
>
> > + }
> > +
> > + return mask;
> > +}
> > +
> > +/*
> > + * Prepare for test.
> > + * Perform CPU partitioning and start RT load.
> > + */
> > +static int setup(void)
> > +{
> > + const unsigned long nohz_mask = determine_nohz_mask();
> > + int bit;
> > + int nr_children = 0;
> > +
> > + info("Nohz CPU mask: %#lx", nohz_mask);
> > + tst_require_root(NULL);
> > + for (bit = 0; bit < (int) (sizeof (nohz_mask) * 8); bit++)
> > {
> > + if (nohz_mask & (1lu << bit)) {
> > + launch_child(load_rt, "rt", bit);
> > + nr_children++;
> > + }
> > + }
> > +
> > + info("All children started");
> > + return nr_children;
> > +}
>
> The setup should exit with tst_brkm(TCONF, "Cpuset not available") in
> case cpuset is not compiled in kernel.
>
> The easiest way would be trying access() on suitable sysfs path such
> as "/sys/fs/cgroup/cpuset/".
Makes sense. I'll fix this.
>
> > +/*
> > + * Test main function.
> > + * Perform tick measurement for the given number of seconds.
> > + */
> > +static void test(time_t duration)
> > +{
> > + uint64_t nr_ticks;
> > + time_t time_finished;
> > + /* If sched_tick_max_deferment patch has been applied,
> > expect that the
> > + * partitioning has disabled ticks completely. Otherwise,
> > expect
> > + * 1Hz ticks */
> > + const int expect_0hz =
> > +
> > access("/sys/kernel/debug/sched_tick_max_deferment", F_OK) == 0;
>
> This is a bit hacky, I would just use if (...) instead.
I like const declaration, they make things less error prone. So
personally I prefer small amount of hackiness to buy me less error
prone code... But this is a matter of taste.
>
> > + const time_t current_time = time(NULL);
> > +
> > + const unsigned int nr_children = setup();
>
> Returning number of children from setup is a bit confusing. Maybe it
> would be better to make nr_children global variable or rename the
> function.
The name "setup" doesn't really give any hints on what is returned,
only what is being done. I'm not confused by it, but I guess I'm
biased...
Would you prefer that "setup" is renamed to "setup_return_nr_children"?
>
> > + const uint64_t expected_ticks = expect_0hz ? 0 : duration
> > * nr_children; +
> > + shell("count_ticks --cpu rt --start");
> > +
> > + time_finished = time(NULL) + duration;
> > +
> > + info("Execution started: %s", ctime(¤t_time));
> > + info("Execution ends : %s", ctime(&time_finished));
> > +
> > + while (time(NULL) != time_finished);
>
> This is not safe, it should be time(NULL) > time_finished, because
> otherwise the test will spin forever if the one exact second was
> missed, either due to machine load or because ntp daenom that has
> updated system time while this loop was running.
>
> Generally it would be better to use clock_gettime() with
> CLOCK_MONOTONIC_RAW that is not subject to adjustements.
When thinking about it, this code isn't run in the RT partition, which
means that timeouts are ok. So I could use timerfd_* for the timeout.
>
> > + nr_ticks = shell_int(0, "count_ticks --cpu rt --batch
> > --end"); +
> > + if (expect_0hz) {
> > + if (nr_ticks != 0)
> > + fail("Expected no ticks, but got %"
> > PRIu64, nr_ticks);
> > + else
> > + pass("No ticks occurred");
> > + } else {
> > + if (nr_ticks > expected_ticks)
> > + fail("Expected maximum %" PRIu64 " ticks,
> > but got %" PRIu64,
> > + expected_ticks, nr_ticks);
> > + else
> > + pass("%" PRIu64 " ticks occurred, which
> > was expected",
> > + nr_ticks);
> > + }
> > +
> > + cleanup();
> > + tst_exit();
> > +}
> > +
> > +/*
> > + * Show usage text and exit.
> > + */
> > +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, --help Display this usage text and
> > exit.\n"
> > + " -d, --duration Number of seconds to run the
> > test.\n"
> > + ,
> > + appname, appname
> > + );
> > +
> > + exit(1);
> > +}
> > +
> > +/*
> > + * Program entry
> > + */
>
> Again commenting the obvious.
Ok.
>
> > +int main(int argc, char *argv[])
> > +{
> > + time_t duration = 0;
> > + int done = 0;
> > +
> > + static const struct option options[] = {
> > + {"verbose", optional_argument, NULL, 'v'},
> > + {"duration", required_argument, NULL, 'd'},
> > + {"help", no_argument, NULL, 'h'}
> > + };
> > +
> > + static const char short_options[] = "v::d:p:h";
> > +
> > + appname = basename(argv[0]);
> > +
> > + while (!done) {
> > + const int opt = getopt_long(
> > + argc, argv, short_options, options, NULL);
> > +
> > + switch (opt) {
> > + case -1:
> > + done = 1;
> > + break;
> > + case '?':
> > + err("Use '--help' for usage");
> > + case 'h':
> > + usage();
> > + case 'd':
> > + duration =
> > + (time_t) str_to_ulong(optarg,
> > "--duration", 0);
> > + break;
> > + case 'v':
> > + if (optarg != NULL)
> > + verbose = str_to_ulong(optarg,
> > "--verbose", 0);
> > + else
> > + verbose = 1;
> > + break;
> > + default:
> > + err("-%c: Missing implementation for
> > argument", opt);
> > + }
> > +
> > + }
>
> Use LTP parse_opts() instead.
Ok.
>
> > + if (duration == 0)
> > + err("No duration specified, nothing to do");
> > +
> > + info("%s: Compiled %s %s", __FILE__, __DATE__, __TIME__);
> > + info("This test uses partrt, list2mask and count_ticks
> > tools");
> > + info("These tools can be found at:
> > <https://github.com/OpenEneaLinux/rt-tools>");
> > + info("Make sure you have run 'partrt create <cpumask>'
> > before this test"); +
> > + test(duration);
>
> You should do the standard test looping here (as explained in the
> example testcase in the Test Writing Guidelines).
>
> I.e.
> for (lc = 0; TEST_LOOPING(lc); lc++)
> test(duration);
Ok.
>
> > + /* Should not end up here */
> > + return 1;
>
> You should have used cleanup() and tst_exit() here instead.
>
Ok.
Regards
Mats Liljegren
------------------------------------------------------------------------------
Start Your Social Network Today - Download eXo Platform
Build your Enterprise Intranet with eXo Platform Software
Java Based Open Source Intranet - Social, Extensible, Cloud Ready
Get Started Now And Turn Your Intranet Into A Collaboration Platform
http://p.sf.net/sfu/ExoPlatform
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
