Add an extensible phase workload definition functionality. #54

jmarinho · 2018-08-29T10:56:00Z

Allow phases to execute workloads from shared objects present in the rtapp_function/ directory.
This allows for a more straightforward integration of external workloads with rt-app, ideally it makes it a less cumbersome exercise. It would allow, for instance, NEON kernels or any other particular code to be used in rt-app more easily. The workloads can be easily contributed back to the rt-app repo and will not contribute to rt-app base src pollution since they are fairly isolated.

jlelli · 2018-09-13T13:23:59Z

Got the following while compiling (gcc (GCC) 8.1.1)

$ make
Making all in libdl
make[1]: Entering directory '/home/juri/work/scheduler-tools/rt-app/libdl'
gcc -DHAVE_CONFIG_H -I. -I../src     -g -O2 -MT dl_syscalls.o -MD -MP -MF .deps/dl_syscalls.Tpo -c -o dl_syscalls.o dl_syscalls.c
mv -f .deps/dl_syscalls.Tpo .deps/dl_syscalls.Po
rm -f libdl.a
ar cru libdl.a dl_syscalls.o
ranlib libdl.a
make[1]: Leaving directory '/home/juri/work/scheduler-tools/rt-app/libdl'
Making all in src
make[1]: Entering directory '/home/juri/work/scheduler-tools/rt-app/src'
make  all-am
make[2]: Entering directory '/home/juri/work/scheduler-tools/rt-app/src'
gcc -DHAVE_CONFIG_H -I.  -I./../libdl/   -g -O2 -MT rt-app_utils.o -MD -MP -MF .deps/rt-app_utils.Tpo -c -o rt-app_utils.o rt-app_utils.c
mv -f .deps/rt-app_utils.Tpo .deps/rt-app_utils.Po
gcc -DHAVE_CONFIG_H -I.  -I./../libdl/   -g -O2 -MT rt-app_args.o -MD -MP -MF .deps/rt-app_args.Tpo -c -o rt-app_args.o rt-app_args.c
mv -f .deps/rt-app_args.Tpo .deps/rt-app_args.Po
gcc -DHAVE_CONFIG_H -I.  -I./../libdl/   -g -O2 -MT rt-app.o -MD -MP -MF .deps/rt-app.Tpo -c -o rt-app.o rt-app.c
mv -f .deps/rt-app.Tpo .deps/rt-app.Po
gcc -DHAVE_CONFIG_H -I.  -I./../libdl/   -g -O2 -MT rt-app_parse_config.o -MD -MP -MF .deps/rt-app_parse_config.Tpo -c -o rt-app_parse_config.o rt-app_parse_config.c
rt-app_parse_config.c: In function ‘initialise_external_library’:
rt-app_parse_config.c:385:2: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
  strncpy(full_library_name, shared_library_dir_name, base_path_len);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
rt-app_parse_config.c:380:22: note: length computed here
  int base_path_len = strlen(shared_library_dir_name);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mv -f .deps/rt-app_parse_config.Tpo .deps/rt-app_parse_config.Po
/bin/sh ../libtool  --tag=CC   --mode=link gcc  -g -O2   -o rt-app rt-app_utils.o rt-app_args.o rt-app.o rt-app_parse_config.o  ../libdl/libdl.a -ljson-c -ldl -lrt -lm -lpthread
libtool: link: gcc -g -O2 -o rt-app rt-app_utils.o rt-app_args.o rt-app.o rt-app_parse_config.o  ../libdl/libdl.a -ljson-c -ldl -lrt -lm -lpthread
make[2]: Leaving directory '/home/juri/work/scheduler-tools/rt-app/src'
make[1]: Leaving directory '/home/juri/work/scheduler-tools/rt-app/src'
Making all in rtapp_function
make[1]: Entering directory '/home/juri/work/scheduler-tools/rt-app/rtapp_function'
*** building external workload fpBench/fp
gcc fpBench/fp.c  -rdynamic -shared -fPIC -o fpBench/fp.so
make[1]: Leaving directory '/home/juri/work/scheduler-tools/rt-app/rtapp_function'
make[1]: Entering directory '/home/juri/work/scheduler-tools/rt-app'
make[1]: Nothing to be done for 'all-am'.
make[1]: Leaving directory '/home/juri/work/scheduler-tools/rt-app'

Please fix.

jmarinho · 2018-09-13T16:50:19Z

fixed the warning at rt-app_parse_config.c:385 .

jlelli · 2018-09-14T15:21:05Z

Unfortunately this still has problems:

rt-app_parse_config.c: In function ‘initialise_external_library’:
rt-app_parse_config.c:386:2: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
  strncpy(full_library_name, shared_library_dir_name, full_library_name_length);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
rt-app_parse_config.c:380:22: note: length computed here
  int base_path_len = strlen(shared_library_dir_name);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

AFAIK snprintf is actually to prefer to strncpy (e.g., https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8/ - Handling truncation when it occurs)

jmarinho · 2018-09-18T08:09:38Z

Removed strncpy and substituted those code spinets with snprintf. Gcc 8.1 with -Wall does not issue the warnings you were referring to. Which C flags are you using in the compilation?

jlelli · 2018-09-18T08:35:12Z

doc/examples/shared_object_workload.json

+            }
+		}
+	}
+}


This doesn't seem to produce any sort of logs or numbers.
How is this supposed to be used? How the user get to know what happended or how the external workload performed?

Made the example a bit more complex so as to show a potential user how to manage the functionality. The external_workload should behave as a regular event. The execution of the external workload produces debug output (when that log level is selected) in the rt-app.c

This should probably be moved to doc/examples/tutorial/example9.json

Also please include a global section like the following

"global" : { "calibration" : "CPU0", "default_policy" : "SCHED_OTHER", "pi_enabled" : false, "lock_pages" : false, "logdir" : "./", "log_basename" : "rt-app9", "ftrace" : false, "gnuplot" : false, }

and indent tasks as for the other examples

jlelli · 2018-09-18T08:48:15Z

src/rt-app_args.c

+	if (!realpath(argv[0], full_path))
+	{
+		log_error("failed to get rt-app base path: %s", strerror(errno));
+		exit(0);


Please use exit(EXIT_FAILURE) here and where appropriate.

jlelli · 2018-09-18T09:02:56Z

src/rt-app_args.c

+		exit(0);
+	}
+
+    snprintf(main_install_path,main_install_path_len,"%s%s", full_path, shared_lib_dir_name);


Guess we should check for errors?
Also please align properly with a tab.

jlelli · 2018-09-18T09:06:15Z

src/rt-app_parse_config.c

+
+		/*if (!json_object_is_type(obj, json_type_string))
+			goto unknown_event;
+		*/


Why is this commented out?

removed comment

jlelli · 2018-09-18T09:23:43Z

src/rt-app_parse_config.c

+		* Get the name of the shared library as well as
+		* the optional method name
+		*/
+		char *tmp = get_string_value_from(obj, "library_name", FALSE, "");


Do you really need the tmp variable?

removed tmp declaration

jlelli · 2018-09-18T09:24:42Z

src/rt-app_parse_config.c

+	char const* shared_library_dir_name = get_shared_library_base_path();
+	if (!shared_library_dir_name)
+	{
+		log_error("failed to obtain shared library directory\n");


exit(EXIT_FAILURE)?
Also please don't mix declarations and code.

jlelli · 2018-09-18T09:26:46Z

src/rt-app_parse_config.c

 	return tmp;
 }

+void initialise_external_library(rtapp_resource_t *library_struct)


simply rdata maybe (as it contains more data that the one strictly associated with the library)?

I am having difficulties understanding what the comment refers to. Can you please rephrase?

Oh, sorry, simply a huge nitpick.. I'd s/library_struct/rdata/ :-)

changed library_struct to rdata

jlelli · 2018-09-18T09:27:18Z

src/rt-app_parse_config.c

+
+	char *full_library_name = malloc(full_library_name_length);
+
+	snprintf(full_library_name, full_library_name_length, "%s%s", shared_library_dir_name, library_name);


Check for errors.

jlelli · 2018-09-18T09:35:33Z

src/rt-app.c

 		break;
+    case rtapp_external_workload:
+        rdata->res.external_workload.workload();
+        break;


Please align with tabs.

jlelli · 2018-09-18T09:36:50Z

rtapp_function/fpBench/fp.c

+
+        Adapted from Press et al., "Numerical Recipes in C".
+
+*/


Did you write this yourself or is the code taken from some other source?

This is an floating point benchmark obtained from some external source. It was uploaded with the intent of showcasing that complex code can be ran as a rt-app phase. This probably should not be kept as part of the PR.

removed fp benchmark.

jlelli · 2018-09-18T09:39:23Z

rtapp_function/Makefile

@@ -0,0 +1,14 @@
+SOURCE = $(wildcard */*.c)


Mmm, I'm wondering if pulling "random" code into rt-app sources is really a good idea.
I guess we are basically subscribing to maintaining all the snippets we import.. :-(
Alternatives?

Agree, this should be mostly a mechanism to allow for random code to be used as rt-app phases. Only very select and generic code snippets should be committed to rt-app. The current workload which is part of the PR has the purpose of showcasing that the mechanism works for complex workloads, it should not be part of the PR when it hypothetically gets merged.

Mmm OK. I wonder what's the best way to make life easy for potential users of this new feature, though. I guess having a very simple example in the repo would certainly help. And I also guess adding substantial documentation to tutorial.txt should help as well.

created a simple workload which just prints an integer. The workload has two methods that behave differently with respect to the integer increment.

Added documentation to doc/tutorial.txt detailing the external_workload event.

jlelli · 2018-09-24T08:53:08Z

src/rt-app_args.c

 	{0,		0,		0,	0}
 };

+static char* main_install_path = NULL;


No need to initialize this to NULL.

removed initialisation.

jlelli · 2018-09-24T08:55:31Z

src/rt-app_args.c

+	int full_path_len;
+	int main_install_path_len;
+
+	static const char shared_lib_dir_name[] = "/../rtapp_function/";


Why static?

removed static.

jlelli · 2018-09-24T08:55:50Z

src/rt-app_args.c

+	/* get the rt-app base directory full name */
+	char* full_path = malloc(PATH_MAX);
+	int index;
+    int return_snprintf = 0;


Fix the indentation.

jlelli · 2018-09-24T08:58:14Z

src/rt-app_args.c

+
+	/* get the program invocation string and extract the base directory content */
+	if (!realpath(argv[0], full_path))
+	{


Nitpick!
Not sure we enforce this everywhere, but I like better parenthesis to be aligned with if and for statements.

if (condition) { } for (;;) { }

moved curlies on all relevant lines.

jlelli · 2018-09-24T09:04:28Z

src/rt-app_args.c

+		exit(EXIT_FAILURE);
+	}
+
+    return_snprintf = snprintf(main_install_path, main_install_path_len,"%s%s", full_path, shared_lib_dir_name);


Fix indentation.

jlelli · 2018-09-24T09:28:14Z

rtapp_function/test_workload/test.c

+int test1() {
+
+    testVar++;
+    printf("first test %d\n", testVar);


Mmm, this breaks rt-app log_* output format. :-/
Wonder if we can make that available to external workloads and actually enforce that is used?

Are the external workloads generally expected to call printf or do any logging for that matter? Is this really required?

jlelli · 2018-09-24T10:07:17Z

doc/examples/shared_object_workload.json

+            }
+		}
+	}
+}


This should probably be moved to doc/examples/tutorial/example9.json

Also please include a global section like the following

"global" : { "calibration" : "CPU0", "default_policy" : "SCHED_OTHER", "pi_enabled" : false, "lock_pages" : false, "logdir" : "./", "log_basename" : "rt-app9", "ftrace" : false, "gnuplot" : false, }

and indent tasks as for the other examples

jlelli · 2018-09-24T10:08:51Z

doc/tutorial.txt

 * yield: String. Calls pthread_yield(), freeing the CPU for other tasks. This has a
 special meaning for SCHED_DEADLINE tasks. String can be empty.

+* external_workload: Executes a function declared in a shared library. The shared


Add type (e.g. sync event).

jlelli · 2018-09-24T10:10:33Z

doc/tutorial.txt

 special meaning for SCHED_DEADLINE tasks. String can be empty.

+* external_workload: Executes a function declared in a shared library. The shared
+libraries are stored at rtapp_function/<workload base directory>/ .For each c file


Nitpick. Space after full stop.

jlelli · 2018-09-24T10:10:59Z

doc/tutorial.txt

+* external_workload: Executes a function declared in a shared library. The shared
+libraries are stored at rtapp_function/<workload base directory>/ .For each c file
+present in the workload base directory a shared library is compiled. The external_workload
+must is defined, in the rt-app taskset definition, by two fields:


Add support for phases declared as "external_workload".

Execute the method previously loaded from the shared object of an "external_workload" phase.

New external test workload added to rtapp_function/test. The json example usage is at doc/examples/shared_object_workload.json

jlelli requested review from jlelli and vingu-linaro August 29, 2018 12:01

jlelli added the enhancement label Sep 10, 2018

jmarinho force-pushed the master branch from 132e6fd to 00e8209 Compare September 13, 2018 16:46

jmarinho force-pushed the master branch 3 times, most recently from e75c1b7 to 4150d87 Compare September 17, 2018 16:17

jlelli reviewed Sep 18, 2018

View reviewed changes

jmarinho force-pushed the master branch from 4150d87 to e5b34e2 Compare September 18, 2018 08:48

jlelli reviewed Sep 18, 2018

View reviewed changes

jmarinho force-pushed the master branch from e5b34e2 to 2d8fbd9 Compare September 18, 2018 09:03

jlelli requested changes Sep 18, 2018

View reviewed changes

jlelli reviewed Sep 18, 2018

View reviewed changes

jmarinho force-pushed the master branch 7 times, most recently from d3d2c4f to 53469a7 Compare September 21, 2018 16:06

jlelli requested changes Sep 24, 2018

View reviewed changes

Load symbol from shared object.

ccb1432

Add support for phases declared as "external_workload".

jmarinho force-pushed the master branch from 53469a7 to dd37ccb Compare September 26, 2018 15:11

jmarinho force-pushed the master branch from dd37ccb to fd4da88 Compare October 8, 2018 10:20

jmarinho added 3 commits October 8, 2018 11:35

Execute method declared from shared object.

1baa527

Execute the method previously loaded from the shared object of an "external_workload" phase.

Add external workload and example json usage.

8c21c78

New external test workload added to rtapp_function/test. The json example usage is at doc/examples/shared_object_workload.json

Add external_workload info onto doc/tutorial.tex

c2354c0

jmarinho force-pushed the master branch from fd4da88 to c2354c0 Compare October 8, 2018 10:35


		char *full_library_name = malloc(full_library_name_length);

		snprintf(full_library_name, full_library_name_length, "%s%s", shared_library_dir_name, library_name);

Add an extensible phase workload definition functionality. #54

Are you sure you want to change the base?

Add an extensible phase workload definition functionality. #54

Uh oh!

Conversation

jmarinho commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlelli commented Sep 13, 2018

Uh oh!

jmarinho commented Sep 13, 2018

Uh oh!

jlelli commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarinho commented Sep 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jmarinho commented Aug 29, 2018 •

edited

Loading

jlelli commented Sep 14, 2018 •

edited

Loading