OP-TEE benchmark: initial benchmark app #1

igoropaniuk · 2017-02-22T15:51:52Z

Related PRs:

Benchmark app:
#1

Changes in OP-TEE
linaro-swg/linux#38
OP-TEE/build#124
OP-TEE/optee_os#1365
OP-TEE/optee_client#79

Dedicated repo manifest (for testing):

repo init -u https://github.com/igoropaniuk/manifest.git -m qemu_v7_benchmark.xml -b benchmark

Invocation:

# benchmark client_app [client_app params]

Example:

# benchmark ./xtest 1001

Signed-off-by: Igor Opaniuk [email protected]

igoropaniuk · 2017-04-19T15:35:17Z

main.c

+						&ts_data);
+			if (!ret) {
+				ts_received = true;
+				printf("Timestamp CPU = %ld"


Adding now code for dumping timestamps to CSV file instead of this debug output

igoropaniuk · 2017-04-19T15:36:51Z

@etienne-lms @jenswi-linaro Could check please also this PR, it includes almost 40-50% of the functionality. Thanks

etienne-lms

Looks ok.
Nice to benchmark a specific executable file.

Maybe the benchmark tool could also enable benchmark and sleep until it is killed. Launching benchmark --enable would allow benchmarking already running CAs and TAs.

etienne-lms · 2017-04-19T15:49:24Z

benchmark_aux.c

+
+		testapp_argv[i - 1] = malloc(length);
+		if (i == 1)
+			memcpy(testapp_argv[i-1], base, strlen(base) + 1);


Allocate strlen(base) for testapp_argv[0]. Maybe argv0 should be processed before the for loop.

igoropaniuk · 2017-04-21T10:34:37Z

Maybe the benchmark tool could also enable benchmark and sleep until it is killed. Launching benchmark --enable would allow benchmarking already running CAs and TAs.

@etienne-lms Good idea, why not. Anyway, an app collects system-wide timestamps in both cases

etienne-lms · 2017-04-21T11:05:19Z

Don't it be hard to find which timestamp belong to which TEE session ? Adding a session ID to stamps could even help filtering out sessions for which no time-stamping is requested.

etienne-lms

2 extra minor comments.

etienne-lms · 2017-04-21T13:40:17Z

main.c

+		if (pthread_join(consumer_thread, NULL)) {
+			fprintf(stderr, "Error joining thread\n");
+			return 2;
+


minor: empty line

etienne-lms · 2017-04-21T13:41:30Z

main.c

+	else {
+		execvp(testapp_path, testapp_argv);
+		tee_errx("execve() failed", EXIT_FAILURE);
+		_exit(EXIT_FAILURE);


minor: useless _exit();

etienne-lms · 2017-04-26T13:17:21Z

main.c

+static TEEC_SharedMemory ts_buf_shm = {
+		.flags = TEEC_MEM_INPUT | TEEC_MEM_OUTPUT,
+		.buffer = NULL,
+		.size = 0


minor: these 2 inits are useless.

etienne-lms · 2017-04-26T13:27:28Z

main.c

+	uint32_t ret_orig;
+
+	ts_buf_shm.size = sizeof(struct tee_ts_global)
+			+ sizeof(struct tee_ts_cpu_buf) * cores;


Minor: prefer the + sign at the end of the former line.

etienne-lms · 2017-04-26T13:28:08Z

main.c

+	op.paramTypes = TEEC_PARAM_TYPES(TEEC_MEMREF_PARTIAL_INOUT,
+			TEEC_NONE, TEEC_NONE, TEEC_NONE);
+	op.params[0].memref.parent = &ts_buf_shm;
+	op.params[0].memref.offset = 0;


Minor: no need to set offset to 0.

etienne-lms · 2017-04-26T13:31:09Z

main.c

+		return RING_BADPARM;
+
+	if (cpu_buf->tail >= cpu_buf->head)
+		return RING_NODATA;


As referencing a ring buffer, tail might get higher than head and buffer can still hold valid content.

both head and tail are uint64_t, and they are not actually an index in the circular buffer. Index is ts_i & TEE_BENCH_MAX_MASK

tail might get higher than head and buffer can still hold valid content.

this could be true only if head overflows, which is almost an impossible case, if to consider an intensity of adding timestamps

Ok, I see your scheme.
So you can detect overflows of the buffers from (head - tail > TEE_BENCH_MAX_MASK).

Since pushing and poping data to ts buffer are concurrent, I think you insure the poped data are valid before storing them into the benchmark data file:

If no data (tail>=head), return with "no data".

Locally store ts data referred by tail.

Check for overflow (head - tail > TEE_BENCH_MAX_MASK)
If no overflow (then previous stored ts data are valid): increment tail and return the previously locally stored ts data.
If overflow found, update tail = head - TEE_BENCH_MAX_MASK, and return with a overflow status.

@etienne-lms

let's keep it as known limitation for now, I'll add support for it in future PRs, OK?

etienne-lms · 2017-04-26T13:32:01Z

main.c

+						struct tee_time_st *ts)
+{
+	struct tee_time_st ts_data;
+	uint64_t ts_tail = 0;


minor: these 2 are quite useless.

etienne-lms · 2017-04-26T13:33:35Z

main.c

+	if (cpu_buf->tail >= cpu_buf->head)
+		return RING_NODATA;
+
+	ts_tail = cpu_buf->tail++;


If tail is higher than TEE_BENCH_MAX_MASK, it should get back to 0.

not necessarily, because

ts_data = cpu_buf->stamps[ts_tail & TEE_BENCH_MAX_MASK];

ok, agree. sorry.

jbech-linaro · 2017-04-27T08:22:55Z

benchmark_aux.c

+		tee_errx(errmsg, res);
+}
+
+const char *bench_str_src(uint64_t source)


Wouldn't it be better to just simply use a lookup table in terms on a const char array? (Only drawback is not being able to catch "???").

This wasn't a good suggestion. Since the defined values or not simply from 0 to 4, you cannot use a lookup table. Having that said is there a good reason to have the values defined as (instead of 0 to 4)?

+/* OP-TEE susbsystems ids */ +#define TEE_BENCH_CLIENT 0x10000000 +#define TEE_BENCH_KMOD 0x20000000 +#define TEE_BENCH_CORE 0x30000000 +#define TEE_BENCH_UTEE 0x40000000 +#define TEE_BENCH_DUMB_TA 0xF0000001

jbech-linaro · 2017-04-27T08:34:47Z

main.c

+
+#define STAT_AMOUNT 5
+
+#define RING_SUCCESS	0


The indentation for the defines here and below are a bit weird (random spacing).

jbech-linaro · 2017-04-27T08:57:59Z

main.c

+
+#define TSFILE_NAME_SUFFIX			".ts"
+static const TEEC_UUID pta_benchmark_uuid = PTA_BENCHMARK_UUID;
+struct tee_ts_global *bench_ts_global;


I cannot find where you've declared struct tee_ts_global? I can see further down that you do for example

static void init_ts_global(struct tee_ts_global *ts_global, uint32_t cores) { ... /* init global timestamp buffer */ bench_ts_global = ts_global; bench_ts_global->cores = cores; ... }

What do I miss here? (I've tried to grep for it also).

Nevermind, I found it in the public/tee_bench.h in the optee_client PR ...

jbech-linaro · 2017-04-27T09:10:26Z

main.c

+	res = TEEC_AllocateSharedMemory(&ctx, &ts_buf_shm);
+	tee_check_res(res, "TEEC_AllocateSharedMemory");
+
+	init_ts_global((struct tee_ts_global *)ts_buf_shm.buffer, cores);


This comment is maybe a bit overkill / personal preference. But the intention with this function call is to take a buffer and transform it into a time stamp buffer. For me it sounds like this should be a void * when calling this function and after that we can refer to it as a struct tee_ts_global * buffer.

I.e, what I'm saying is, that I think I'd have changed init_ts_globalto:

static void init_ts_global(void *buf, uint32_t cores)

jbech-linaro · 2017-04-27T10:07:26Z

main.c

+
+	TEEC_InvokeCommand(&sess, BENCHMARK_CMD_REGISTER_MEMREF,
+					&op, &ret_orig);
+	tee_check_res(res, "TEEC_InvokeCommand");


I'm not a big fan of the debug prints here, since you can't tell the difference between them (this and the one on line 143 for example will print the same). Not a big deal for this particular PR/commit. But I think we should address this as soon as possible after merging this.

igoropaniuk · 2017-05-05T12:46:22Z

Don't it be hard to find which timestamp belong to which TEE session ? Adding a session ID to stamps could even help filtering out sessions for which no time-stamping is requested.

@etienne-lms Now we have a pretty generic bm_timestamp implementation, which can be theoretically called within anywhere in OP-TEE OS, Linux kernel, and optee_client, and not only in functions, that are related to InvokeCommand/OpenSession calls.

In the future, I plan to add ability to attach some payload to each timestamp, which can be TEE_Session id, whatever.

igoropaniuk · 2017-05-05T13:43:09Z

@etienne-lms

Maybe the benchmark tool could also enable benchmark and sleep until it is killed. Launching benchmark --enable would allow benchmarking already running CAs and TAs.

I thinks maybe let's add it in future PRs?

jbech-linaro · 2017-05-08T12:56:11Z

Maybe the benchmark tool could also enable benchmark and sleep until it is killed. Launching benchmark --enable would allow benchmarking already running CAs and TAs.
I thinks maybe let's add it in future PRs?

Agreed! Let's try to get the main patches merged so that we can start working with smaller and more well defined tasks.

etienne-lms · 2017-05-19T13:53:47Z

Reviewed-by: Etienne Carriere <[email protected]>

optee_benchmark application handles all provided timestamp data for the roundtrip time from libteec to OP-TEE OS core. Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Igor Opaniuk <[email protected]>

igoropaniuk · 2017-05-23T11:03:03Z

Tags applied, ready for merge

igoropaniuk · 2017-05-31T10:46:48Z

Merged

This was referenced Feb 22, 2017

OP-TEE Benchmark OP-TEE/optee_client#79

Merged

OP-TEE benchmark OP-TEE/build#124

Merged

OP-TEE Benchmark OP-TEE/optee_os#1365

Merged

OP-TEE Benchmark linaro-swg/linux#38

Merged

igoropaniuk changed the title ~~Add initial benchmark app~~ [RFC] OP-TEE benchmark: initial benchmark app Feb 22, 2017

igoropaniuk force-pushed the optee_benchmark branch 2 times, most recently from 7a91cb4 to 9fa484c Compare February 22, 2017 20:26

igoropaniuk force-pushed the optee_benchmark branch from 356cf5c to f440309 Compare April 17, 2017 17:25

igoropaniuk changed the title ~~[RFC] OP-TEE benchmark: initial benchmark app~~ OP-TEE benchmark: initial benchmark app Apr 18, 2017

igoropaniuk commented Apr 19, 2017

View reviewed changes

etienne-lms reviewed Apr 19, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 7c8120a to bf64b93 Compare April 21, 2017 13:23

etienne-lms reviewed Apr 21, 2017

View reviewed changes

etienne-lms reviewed Apr 26, 2017

View reviewed changes

jbech-linaro reviewed Apr 27, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 255058e to c8489c7 Compare May 5, 2017 12:53

Add initial optee_benchmark app

ab88c95

optee_benchmark application handles all provided timestamp data for the roundtrip time from libteec to OP-TEE OS core. Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Igor Opaniuk <[email protected]>

igoropaniuk force-pushed the optee_benchmark branch from d0a6d3d to ab88c95 Compare May 23, 2017 11:02

igoropaniuk merged commit ab88c95 into master May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OP-TEE benchmark: initial benchmark app #1

OP-TEE benchmark: initial benchmark app #1

igoropaniuk commented Feb 22, 2017 •

edited

Loading

igoropaniuk Apr 19, 2017 •

edited

Loading

igoropaniuk commented Apr 19, 2017

etienne-lms left a comment

etienne-lms Apr 19, 2017 •

edited

Loading

igoropaniuk commented Apr 21, 2017

etienne-lms commented Apr 21, 2017

etienne-lms left a comment

etienne-lms Apr 21, 2017

etienne-lms Apr 21, 2017

etienne-lms Apr 26, 2017

etienne-lms Apr 26, 2017

etienne-lms Apr 26, 2017

etienne-lms Apr 26, 2017

igoropaniuk Apr 30, 2017

etienne-lms May 2, 2017

igoropaniuk May 5, 2017

etienne-lms May 5, 2017

etienne-lms Apr 26, 2017

etienne-lms Apr 26, 2017

igoropaniuk Apr 30, 2017

etienne-lms May 2, 2017

jbech-linaro Apr 27, 2017 •

edited

Loading

jbech-linaro Apr 27, 2017 •

edited

Loading

jbech-linaro Apr 27, 2017

jbech-linaro Apr 27, 2017

jbech-linaro Apr 27, 2017

jbech-linaro Apr 27, 2017

jbech-linaro Apr 27, 2017 •

edited

Loading

igoropaniuk commented May 5, 2017

igoropaniuk commented May 5, 2017

jbech-linaro commented May 8, 2017

etienne-lms commented May 19, 2017

igoropaniuk commented May 23, 2017

igoropaniuk commented May 31, 2017


		#define STAT_AMOUNT 5

		#define RING_SUCCESS 0

OP-TEE benchmark: initial benchmark app #1

OP-TEE benchmark: initial benchmark app #1

Conversation

igoropaniuk commented Feb 22, 2017 • edited Loading

igoropaniuk Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

igoropaniuk commented Apr 19, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

igoropaniuk commented Apr 21, 2017

etienne-lms commented Apr 21, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

jbech-linaro Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

igoropaniuk commented May 5, 2017

igoropaniuk commented May 5, 2017

jbech-linaro commented May 8, 2017

etienne-lms commented May 19, 2017

igoropaniuk commented May 23, 2017

igoropaniuk commented May 31, 2017

igoropaniuk commented Feb 22, 2017 •

edited

Loading

igoropaniuk Apr 19, 2017 •

edited

Loading

etienne-lms Apr 19, 2017 •

edited

Loading

jbech-linaro Apr 27, 2017 •

edited

Loading

jbech-linaro Apr 27, 2017 •

edited

Loading

jbech-linaro Apr 27, 2017 •

edited

Loading