OP-TEE Benchmark #79

igoropaniuk · 2017-02-22T16:03:28Z

Related PRs:

Benchmark app:
linaro-swg/optee_benchmark#1

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

etienne-lms · 2017-02-27T13:25:38Z

libteec/src/tee_bench_aux.c

+struct statistics *bench_stat_buf;
+
+#ifdef CFG_TEE_BENCHMARK
+static const TEEC_UUID sta_benchmark_uuid = BENCHMARK_STA_UUID;


minor: rather user pta_xxx than sta_xxx. "static TA" is a deprecated reference.
here and there.

etienne-lms · 2017-02-27T13:27:10Z

libteec/src/tee_bench_aux.c

+	double delta = x - s->m;
+
+	s->n++;
+	s->m += delta/s->n;


minor: spaces around /

removed all statistics handling code

etienne-lms · 2017-02-27T13:32:45Z

libteec/src/tee_bench_aux.c

+	offset = (off_t)hw_addr % getpagesize();
+	page_addr = (off_t)(hw_addr - offset);
+
+	hw_addr = (intptr_t *)mmap(0, 4, PROT_READ|PROT_WRITE,


should check that mmap() succeeded.

etienne-lms · 2017-02-27T13:36:24Z

libteec/src/tee_client_api.c

@@ -553,6 +552,11 @@ TEEC_Result TEEC_InvokeCommand(TEEC_Session *session, uint32_t cmd_id,
 		goto out;
 	}

+	if ((cmd_id & BENCHMARK_CMD(0)) != BENCHMARK_CMD(0)) {


This expects the 0xFA19xxxx IDs are reserved to the benchmark pseudo TA. This is not fair.

Right, but the only one alternative(IMHO) is to hijack(how it's done for benchmark buf) session_id (opened to Benchmark PTA) and store it in all OP_TEE layers
If it's definitely better solution, I'll use it

fixed. Now timestamp buffer is registered via RPC call from SW

etienne-lms · 2017-02-27T13:37:11Z

public/tee_bench.h

+struct statistics {
+	int n;
+	double m;
+	double M2;


minor: i would rather reserve the capital letters to macros.

etienne-lms · 2017-02-27T13:38:56Z

public/tee_bench.h

+{
+	uintptr_t pc;
+#ifdef __aarch64__
+	asm volatile("adr %0, ." : "=r"(pc));


asm volatile("adr %0, ." : "=r"(pc)); should work for both AArch32 and AArch64.

etienne-lms · 2017-03-08T08:00:28Z

This PR seems to deprecate #63. If so, could you close it ?

jbech-linaro · 2017-04-26T12:50:53Z

libteec/include/linux/teec_benchmark.h

+#define __TEEC_BENCHMARK_H
+
+#include <linux/types.h>
+#include <stdbool.h>


Is this needed here?

jbech-linaro · 2017-04-26T12:51:26Z

libteec/include/linux/teec_benchmark.h

+#include <linux/types.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <sys/types.h>


jbech-linaro · 2017-04-26T12:59:54Z

libteec/src/teec_benchmark.c

+	return ccounter * TEE_BENCH_DIVIDER;
+}
+
+/* Misc auxilary functions */


Doesn't matter now, but in the long run we'd better move it into teec_trace.c (and cleanup that file. I don't think the stuff in there is used any longer ... old ST-Ericsson code).

etienne-lms · 2017-04-18T09:08:02Z

libteec/src/tee_client_api.c

@@ -553,6 +553,12 @@ TEEC_Result TEEC_InvokeCommand(TEEC_Session *session, uint32_t cmd_id,
 		goto out;
 	}

+	if (!pthread_mutex_trylock(&teec_bench_mu)) {


Trylock induce some memory barrier. If bench is disabled, these barriers are useless. Could we condition this to CFG_TEE_BENCHMARK ?

etienne-lms · 2017-04-18T09:13:44Z

libteec/src/teec_benchmark.c

+
+	/* fill timestamp data */
+	if (cur_cpu >= bench_ts_global->cores)
+		return;


Restore cpu affinity before returning.

etienne-lms · 2017-04-18T09:14:28Z

libteec/src/teec_benchmark.c

+
+	/* set back affinity mask */
+	ret = sched_setaffinity(0, sizeof(cpu_set_old), &cpu_set_old);
+	if (ret)


can void the return value.

etienne-lms · 2017-05-02T10:57:52Z

libteec/src/teec_benchmark.c

+
+#define PTA_BENCHMARK_UUID \
+		{ 0x0b9a63b0, 0xb4c6, 0x4c85, \
+			{ 0xa2, 0x84, 0xa2, 0x28, 0xef, 0x54, 0x7b, 0x4e } }


minor: could move to tee_bench.h next to pTA command IDs definition.

etienne-lms · 2017-05-02T11:03:32Z

libteec/src/teec_benchmark.c

+	TEEC_Operation op = { 0 };
+	uint32_t ret_orig;
+
+	benchmark_pta_open();


If this fails, the command should not be invoked.

etienne-lms · 2017-05-02T11:04:03Z

libteec/src/teec_benchmark.c

+
+	res = TEEC_InvokeCommand(&bench_sess, BENCHMARK_CMD_GET_MEMREF,
+					&op, &ret_orig);
+	tee_check_res(res, "TEEC_InvokeCommand");


If this fails, *paddr_ts_buf should not be filled.

etienne-lms · 2017-05-02T11:04:26Z

libteec/src/teec_benchmark.c

+	int   devmem;
+	off_t offset = 0;
+	off_t page_addr;
+


minor: empty line

etienne-lms · 2017-05-02T11:06:06Z

libteec/src/teec_benchmark.c

+	hw_addr = (intptr_t *)mmap(0, 4, PROT_READ|PROT_WRITE,
+					MAP_SHARED, devmem, page_addr);
+	if (hw_addr == MAP_FAILED)
+		return 0;


minor: return NULL

etienne-lms · 2017-05-02T11:15:49Z

libteec/src/teec_benchmark.c

+	cpu_buf->stamps[ts_i & TEE_BENCH_MAX_MASK] = ts_data;
+
+	/* set back affinity mask */
+	ret = sched_setaffinity(0, sizeof(cpu_set_old), &cpu_set_old);


minor: drop return value.

etienne-lms · 2017-05-02T11:18:08Z

public/tee_bench.h

+/*
+ * Copyright (c) 2017, Linaro Limited
+ *
+ * This software is licensed under the terms of the GNU General Public


Prefer the BSD style licence.

etienne-lms · 2017-05-02T11:20:08Z

public/tee_bench.h

+#define BENCHMARK_CMD_GET_MEMREF		BENCHMARK_CMD(2)
+#define BENCHMARK_CMD_UNREGISTER		BENCHMARK_CMD(3)
+
+#define TEE_BENCH_DIVIDER		64


Add same comment as in optee_os/core/.../bench.h:

/* * Cycle count divider is enabled (in PMCR), * CCNT value is incremented every 64th clock cycle */

etienne-lms · 2017-05-02T11:25:45Z

libteec/src/teec_benchmark.c

+	offset = (off_t)hw_addr % getpagesize();
+	page_addr = (off_t)(hw_addr - offset);
+
+	hw_addr = (intptr_t *)mmap(0, 4, PROT_READ|PROT_WRITE,


This request maps of only 4 bytes. Should better map up to cores field, then remap the full bench ts data structure.

etienne-lms · 2017-05-02T11:30:53Z

libteec/src/teec_benchmark.c

+
+	intptr_t *hw_addr = (intptr_t *)paddr;
+
+	devmem = open("/dev/mem", O_RDWR | O_SYNC);


Nicely warn and exit if this fails. Maybe /dev/mem is not accessible.

etienne-lms · 2017-05-05T14:03:23Z

libteec/src/teec_benchmark.c

 	op.paramTypes = TEEC_PARAM_TYPES(TEEC_VALUE_OUTPUT, TEEC_NONE,
 					TEEC_NONE, TEEC_NONE);

 	res = TEEC_InvokeCommand(&bench_sess, BENCHMARK_CMD_GET_MEMREF,
 					&op, &ret_orig);
-	tee_check_res(res, "TEEC_InvokeCommand");
+	if (res != TEEC_SUCCESS)
+		return res;


benchmark_pta_close();

etienne-lms · 2017-05-05T14:06:37Z

libteec/include/linux/teec_benchmark.h

+#include <stdbool.h>
+#include <tee_bench.h>
+
+void bm_init_ts_buf(void);


minor: deprecated bm_init_ts_buf()

etienne-lms · 2017-05-05T14:07:01Z

libteec/src/teec_benchmark.c

+	sched_setaffinity(0, sizeof(cpu_set_old), &cpu_set_old);
+}
+#else /* CFG_TEE_BENCHMARK */
+void bm_init_ts_buf(void) {}


minor: deprecated bm_init_ts_buf()

etienne-lms · 2017-05-05T14:08:55Z

libteec/src/tee_client_api.c

@@ -553,6 +554,14 @@ TEEC_Result TEEC_InvokeCommand(TEEC_Session *session, uint32_t cmd_id,
 		goto out;
 	}

+#ifdef CFG_TEE_BENCHMARK
+	if (!pthread_mutex_trylock(&teec_bench_mu)) {


If the lock is held, this will not get any timestamp. Is this expected ? (maybe this has already been discussed)

The full sequence (including lock and get-buffer) could be implemented inside bm_timestamp()`.

etienne-lms · 2017-05-05T14:18:22Z

libteec/src/teec_benchmark.c

+	page_addr = (off_t)(hw_addr - offset);
+
+	hw_addr = (intptr_t *)mmap(0, size, PROT_READ|PROT_WRITE,
+					MAP_SHARED, devmem, page_addr);


Can (should) close(devmem); from here (after mmap() returns).

igoropaniuk · 2017-05-19T11:57:42Z

Rebased on latest master and tested

etienne-lms · 2017-05-19T13:39:25Z

libteec/src/teec_benchmark.c

+{
+	uint64_t ts_buf_raw = 0;
+	uint64_t ts_buf_size = 0;
+


style: empty line

etienne-lms · 2017-05-19T13:41:13Z

libteec/src/teec_benchmark.c

+	ret_addr = __builtin_return_address(0);
+
+	cpu_buf = &bench_ts_global->cpu_buf[cur_cpu];
+	ts_i = __sync_fetch_and_add(&cpu_buf->head, 1);


minor: useless usage of __sync_fetch_and_add(), you old a lock.

@etienne-lms why useless?
we can be preempted here

your lock + cpu affinity prevent another thread from accessing the buffer.

sched_setaffinity call just tells linux scheduler that this thread should be scheduled on specified core (or cores), it doesn't somehow affect preemption (if to compare with get_cpu()/put_cpu() in the linux kernel)

It's theoretically possible, that while running simple non-atomic cpu_buf->head++ thread will be preempted, and another one will use non-actual cpu-buf->head value. Or maybe I just didn't get what actually you meant

@etienne-lms

Ok, understood. Thanks.

etienne-lms · 2017-05-19T13:41:45Z

libteec/src/teec_benchmark.c

+	cpu_buf = &bench_ts_global->cpu_buf[cur_cpu];
+	ts_i = __sync_fetch_and_add(&cpu_buf->head, 1);
+	ts_data.cnt = read_ccounter();
+	ts_data.addr = (uintptr_t) ret_addr;


style: space between cast and cast'ed.

etienne-lms · 2017-05-19T13:43:50Z

public/tee_bench.h

+
+#include <inttypes.h>
+
+#define UNUSED(x) (void)(x)


minor: i fear this may conflict with some other external definition of UNUSED().

etienne-lms

With minor style comment addressed or not: Reviewed-by: Etienne Carriere <[email protected]>

etienne-lms · 2017-05-22T11:38:06Z

Makefile

-	@$(MAKE) --directory=libteec --no-print-directory --no-builtin-variables
+	@$(MAKE) --directory=libteec CFG_TEE_BENCHMARK=$(CFG_TEE_BENCHMARK) \
+	CFG_TEE_PRINT_LATENCY_STAT=$(CFG_TEE_PRINT_LATENCY_STAT) \
+	--no-print-directory --no-builtin-variables


minor style: maybe insert an indentation in the 2 later line.

etienne-lms · 2017-05-22T13:42:49Z

Makefile

@@ -29,7 +29,9 @@ all: build install

 build-libteec:
 	@echo "Building libteec.so"
-	@$(MAKE) --directory=libteec --no-print-directory --no-builtin-variables
+	@$(MAKE) --directory=libteec CFG_TEE_BENCHMARK=$(CFG_TEE_BENCHMARK) \
+	CFG_TEE_PRINT_LATENCY_STAT=$(CFG_TEE_PRINT_LATENCY_STAT) \


CFG_TEE_PRINT_LATENCY_STAT seems not used. Maybe discard it until some code rely on its value.

igoropaniuk · 2017-05-23T10:55:48Z

Rebased & applied tags

OP-TEE Benchmark feature provides 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]>

This was referenced Feb 22, 2017

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

OP-TEE benchmark: initial benchmark app linaro-swg/optee_benchmark#1

Merged

igoropaniuk force-pushed the optee_benchmark branch 6 times, most recently from 8a3391c to 80880d5 Compare February 23, 2017 19:03

etienne-lms reviewed Feb 27, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 80880d5 to 52c8079 Compare April 9, 2017 16:47

igoropaniuk changed the title ~~[RFC] OP-TEE Benchmark~~ OP-TEE Benchmark Apr 18, 2017

igoropaniuk force-pushed the optee_benchmark branch from b847b41 to b37b86c Compare April 19, 2017 14:31

jbech-linaro reviewed Apr 26, 2017

View reviewed changes

etienne-lms reviewed Apr 26, 2017

View reviewed changes

etienne-lms reviewed May 2, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 7f6e69d to 0dd7193 Compare May 5, 2017 12:24

etienne-lms reviewed May 5, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch 2 times, most recently from 013d0dd to 8c567f6 Compare May 19, 2017 11:49

igoropaniuk force-pushed the optee_benchmark branch from 8c567f6 to 5b007c7 Compare May 19, 2017 12:12

etienne-lms reviewed May 19, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch from 5b007c7 to 7e26c0a Compare May 19, 2017 15:29

etienne-lms reviewed May 22, 2017

View reviewed changes

igoropaniuk force-pushed the optee_benchmark branch 2 times, most recently from 0258a56 to 113ef0d Compare May 23, 2017 10:55

OP-TEE Benchmark

e64cd30

OP-TEE Benchmark feature provides 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 113ef0d to e64cd30 Compare May 30, 2017 12:18

jforissier merged commit 63106fb into OP-TEE:master May 30, 2017

igoropaniuk deleted the optee_benchmark branch July 4, 2017 17:15


		intptr_t hw_addr = (intptr_t )paddr;

		devmem = open("/dev/mem", O_RDWR \| O_SYNC);

OP-TEE Benchmark #79

OP-TEE Benchmark #79

Conversation

igoropaniuk commented Feb 22, 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

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

etienne-lms commented Mar 8, 2017

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

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

igoropaniuk commented May 19, 2017

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

igoropaniuk May 19, 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

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

igoropaniuk commented May 23, 2017

igoropaniuk commented Feb 22, 2017 •

edited

Loading

igoropaniuk May 19, 2017 •

edited

Loading