Dump and Reset stats data on demand using SIGUSR1 signal #1857
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
pal/src/host/linux-sgx/host_exception.c line 18 at r1 (raw file):
*/ #include <dirent.h>
I feel like you added a lot of unnecessary includes. Please remove the not-needed ones.
pal/src/host/linux-sgx/host_exception.c line 35 at r1 (raw file):
#include "ucontext.h" #define MAX_DBG_THREADS 4096
But this macro is declared somewhere else, why not #include <where-it-declared.h>?
pal/src/host/linux-sgx/host_exception.c line 38 at r1 (raw file):
static const int ASYNC_SIGNALS[] = {SIGTERM, SIGCONT}; static int send_sigusr1_signal_to_children(void);
What's the point of this declaration?
pal/src/host/linux-sgx/host_exception.c line 199 at r1 (raw file):
} static int send_sigusr1_signal_to_children() {
Need (void) here
pal/src/host/linux-sgx/host_exception.c line 203 at r1 (raw file):
for (size_t i = 1; i < MAX_DBG_THREADS; i++) { int child_tid = ((struct enclave_dbginfo*)DBGINFO_ADDR)->thread_tids[i];
TID can legitimately be 0. Please use another struct field to determine whether this array item is a legit thread.
pal/src/host/linux-sgx/host_exception.c line 222 at r1 (raw file):
static const uint64_t SLEEP_STEP = 1000000; /* 100 steps before capped */ if(g_sgx_enable_stats) {
Please change this cascade of IFs to early-returns:
if (!g_sgx_enable_stats)
return;
if (DO_SYSCALL(gettid) != g_host_pid) {
/* non-main threads, just dump thread-local stats */
}
pal/src/host/linux-sgx/host_exception.c line 229 at r1 (raw file):
uint64_t sleep_time = 0; while((no_of_children) > (__atomic_load_n(&no_of_children_visited, __ATOMIC_RELAXED))) {
You can't use RELAXED if you use ACK_REL elsewhere
pal/src/host/linux-sgx/host_exception.c line 234 at r1 (raw file):
sleep_time += SLEEP_STEP; struct timespec tv = {.tv_sec = 0, .tv_nsec = sleep_time}; (void)DO_SYSCALL(nanosleep, &tv, /*rem=*/NULL);
All these constants seem irrelevant, I suggest to just do sched_yield() instead of nanosleep. And remove all constants with LOOP and SLEEP.
pal/src/host/linux-sgx/host_exception.c line 240 at r1 (raw file):
} } update_and_print_stats(true);
Please add /*process_wide=*/
pal/src/host/linux-sgx/host_exception.c line 240 at r1 (raw file):
} } update_and_print_stats(true);
But this means that main thread does not reset his local stats?
pal/src/host/linux-sgx/host_thread.c line 75 at r1 (raw file):
__atomic_exchange_n(&g_aex_cnt, 0, __ATOMIC_ACQ_REL); __atomic_exchange_n(&g_sync_signal_cnt, 0, __ATOMIC_ACQ_REL); __atomic_exchange_n(&g_async_signal_cnt, 0, __ATOMIC_ACQ_REL);
Why exchange, and not store?
pal/src/host/linux-sgx/host_thread.c line 107 at r1 (raw file):
int ret; /* set GS reg of this thread to thread's TCB; */
Why is this needed?
TejaswineeL
changed the title
TejaswineeL
changed the title
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
TejaswineeL
force-pushed
the
debug_stats_dump_reset
branch
2 times, most recently
from
8caf722
to
5fcbfed
Compare
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/host/linux-sgx/host_exception.c line 203 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TID can legitimately be
0. Please use another struct field to determine whether this array item is a legit thread.
Approach 1:
As other structures storing children thread ids are not accessible to host_exception.c (based on the makefile),
I propose the below:
at (https://github.com/gramineproject/gramine/blob/53f5511123d51e7baf9cf98ffc7d3ad6096e6c6b/pal/src/host/linux-sgx/host_thread.c#L165)
we have g_enclave_thread_num keeping track of the count of tid assignment of the array thread_tids[]
I have an idea of using this variable to decide legit thread_tids[] inorder to send SIGUSR1 signal
we will be doing below changes if we go ahead with this idea.
https://github.com/TejaswineeL/gramine/blob/14a8be3adcb801ed4a9a1ff65afb178448111d86/pal/src/host/linux-sgx/host_exception.c#L197
/* using g_enclave_thread_num in the for loop condition check */
spinlock\_lock(&g\_enclave\_thread\_map\_lock);
for (size\_t i = 1; i < g\_enclave\_thread\_num; i++) {
int child\_tid = ((struct enclave\_dbginfo\*)DBGINFO\_ADDR)->thread\_tids\[i\];
log\_always("child tid with g\_enclave\_thread\_num %d", child\_tid);
DO\_SYSCALL(tkill, child\_tid, SIGUSR1);
signal\_counter++;
}
spinlock\_unlock(&g\_enclave\_thread\_map\_lock);
/* removing static keyword so that we can use these variables in host_exception.c file */
spinlock\_t g\_enclave\_thread\_map\_lock = INIT\_SPINLOCK\_UNLOCKED;size\_t g\_enclave\_thread\_num = 0;
stats_PR/changes_stats_or/gramine/pal/src/host/linux-sgx/pal_tcb.h
/* adding below lines to the file included in host_exception.c*/
extern size\_t g\_enclave\_thread\_num;
extern spinlock\_t g\_enclave\_thread\_map\_lock;
Approach 2:
All the children thread ids are present at location /proc/pid/task. This location only stores the main process id and children thread ids.
I can validate thread ids of the array thread_tids[] with the tids present at /proc//task during execution
Below snippet gives us the children thread ids
-defining the filepath whre we can find the children thread ids
snprintf(filepath, sizeof(filepath), "/proc/%d/task/", g\_host\_pid);
-Fetching the children tids from the filepath location with files
struct dirent \*entry;
dir = opendir(filename);
if (dir == NULL) {
perror("Error opening directory");
return -1;
}
while ((entry = readdir(dir)) != NULL) {
if (entry->d\_type == DT\_DIR && atoi(entry->d\_name) != g\_host\_pid) {
if (strcmp(entry->d\_name, ".") == 0 || strcmp(entry->d\_name, "..") == 0) {
// Skip current directory and parent directory
continue;
}
printf(" thread id from proc %s", entry->d\_name);
}
}
closedir(dir);
Kindly, Let us know which of the above approaches would you like.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/host/linux-sgx/host_exception.c line 203 at r1 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
Approach 1:
As other structures storing children thread ids are not accessible to host_exception.c (based on the makefile),
I propose the below:at (https://github.com/gramineproject/gramine/blob/53f5511123d51e7baf9cf98ffc7d3ad6096e6c6b/pal/src/host/linux-sgx/host_thread.c#L165)
we have g_enclave_thread_num keeping track of the count of tid assignment of the array thread_tids[]
I have an idea of using this variable to decide legit thread_tids[] inorder to sendSIGUSR1signal
we will be doing below changes if we go ahead with this idea.https://github.com/TejaswineeL/gramine/blob/14a8be3adcb801ed4a9a1ff65afb178448111d86/pal/src/host/linux-sgx/host_exception.c#L197
/* using g_enclave_thread_num in the for loop condition check */
spinlock\_lock(&g\_enclave\_thread\_map\_lock);
for (size\_t i = 1; i < g\_enclave\_thread\_num; i++) {
int child\_tid = ((struct enclave\_dbginfo\*)DBGINFO\_ADDR)->thread\_tids\[i\];
log\_always("child tid with g\_enclave\_thread\_num %d", child\_tid);
DO\_SYSCALL(tkill, child\_tid, SIGUSR1);
signal\_counter++;
}
spinlock\_unlock(&g\_enclave\_thread\_map\_lock);
/* removing static keyword so that we can use these variables in host_exception.c file */
spinlock\_t g\_enclave\_thread\_map\_lock = INIT\_SPINLOCK\_UNLOCKED;
size\_t g\_enclave\_thread\_num = 0;stats_PR/changes_stats_or/gramine/pal/src/host/linux-sgx/pal_tcb.h
/* adding below lines to the file included in host_exception.c*/
extern size\_t g\_enclave\_thread\_num;
extern spinlock\_t g\_enclave\_thread\_map\_lock;Approach 2:
All the children thread ids are present at location /proc/pid/task. This location only stores the main process id and children thread ids.
I can validate thread ids of the array thread_tids[] with the tids present at /proc//task during execution
Below snippet gives us the children thread ids-defining the filepath whre we can find the children thread ids
snprintf(filepath, sizeof(filepath), "/proc/%d/task/", g\_host\_pid);-Fetching the children tids from the filepath location with files
struct dirent \*entry;
dir = opendir(filename);
if (dir == NULL) {
perror("Error opening directory");
return -1;
}
while ((entry = readdir(dir)) != NULL) {
if (entry->d\_type == DT\_DIR && atoi(entry->d\_name) != g\_host\_pid) {
if (strcmp(entry->d\_name, ".") == 0 || strcmp(entry->d\_name, "..") == 0) {
// Skip current directory and parent directory
continue;
}
printf(" thread id from proc %s", entry->d\_name);
}
}
closedir(dir);Kindly, Let us know which of the above approaches would you like.
Approach1 logs
Sharing below the logs showing threads_tids[] array contents and value of g_enclave_thread_num used in approach1,
Here, threads_tids[0] always denotes the main thread tid (which we dont have to send SIGUSR1 signal to)threads_tids[1] to threads_tids[5] shows the children tids. threads_tids[6] and threads_tids[7] are empty.
Approach2
Sharing below the contents of dir /proc/pid/task showing children tids
Plan is to validate children tids got from threads_tids[] with children tids of /proc/pid/task
Kindly, Let us know which approach would you prefer.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
-- commits line 12 at r4:
We don't use fixups to fixups. Each next fixup is supposed to be a fixup to the first (main) commit. Please check other PRs from other maintainers, who they do it.
pal/src/host/linux-sgx/host_exception.c line 18 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I feel like you added a lot of unnecessary includes. Please remove the not-needed ones.
Is dirent.h needed? This header is about file system directories, but you don't use them in your patch?
pal/src/host/linux-sgx/host_exception.c line 203 at r1 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
Approach1 logs
Sharing below the logs showing threads_tids[] array contents and value of g_enclave_thread_num used in approach1,
Here, threads_tids[0] always denotes the main thread tid (which we dont have to send SIGUSR1 signal to)threads_tids[1] to threads_tids[5] shows the children tids. threads_tids[6] and threads_tids[7] are empty.
Approach2
Sharing below the contents of dir "/proc/pid/task " showing children tids
Plan is to validate children tids got from threads_tids[] with children tids of '/proc/pid/task'
Kindly, Let us know which approach would you prefer.
I'm sorry that I confused you. I now realized that TID 0 (= PID 0) is the kernel PID on the host Linux. So actually we're safe here, and you can consider tid == 0 as "no actual Gramine thread".
So your code is correct, and please excuse me that you had to go research the possibilities :(
pal/src/host/linux-sgx/host_exception.c line 240 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But this means that main thread does not reset his local stats?
Could you answer my question? I still think that the main thread doesn't reset its local stats.
pal/src/host/linux-sgx/host_exception.c line 194 at r4 (raw file):
static int send_sigusr1_signal_to_children(void) { int signal_counter= 0;
= -- add space
pal/src/host/linux-sgx/host_exception.c line 194 at r4 (raw file):
static int send_sigusr1_signal_to_children(void) { int signal_counter= 0;
That's a bad variable name. Please rename similar to how you have it in another func: no_of_children
pal/src/host/linux-sgx/host_exception.c line 194 at r4 (raw file):
static int send_sigusr1_signal_to_children(void) { int signal_counter= 0;
We don't like to use int in cases of clearly unsigned "counter" integers. So please use size_t, that's the safest option.
pal/src/host/linux-sgx/host_exception.c line 196 at r4 (raw file):
int signal_counter= 0; for (size_t i = 1; i < MAX_DBG_THREADS; i++) {
Why do you start with 1?
If you do this to NOT send the signal to yourself, then please add a func argument and use it for checking:
static int send_sigusr1_signal_to_children(int my_tid) {
...
for (size_t i = 0; i < MAX_DBG_THREADS; i++) {
int child_tid = ((struct enclave_dbginfo*)DBGINFO_ADDR)->thread_tids[i];
if (child_tid == my_tid)
continue;
}
pal/src/host/linux-sgx/host_exception.c line 198 at r4 (raw file):
for (size_t i = 1; i < MAX_DBG_THREADS; i++) { int child_tid = ((struct enclave_dbginfo*)DBGINFO_ADDR)->thread_tids[i]; if(child_tid > 0) {
No need for > 0, simply:
if (child_tid) {
pal/src/host/linux-sgx/host_exception.c line 208 at r4 (raw file):
static void dump_and_reset_stats(void) {
Please use our coding style in this PR. In particular, here { should be on the same line as the func declaration:
static void dump_and_reset_stats(void) {
pal/src/host/linux-sgx/host_exception.c line 209 at r4 (raw file):
static void dump_and_reset_stats(void) { static atomic_int no_of_children_visited = 0;
You don't need atomic_int if you use __atomic_xxx() operations. These are two different and unrelated ways of using atomics.
Here, you can just use int.
pal/src/host/linux-sgx/host_exception.c line 210 at r4 (raw file):
{ static atomic_int no_of_children_visited = 0; static const uint64_t LOOP_ATTEMPTS_MAX = 10000; /* rather arbitrary */
Why you keep this const? Just remove it and always do sched_yield() (and don't do CPU_RELAX() at all)
pal/src/host/linux-sgx/host_exception.c line 212 at r4 (raw file):
static const uint64_t LOOP_ATTEMPTS_MAX = 10000; /* rather arbitrary */ if(DO_SYSCALL(gettid) == g_host_pid) {
if ( -- please add space in between. Also, please check all other such places in your PR.
pal/src/host/linux-sgx/host_exception.c line 214 at r4 (raw file):
if(DO_SYSCALL(gettid) == g_host_pid) { int no_of_children = send_sigusr1_signal_to_children(); uint64_t loop_attempts = 0;
Useless variable.
pal/src/host/linux-sgx/host_exception.c line 216 at r4 (raw file):
uint64_t loop_attempts = 0; /* Wait here until all the children are done processing the signal. */
Remove this comment, it's obvious what the code does
pal/src/host/linux-sgx/host_exception.c line 217 at r4 (raw file):
/* Wait here until all the children are done processing the signal. */ while((no_of_children) > (__atomic_load_n(&no_of_children_visited, __ATOMIC_ACQUIRE))) {
Please swap these two expressions places, so it's more readable ("while number of visited children is still less than the total number of children")
pal/src/host/linux-sgx/host_exception.c line 229 at r4 (raw file):
__atomic_store_n(&no_of_children_visited, 0, __ATOMIC_RELEASE); } else { log_always("----- DUMPTING and RESETTING SGX STATS -----");
You sure we want to print this out? Sounds rather useless, because update_and_print_stats() will also print an information line.
pal/src/host/linux-sgx/host_exception.c line 235 at r4 (raw file):
PAL_HOST_TCB* tcb = pal_get_host_tcb(); int ret = pal_host_tcb_reset_stats(tcb);
Why do you do pal_get_host_tcb() in this function and pass it to pal_host_tcb_reset_stats()? You can move that into pal_host_tcb_reset_stats() itself, just like is done in update_nad_print_stats().
pal/src/host/linux-sgx/host_exception.c line 237 at r4 (raw file):
int ret = pal_host_tcb_reset_stats(tcb); if(ret < 0) return;
There's no need for return as this is a void function and you're at the end of this function.
Since you don't need return, you also don't need ret.
pal/src/host/linux-sgx/host_exception.c line 245 at r4 (raw file):
__UNUSED(uc); if(g_sgx_enable_stats)
if ( -- space
pal/src/host/linux-sgx/host_exception.c line 248 at r4 (raw file):
dump_and_reset_stats(); return;
There's no need in return in a void function. Remove this line.
pal/src/host/linux-sgx/host_thread.c line 107 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this needed?
I still don't see a need in this arch_prctl() syscall. Why did you add it here?
pal/src/host/linux-sgx/host_thread.c line 69 at r4 (raw file):
" # of async signals: %lu", pid, g_eenter_cnt, g_eexit_cnt, g_aex_cnt, g_sync_signal_cnt, g_async_signal_cnt);
All these g_ variables must be read via __atomic_load_n().
There was a problem hiding this comment.
Reviewable status: all files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/host/linux-sgx/host_exception.c line 18 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is
dirent.hneeded? This header is about file system directories, but you don't use them in your patch?
That's right. Its not needed. I will remove in the upcoming patch.
pal/src/host/linux-sgx/host_exception.c line 35 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But this macro is declared somewhere else, why not
#include <where-it-declared.h>?
The Macro is declared in #include "gdb_integration/sgx_gdb.h. This is already included.
pal/src/host/linux-sgx/host_exception.c line 240 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you answer my question? I still think that the main thread doesn't reset its local stats.
Main thread does reset its local stats. Unconditional code snippet shown below does that.
https://github.com/gramineproject/gramine/pull/1857/files#diff-36ddf4f0901a94224ef8f08faa82ad332c9afc3ed952d5950e5597f110464ec7R235
Here's the logs of the main thread stats dump and reset
pal/src/host/linux-sgx/host_exception.c line 196 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you start with 1?
If you do this to NOT send the signal to yourself, then please add a func argument and use it for checking:
static int send_sigusr1_signal_to_children(int my_tid) { ... for (size_t i = 0; i < MAX_DBG_THREADS; i++) { int child_tid = ((struct enclave_dbginfo*)DBGINFO_ADDR)->thread_tids[i]; if (child_tid == my_tid) continue; }
Yes, thread_tids[0] is the main thread id. It is the same tid to which we send kill -SIGUSR1 signal via command line.
I will add the check you suggested I will use var name as main_tid instead of my_tid.
pal/src/host/linux-sgx/host_exception.c line 229 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You sure we want to print this out? Sounds rather useless, because
update_and_print_stats()will also print an information line.
The intention is to differentiate usual display of stats (as a result of sgx.enable_stats=true) from 'dump and reset' of stats (with kill -SIGUSR1). Also, it will acknowledge the receival of kill -SIGUSR1 signal
pal/src/host/linux-sgx/host_thread.c line 107 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I still don't see a need in this
arch_prctl()syscall. Why did you add it here?
I see that without adding arch_prctl(), the stats are getting reset successfully.
Removing arch_prctl() in the upcoming patch.
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
a discussion (no related file):
Please update the Performance tuning and analysis page in the documentation accordingly.
You can take inspiration from this patch: https://github.com/gramineproject/gramine/pull/1751/files
pal/src/host/linux-sgx/host_exception.c line 240 at r1 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
Main thread does reset its local stats. Unconditional code snippet shown below does that.
https://github.com/gramineproject/gramine/pull/1857/files#diff-36ddf4f0901a94224ef8f08faa82ad332c9afc3ed952d5950e5597f110464ec7R235Here's the logs of the main thread stats dump and reset
Ok, you're right, pal_host_tcb_reset_stats() is called unconditionally
pal/src/host/linux-sgx/host_exception.c line 229 at r4 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
The intention is to differentiate usual display of stats (as a result of 'sgx.enable_stats=true') from 'dump and reset' of stats (with 'kill -SIGUSR1'). Also, it will acknowledge the receival of 'kill -SIGUSR1' signal
Ok. Don't we want the same message on the main thread?
pal/src/host/linux-sgx/host_exception.c line 248 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There's no need in
returnin a void function. Remove this line.
You forgot about this comment.
pal/src/host/linux-sgx/host_exception.c line 192 at r5 (raw file):
static size_t send_sigusr1_signal_to_children(pid_t main_tid) { size_t no_of_signal_sent = 0;
no_of_signal_sent -> no_of_signals_sent
pal/src/host/linux-sgx/host_exception.c line 213 at r5 (raw file):
size_t no_of_children = send_sigusr1_signal_to_children(g_host_pid); while ((__atomic_load_n(&no_of_children_visited, __ATOMIC_ACQUIRE)) < (no_of_children)) {
There's no reason to have no_of_children in parentheses, please remove them
pal/src/host/linux-sgx/host_exception.c line 219 at r5 (raw file):
__atomic_store_n(&no_of_children_visited, 0, __ATOMIC_RELEASE); } else { log_always("----- DUMPTING and RESETTING SGX STATS -----");
DUMPING (you have a typo)
pal/src/host/linux-sgx/host_thread.c line 70 at r5 (raw file):
pid, __atomic_load_n(&g_eenter_cnt, __ATOMIC_ACQUIRE), __atomic_load_n(&g_eexit_cnt, __ATOMIC_ACQUIRE), __atomic_load_n(&g_aex_cnt, __ATOMIC_ACQUIRE), __atomic_load_n(&g_sync_signal_cnt, __ATOMIC_ACQUIRE), __atomic_load_n(&g_async_signal_cnt, __ATOMIC_ACQUIRE));
Please now re-wrap these lines to 100-chars-per-line limit.
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
TejaswineeL
force-pushed
the
debug_stats_dump_reset
branch
from
5f1f573
to
c876141
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/host/linux-sgx/host_exception.c line 229 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok. Don't we want the same message on the main thread?
Yes we want. Adding it.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
Documentation/performance.rst line 106 at r6 (raw file):
trends. It is also possible to reset the performance statistics interactively, using
to reset the performance statistics -> to dump and reset SGX-related statistics
Documentation/performance.rst line 108 at r6 (raw file):
It is also possible to reset the performance statistics interactively, using ``SIGUSR1`` signal. This helps to collect performance statistics only for a particular period e.g., skipping the Gramine startup and application
Please add a comma before e.g.
Documentation/performance.rst line 111 at r6 (raw file):
initialization time and concentrating only on the actual application processing. Send ``SIGUSR1`` using command ``kill -SIGUSR1 <gramine-pid>`` (note the minus sign before <gramine-pid>). Sending multiple ``SIGUSR1`` will result
Please remove the (note the minus sign ... ) sentence, it is not relevant here.
Documentation/performance.rst line 113 at r6 (raw file):
minus sign before <gramine-pid>). Sending multiple ``SIGUSR1`` will result in a sequential dump and reset of the statistics, each dump and reset of statistics will be done after the previous ``SIGUSR1``.
The whole Sending multiple ... sentence seems redundant, as it is clear that "dump and reset" means that the statistics will be reset, and one can send a next SIGUSR1.
Please just remove the whole sentence.
pal/src/host/linux-sgx/host_exception.c line 215 at r6 (raw file):
while ((__atomic_load_n(&no_of_children_visited, __ATOMIC_ACQUIRE)) < no_of_children) { DO_SYSCALL(sched_yield); }
Please add an empty line after this line
pal/src/host/linux-sgx/host_exception.c line 221 at r6 (raw file):
} else { log_always("----- DUMPING and RESETTING SGX STATS -----"); update_and_print_stats(/*process_wide=*/false);
Actually, when you have many child threads, won't their outputs be mixed? Looks like we need some locking here...
pal/src/host/linux-sgx/host_exception.c line 287 at r6 (raw file):
ret = set_signal_handler(SIGUSR1, handle_sigusr1); if (ret < 0) goto err;
Actually, can you move this set of signal handler to right-after set_signal_handler(SIGCONT)? There it would make more sense (the order of signals will be more readable).
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
There was a problem hiding this comment.
Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/host/linux-sgx/host_exception.c line 221 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, when you have many child threads, won't their outputs be mixed? Looks like we need some locking here...
Yes, that's right, their outputs get mixed. I added a change, where "DUMPING and RESETTING SGX STATS" log is displayed only once. The log is displayed once per SIGUSR1 signal, at the start of the dumping and resetting operation.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
-- commits line 27 at r7:
Please don't perform git merge (and don't allow atomatic git merge). That's for the future.
pal/src/host/linux-sgx/host_exception.c line 221 at r6 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
Yes, that's right, their outputs get mixed. I added a change, where "DUMPING and RESETTING SGX STATS" log is displayed only once. The log is displayed once per
SIGUSRsignal, at the start of the dumping and resetting operation.
Ok, this looks reasonable to me.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
a discussion (no related file):
I followed the steps in the PR description and successfully tested the Redis example. For me, the PR is ready. Thanks for this effort @TejaswineeL !
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please don't perform git merge (and don't allow atomatic git merge). That's for the future.
Accidentally put a blocking comment. Unblocking now.
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
pal/src/host/linux-sgx/host_thread.c line 35 at r7 (raw file):
static atomic_ulong g_aex_cnt = 0; static atomic_ulong g_sync_signal_cnt = 0; static atomic_ulong g_async_signal_cnt = 0;
I didn't notice this before, but all these variables use atomic_ (C99 atomics syntax). We decided to not use it in new code, so please modify all these to be simple uint64_t:
static uint64_t g_eenter_cnt = 0;
...
static uint64_t g_async_signal_cnt = 0;
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @TejaswineeL)
pal/src/host/linux-sgx/host_exception.c line 235 at r11 (raw file):
Should we even consider this case?
Yes. We don't want to have any race conditions, especially ones which may lead to memory corruptions (I'm not sure if this one can, didn't analyze it that closely).
It's the user who sends SIGUSR1, so it becomes user responsibility not to spam the application with these signals. (And it's easy for the user to learn whether the last handling finished -- if the user sees
Total SGX stats for process, then the user knows it is safe to send another SIGUSR1)
a) Someone can automate it and send it from a script.
b) Something may randomly lag (e.g. because of swapping) and the user may send another signal thinking that the first one somehow didn't arrive (because they didn't see any output yet).
I had checked this possibility with existing changes, gramine does not allow spamming of signals.
a signal is only received when it has already processed the previous signal. I did not have to add any changes for this, it was already taken care of.
Why do you think so? There's no such mechanism here, you just install the signal handler here and that's all (in this very PR).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
pal/src/host/linux-sgx/host_exception.c line 235 at r11 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Should we even consider this case?
Yes. We don't want to have any race conditions, especially ones which may lead to memory corruptions (I'm not sure if this one can, didn't analyze it that closely).
It's the user who sends SIGUSR1, so it becomes user responsibility not to spam the application with these signals. (And it's easy for the user to learn whether the last handling finished -- if the user sees
Total SGX stats for process, then the user knows it is safe to send another SIGUSR1)a) Someone can automate it and send it from a script.
b) Something may randomly lag (e.g. because of swapping) and the user may send another signal thinking that the first one somehow didn't arrive (because they didn't see any output yet).I had checked this possibility with existing changes, gramine does not allow spamming of signals.
a signal is only received when it has already processed the previous signal. I did not have to add any changes for this, it was already taken care of.Why do you think so? There's no such mechanism here, you just install the signal handler here and that's all (in this very PR).
So it was easier to write my own code: #1944
Unfortunately, there's this DEBUG vs no-DEBUG build issue. I'm not sure (1) why assembly breaks in non-DEBUG build, and (2) how we want to compile profiling support and this SGX-stats-reset support -- in DEBUG mode only, or in release mode too.
Signed-off-by: sreeharikax <sreex.harika.kalakatta@intel.com>
Signed-off-by: sreeharikax <sreex.harika.kalakatta@intel.com>
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
pal/src/host/linux-sgx/host_exception.c line 235 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So it was easier to write my own code: #1944
Unfortunately, there's this DEBUG vs no-DEBUG build issue. I'm not sure (1) why assembly breaks in non-DEBUG build, and (2) how we want to compile profiling support and this SGX-stats-reset support -- in DEBUG mode only, or in release mode too.
Thank You @dimakuv for resolving the above issue.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r11, 2 of 2 files at r12, 4 of 4 files at r13, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
-- commits line 4 at r13:
It should be added here that the PR was co-developed by Dmitrii Kuvaiskii (me). I will add the Co-developed and Signed-off-by lines during final merge.
-- commits line 62 at r13:
Who is this person? Do we need to add this person to the list of Co-developed-by and Signed-off-by in the first commit message?
pal/src/host/linux-sgx/enclave_ocalls.c line 139 at r13 (raw file):
} long result = COPY_UNTRUSTED_VALUE(&req->result);
This is taken from #1955
This PR will need to be rebased when that one is merged. Blocking until then.
pal/src/host/linux-sgx/host_exception.c line 191 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@mkow This is needed because the logic is different from #1751.
In PR #1751, all threads were dumping their info to a shared
perf.datafile on each AEX event. SeeSo to "reset" the perf.data, all that was required to be done was to close the old perf-data file and open a new perf-data file. This is enough to be done in one thread (under a lock), and all other threads would automatically pick up the new perf-data file.
In this PR, however, there are no events on which all threads "do something". So we need to generate this event ourselves, which is the SIGUSR1 signal.
Unfortunately, outside programs like
killdo not have the ability to send some signal to all threads of the same process. (Whatkillcan do is to send the signal to all processes in the same process group, but this chooses one thread from each process.)So that's why we have this additional for loop -- we must trigger some event so that each thread updates and dumps its own local information (about the statistics on EENTER, EEXIT, etc). Also, this for loop guarantees "atomicity" -- the main thread waits for other threads to update their statistics and then prints the process-wide statistics.
It will only be received by the main thread.
That's not true, Linux doesn't guarantee that - only that some thread will receive it.
@mkow This is irrelevant for this PR, whether Linux sends the signal to the main thread or to some thread (indeed, Linux sends SIGUSR1 to some thread, not necessarily to the main thread).
But if you look at the implementation of this PR, the logic is basically "whichever thread received SIGUSR1, this thread is designated as main and broadcasts this signal to all other threads in this process". So if you want to, we can rename the
main_tidvariable to something likethis_tid. But I think the logic is generally sound.UPDATE: Ok, I see that
dump_and_reset_stats()usesg_host_pidwhich is the main thread. I outlined the fix for this problem (and for the other problem of doing complex things in the signal handler), see my other comment.
This PR now follows the approach that I outlined in #1944. Resolving this comment.
pal/src/host/linux-sgx/host_exception.c line 235 at r11 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
Thank You @dimakuv for resolving the above issue.
You're welcome.
FYI: I verified that this PR and #1944 have the same implementation.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
pal/src/host/linux-sgx/host_exception.c line 235 at r11 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You're welcome.
FYI: I verified that this PR and #1944 have the same implementation.
What has happened here? Why do we have two PRs with the same contents?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
pal/src/host/linux-sgx/host_exception.c line 235 at r11 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What has happened here? Why do we have two PRs with the same contents?
Done, I closed my "dontmerge" PR. (Forgot to close it before.) That PR was my proof of concept explanation of the new approach.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r12, 4 of 4 files at r13, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
pal/src/host/linux-sgx/host_exception.c line 47 at r13 (raw file):
are dumped and reset on the next AEX.
The current implementation dumps them also on the next normal enclave entry, which means that they must be dumped right now (because in all cases we'll be returning to the enclave after the return from the SIGUSR1 handler).
pal/src/host/linux-sgx/host_exception.c line 52 at r13 (raw file):
On each AEX
Also on normal enclave entry?
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
if (__atomic_compare_exchange_n(&g_stats_reset_leader_tid, &expected_tid, DO_SYSCALL(gettid), /*weak=*/false, __ATOMIC_ACQ_REL, __ATOMIC_RELAXED) == true) {
What if the current thread was interrupted in sgx_ocall_exit() and was chosen to be the leader? This whole feature breaks then?
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
* fortunately this variable is set up even in non-debug builds */ for (size_t i = 0; i < MAX_DBG_THREADS; i++) { int follower_tid = ((struct enclave_dbginfo*)DBGINFO_ADDR)->thread_tids[i];
This is racy, enclave_dbginfo may be modified concurrently while we're here.
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
* fortunately this variable is set up even in non-debug builds */ for (size_t i = 0; i < MAX_DBG_THREADS; i++) { int follower_tid = ((struct enclave_dbginfo*)DBGINFO_ADDR)->thread_tids[i];
enclave_dbginfo is intended only for GDB integration, we cannot use it in non-GDB code. This region isn't even guaranteed to exist:
gramine/pal/src/host/linux-sgx/host_main.c
Lines 605 to 606 in 462168c
pal/src/host/linux-sgx/host_exception.c line 338 at r13 (raw file):
continue; DO_SYSCALL(tkill, follower_tid, SIGUSR1);
This may hit a thread in another process. Use tgkill instead.
pal/src/host/linux-sgx/host_exception.c line 345 at r13 (raw file):
/* called on each AEX and OCALL (in normal context), see host_entry.S */ void dump_and_reset_stats(void) {
This name is misleading, this function most of the time doesn't dump or reset stats.
Code quote:
dump_and_reset_statspal/src/host/linux-sgx/host_exception.c line 363 at r13 (raw file):
size_t followers_num = send_sigusr1_to_followers(leader_tid); while ((__atomic_load_n(&followers_visited_num, __ATOMIC_ACQUIRE)) < followers_num)
What if one of the threads exited in the meantime? (e.g. if it was interrupted in sgx_ocall_exit()) Won't we inf-loop here?
pal/src/host/linux-sgx/host_exception.c line 367 at r13 (raw file):
update_and_print_stats(/*process_wide=*/true); pal_host_tcb_reset_stats();
update_and_print_stats(/*process_wide=*/true); above already does that (except setting tcb->reset_stats to false)
pal/src/host/linux-sgx/host_thread.c line 31 at r13 (raw file):
/* this function is called only on thread/process exit (never in the middle of thread exec) */ void update_and_print_stats(bool process_wide) { static atomic_ulong g_eenter_cnt = 0;
Why this change?
Suggestion:
static uint64_tpal/src/host/linux-sgx/host_thread.c line 78 at r13 (raw file):
__atomic_store_n(&g_aex_cnt, 0, __ATOMIC_RELEASE); __atomic_store_n(&g_sync_signal_cnt, 0, __ATOMIC_RELEASE); __atomic_store_n(&g_async_signal_cnt, 0, __ATOMIC_RELEASE);
Why did you add this zeroing here?
pal/src/host/linux-sgx/pal_tcb.h line 108 at r13 (raw file):
int32_t last_async_event; /* last async signal, reported to the enclave on ocall return */ int* start_status_ptr; /* pointer to return value of clone_thread */ bool reset_stats; /* if true, dump SGX stats and reset them on next AEX event */
Or enclave entry.
Code quote:
them on next AEX event *
There was a problem hiding this comment.
Reviewable status: all files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @TejaswineeL)
a discussion (no related file):
This code is still quite complicated (as you can see by the number of race conditions found). Do we really want this feature? It's just some performance logging which seems to be non-trivial to implement correctly. I'm especially uneasy to include this in release builds, but I'm not even sure this feature is worth the effort.
pal/src/host/linux-sgx/host_entry.S line 45 at r13 (raw file):
movq $EENTER, %rax enclu
What if a new thread is entering the enclave for the first time here and gets SIGUSR1? It's already on the map so the leader will be waiting for it, but this thread will enter the enclave and may exit via some blocking ocall, which won't be interrupted by SIGUSR1 and will deadlock.
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
pushq %rbp .cfi_adjust_cfa_offset 8 movq %rsp, %rbp
Was this a bug? That rbp wasn't restored?
pal/src/host/linux-sgx/host_exception.c line 228 at r13 (raw file):
Moving this out into a separate discussion:
(2) how we want to compile profiling support and this SGX-stats-reset support -- in DEBUG mode only, or in release mode too.
So, why isn't this a debug-only feature? We wouldn't need to execute all this complicated logic in release builds.
pal/src/host/linux-sgx/host_exception.c line 333 at r13 (raw file):
/* we re-use DBGINFO_ADDR special variable (that is primarily used by GDB for debugging), * fortunately this variable is set up even in non-debug builds */ for (size_t i = 0; i < MAX_DBG_THREADS; i++) {
This, but it's racy, this variable is protected by g_enclave_thread_map_lock.
Suggestion:
for (size_t i = 0; i < g_enclave_thread_num; i++) {pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is racy,
enclave_dbginfomay be modified concurrently while we're here.
Why not using enclave_thread_map instead?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
pal/src/host/linux-sgx/host_entry.S line 45 at r13 (raw file):
I do not understand the exact scenario here, let's first understand the specifics.
What if a new thread is entering the enclave for the first time here and gets
SIGUSR1?
Do you mean it's the user-triggered SIGUSR1, or the leader-thread-triggered SIGUSR1? From your following description, you seem to imply the leader-thread-triggered SIGUSR1, so I'll assume this.
may exit via some blocking ocall, which won't be interrupted by
SIGUSR1
Why would the thread be interrupted again by SIGUSR1? The thread already got SIGUSR1 and was interrupted, as you stated in your first sentence. So the thread got SIGUSR1 at that time and memorized this as tcb->reset_stats = true.
Now, on the very next AEX or OCALL, this thread will call dump_and_reset_stats() and will actually dump the statistics. Since you say exit via some blocking ocall, this is covered by newly added code under .Lsgx_entry -- the dump_and_reset_stats() function will be called before performing the OCALL.
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Was this a bug? That
rbpwasn't restored?
Yes and no. The RBP register is "stable" in debug mode (i.e., when GCC builds without optimizations), so the caller func saves it in its prologue and then restores it back. In release mode (GCC with optimizations), the base frame pointer can be omitted and RBP is used as any other general purpose register.
So now that we removed this code from under ifdef DEBUG, we have to do this. Previously, we could skip this.
pal/src/host/linux-sgx/host_exception.c line 47 at r13 (raw file):
The current implementation dumps them also on the next normal enclave entry
No, that's incorrect. The current implementation dumps the stats also right-before the next OCALL. See .Lsgx_entry in the asm code, and be aware that the name "sgx_entry" is totally misleading -- it means "the entry from the SGX enclave to the untrusted OCALL execution" (legacy weird name).
But it's true that I forgot to mention this "right-before executing the next OCALL in untrusted runtime" part in all this text.
pal/src/host/linux-sgx/host_exception.c line 52 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
On each AEX
Also on normal enclave entry?
ditto (on enclave exit, right-before executing the OCALL)
pal/src/host/linux-sgx/host_exception.c line 228 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Moving this out into a separate discussion:
(2) how we want to compile profiling support and this SGX-stats-reset support -- in DEBUG mode only, or in release mode too.
So, why isn't this a debug-only feature? We wouldn't need to execute all this complicated logic in release builds.
We do this also in release mode because sgx.enable_stats always worked in release mode too. There's just nothing that requires it to be in debug mode (in contrast to e.g. profiling support, which requires debug symbols).
So this whole "let's allow this also in release mode" approach is purely for backwards-compatibility.
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What if the current thread was interrupted in
sgx_ocall_exit()and was chosen to be the leader? This whole feature breaks then?
This looks rather benign -- the SIGUSR1 event will then be just "lost" -- the current thread will exit without sending SIGUSR1 to other threads. So the SIGUSR1 event from the user will be just lost -- not amazing, but I guess we should live with this possibility.
pal/src/host/linux-sgx/host_exception.c line 333 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This, but it's racy, this variable is protected by
g_enclave_thread_map_lock.
Yep, I think this is better. We can do this.
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why not using
enclave_thread_mapinstead?
Yes, we can do this.
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
enclave_dbginfois intended only for GDB integration, we cannot use it in non-GDB code. This region isn't even guaranteed to exist:gramine/pal/src/host/linux-sgx/host_main.c
Lines 605 to 606 in 462168c
Good point, we can move to g_enclave_thread_map
pal/src/host/linux-sgx/host_exception.c line 338 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This may hit a thread in another process. Use
tgkillinstead.
I actually tried to use tgkill (with smth like tgkill(getpid(), follower_tid)), but it didn't work... We can check again what happened there.
pal/src/host/linux-sgx/host_exception.c line 345 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This name is misleading, this function most of the time doesn't dump or reset stats.
Yes, can be modified to smth like maybe_dump_and_reset_stats()
pal/src/host/linux-sgx/host_exception.c line 363 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What if one of the threads exited in the meantime? (e.g. if it was interrupted in
sgx_ocall_exit()) Won't we inf-loop here?
Yes, we will. I'm not sure how we want to handle this special case.
I see two options:
- Periodically re-check the current number of threads (read
g_enclave_thread_numunder lock) and compare withfollowers_num, if it's different, then abort the loop and print a warning. But this is not robust -- in the meantime one thread could have died and another one could have been spawned, so the total number of threads stays the same but one SIGUSR1 is lost, and the while loop is still inifinite. - Introduce a time out of several seconds and abort the loop with a warning after timeout expires. I prefer this solution.
pal/src/host/linux-sgx/host_exception.c line 367 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
update_and_print_stats(/*process_wide=*/true);above already does that (except settingtcb->reset_statstofalse)
No, it doesn't.
update_and_print_stats()only updates global stats variables.pal_host_tcb_reset_stats()only updates thread-local stats variables.
So we need to call both functions.
pal/src/host/linux-sgx/host_thread.c line 31 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why this change?
Because at some point we agreed to move all C99 atomics to GCC atomics (or well, maybe not agreed but that's what we've been doing since long)? But this is unrelated change, and if you want to, it can be reverted.
pal/src/host/linux-sgx/host_thread.c line 78 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why did you add this zeroing here?
We want to reset all statistics: both thread-local and global. This particular code snippet resets global statistics.
pal/src/host/linux-sgx/pal_tcb.h line 108 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Or enclave entry.
Actually: ...or enclave exit for an OCALL
There was a problem hiding this comment.
Reviewable status: all files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
This code is still quite complicated (as you can see by the number of race conditions found). Do we really want this feature? It's just some performance logging which seems to be non-trivial to implement correctly. I'm especially uneasy to include this in release builds, but I'm not even sure this feature is worth the effort.
Yes, it is 'once in for all' effort.
For every new workload's performance tuning, this would be that starting point of performance analysis.
Without this we would not be able to exclude the enclave loading/exit noise etc.
This will help pin-point the area of performance optimization.
Will it be still complicated if we enable this feature only in debug mode?
@dimakuv what do you think about @mkow 's above comment?
pal/src/host/linux-sgx/host_exception.c line 47 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The current implementation dumps them also on the next normal enclave entry
No, that's incorrect. The current implementation dumps the stats also right-before the next OCALL. See
.Lsgx_entryin the asm code, and be aware that the name "sgx_entry" is totally misleading -- it means "the entry from the SGX enclave to the untrusted OCALL execution" (legacy weird name).But it's true that I forgot to mention this "right-before executing the next OCALL in untrusted runtime" part in all this text.
Can I reframe the statement as below
so that this thread's statistics are dumped and reset right-before executing the next OCALL in untrusted runtime .
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, we can do this.
We had started with enclave_thread_map itself. But that approach was not useful. The point when, the SIGUSR1 signal is received, enclave_thread_map does not have all the threads. enclave_thread_map fails to give us all the follower_tids (the experiment was done on MySql workload i could get only 2 thread_tid in enclave_thread_map out of the total 40 threads)
To avoid the race condition we can add spinlock on DBGINFO_ADDR
please let me know your opinion on this.
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Good point, we can move to
g_enclave_thread_map
We had started with enclave_thread_map itself. But that approach was not useful. The point when, the SIGUSR1 signal is received, enclave_thread_map does not have all the threads. enclave_thread_map fails to give us all the follower_tids(the experiment was done on MySql workload i could get only 2 thread_tid in enclave_thread_map out of the total 40 threads)
To avoid the race condition we can add spinlock on DBGINFO_ADDR
please let me know your opinion on this.
pal/src/host/linux-sgx/host_exception.c line 338 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I actually tried to use
tgkill(with smth liketgkill(getpid(), follower_tid)), but it didn't work... We can check again what happened there.
I will try with tgkill.
pal/src/host/linux-sgx/host_exception.c line 363 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, we will. I'm not sure how we want to handle this special case.
I see two options:
- Periodically re-check the current number of threads (read
g_enclave_thread_numunder lock) and compare withfollowers_num, if it's different, then abort the loop and print a warning. But this is not robust -- in the meantime one thread could have died and another one could have been spawned, so the total number of threads stays the same but one SIGUSR1 is lost, and the while loop is still inifinite.- Introduce a time out of several seconds and abort the loop with a warning after timeout expires. I prefer this solution.
yes, will add 2nd approach.
pal/src/host/linux-sgx/pal_tcb.h line 108 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually: ...or enclave exit for an OCALL
sure. Can I reframe it as below
dump SGX stats and reset them on enclave exit for an OCALL
There was a problem hiding this comment.
Reviewable status: all files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @TejaswineeL)
a discussion (no related file):
once in for all
What do you mean by that?
pal/src/host/linux-sgx/host_entry.S line 45 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I do not understand the exact scenario here, let's first understand the specifics.
What if a new thread is entering the enclave for the first time here and gets
SIGUSR1?Do you mean it's the user-triggered SIGUSR1, or the leader-thread-triggered SIGUSR1? From your following description, you seem to imply the leader-thread-triggered SIGUSR1, so I'll assume this.
may exit via some blocking ocall, which won't be interrupted by
SIGUSR1Why would the thread be interrupted again by SIGUSR1? The thread already got SIGUSR1 and was interrupted, as you stated in your first sentence. So the thread got SIGUSR1 at that time and memorized this as
tcb->reset_stats = true.Now, on the very next AEX or OCALL, this thread will call
dump_and_reset_stats()and will actually dump the statistics. Since you sayexit via some blocking ocall, this is covered by newly added code under.Lsgx_entry-- thedump_and_reset_stats()function will be called before performing the OCALL.
Ah, lol, I didn't expect "sgx_entry" to actually be "sgx exit" 😑 See the comment added there.
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
i.e., when GCC builds without optimizations), so the caller func saves it in its prologue
That's not guaranteed anywhere.
Previously, we could skip this.
I don't see why would enabling optimization change anything in terms of correctness of this code. SYS-V ABI and how SGX works is the same regardless of GCC optimization level.
Also, what's in that rbp? Why do we need to save it?
pal/src/host/linux-sgx/host_entry.S line 136 at r13 (raw file):
jmp .Ldo_ecall_callee_save .Lsgx_entry:
Unrelated to the PR, but why is this called "sgx_entry"? This is literally enclave exit handler.
Code quote:
.Lsgx_entry:
pal/src/host/linux-sgx/host_exception.c line 52 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (on enclave exit, right-before executing the OCALL)
Yup, please fix the comment then :)
pal/src/host/linux-sgx/host_exception.c line 228 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We do this also in release mode because
sgx.enable_statsalways worked in release mode too. There's just nothing that requires it to be in debug mode (in contrast to e.g. profiling support, which requires debug symbols).So this whole "let's allow this also in release mode" approach is purely for backwards-compatibility.
I'd break it then, I'd prefer to not have this whole logic in production builds.
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This looks rather benign -- the SIGUSR1 event will then be just "lost" -- the current thread will exit without sending SIGUSR1 to other threads. So the SIGUSR1 event from the user will be just lost -- not amazing, but I guess we should live with this possibility.
But then we're left with g_stats_reset_leader_tid permanently set to the TID of that dead thread, completely breaking this feature until the user restarts Gramine / this process?
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
We had started with
enclave_thread_mapitself. But that approach was not useful. The point when, theSIGUSR1signal is received,enclave_thread_mapdoes not have all the threads.enclave_thread_mapfails to give us all thefollower_tids(the experiment was done on MySql workload i could get only 2 thread_tid inenclave_thread_mapout of the total 40 threads)To avoid the race condition we can add spinlock on
DBGINFO_ADDR
please let me know your opinion on this.
@TejaswineeL: Please don't copy-paste long comments between discussions.
pal/src/host/linux-sgx/host_exception.c line 345 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, can be modified to smth like
maybe_dump_and_reset_stats()
SGTM
pal/src/host/linux-sgx/host_exception.c line 363 at r13 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
yes, will add 2nd approach.
No, please don't fix deadlocks by timeouts... It's essentially like fixing race conditions by adding sleep() to a few places.
Please figure out a way of implementing this which works correctly.
pal/src/host/linux-sgx/host_exception.c line 367 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No, it doesn't.
update_and_print_stats()only updates global stats variables.pal_host_tcb_reset_stats()only updates thread-local stats variables.So we need to call both functions.
That's quite a weird semantics. Why both are reset in very different ways?
pal/src/host/linux-sgx/host_thread.c line 31 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Because at some point we agreed to move all C99 atomics to GCC atomics (or well, maybe not agreed but that's what we've been doing since long)? But this is unrelated change, and if you want to, it can be reverted.
I think we didn't, it's just that Borys liked GCC atomics more and wrote his code using them.
I'd keep it unchanged, especially that it makes the code noticeably simpler.
pal/src/host/linux-sgx/host_thread.c line 78 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We want to reset all statistics: both thread-local and global. This particular code snippet resets global statistics.
But now we have a function named "update_and_print_stats", which, when called with process_wide=true:
- prints both local and global stats
- resets global stats
- keeps local stats unchanged
That's quite inconsistent.
pal/src/host/linux-sgx/pal_tcb.h line 108 at r13 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
sure. Can I reframe it as below
dump SGX stats and reset them on enclave exit for an OCALL
on -> after an
There was a problem hiding this comment.
Reviewable status: all files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
once in for all
What do you mean by that?
I think this feature is quite useful, as knowing the approximate number of EENTER/EEXIT/AEX events is useful to pinpoint a potential bottleneck. And for many workloads, the noise of these events at startup time (when Gramine and SGX enclave are only starting and warming up) is significant, so it's quite informative to "flush" the startup-time events first, then start the actual workload (like start the clients against the Gramine app), and then dump the events/statistics again.
So to me, the feature is quite useful, and I've seen this PR (more like the code behind it) being useful for profiling some (proprietary) applications.
If @mkow insists, we could move this feature behind the DEBUG build of Gramine. Though please pay notice that this will break backwards compatibility, as SGX-stats feature is currently working also in RELEASE builds. UPDATE: Now I see that @mkow insists on this (see other comment), so let's move this SGX-stats feature under DEBUG builds (it is still fine because it will also work under DEBUGOPTIMIZED builds, so no issues with fidelity).
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
That's not guaranteed anywhere.
Yes, looks like you're correct and it's just how these particular versions of GCC work. So yes, we can call it a bug in the code.
Also, what's in that rbp? Why do we need to save it?
We're in the AEX event's "landing pad". According to Intel SDM, Chapter 37.3.1 "Processor Synthetic State on Asynchronous Enclave Exit", RBP is restored from SSA.uRBP. So it's important to save it, as SSA.uRBP may be restored on EEXIT and thus should not be modified in the AEX code.
pal/src/host/linux-sgx/host_entry.S line 136 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Unrelated to the PR, but why is this called "sgx_entry"? This is literally enclave exit handler.
Legacy historic name. I think it comes from that weird convention that sgx_ stood for "anything to do with untrusted runtime", and enclave_ stood for "anything to do with trusted enclave". So I would read sgx_entry as "entry to the untrusted runtime".
But now of course this name is totally bogus. Noone bothered to rename it yet.
pal/src/host/linux-sgx/host_exception.c line 47 at r13 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
Can I reframe the statement as below
so that this thread's statistics are dumped and reset right-before executing the next OCALL in untrusted runtime .
Well, it's both on AEX and on OCALL, so the text should be:
* this thread's statistics are dumped and reset on the next AEX or right-before
* executing the next OCALL in untrusted runtime.
pal/src/host/linux-sgx/host_exception.c line 228 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd break it then, I'd prefer to not have this whole logic in production builds.
Ok, sure, we can do it. I also don't have any objections to allow this only in DEBUG builds. Let me put a blocking comment.
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But then we're left with
g_stats_reset_leader_tidpermanently set to the TID of that dead thread, completely breaking this feature until the user restarts Gramine / this process?
Good point. We can fix this by checking if g_stats_reset_leader_tid == current_tid right-before exiting the thread, and if true, then resetting this value to zero and printing a warning. Would that work for you?
UPDATE: There's a more generic thread-exiting approach described in my other comment, please check.
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
To avoid the race condition we can add spinlock on
DBGINFO_ADDRplease let me know your opinion on this.
No, I think @mkow is right and should not use DBGINFO_ADDR variable at all.
The point when, the
SIGUSR1signal is received,enclave_thread_mapdoes not have all the threads.
This is surprising. Please double-check why that happened -- does it happen even if you're under the g_enclave_thread_map_lock lock?
pal/src/host/linux-sgx/host_exception.c line 363 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
No, please don't fix deadlocks by timeouts... It's essentially like fixing race conditions by adding sleep() to a few places.
Please figure out a way of implementing this which works correctly.
Ok, the better approach would be:
- We introduce a new variable
g_stats_reset_epoch, and increment it on a successful wait-for-followers by the leader (in this code snippet here). - On any thread exit, we atomically increment
g_stats_reset_epoch(and also resetg_stats_reset_leader_tidif it was the TID of this exiting thread). - In this code snippet here, we check if the epoch is still the same as it was when we started the while loop. If it changed, then we know some change in the thread-number state happened, so we break the loop with a warning.
@mkow What about this?
pal/src/host/linux-sgx/host_exception.c line 367 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
That's quite a weird semantics. Why both are reset in very different ways?
Ok, agreed, there's no need in a separate pal_host_tcb_reset_stats() function. Everything can be done in update_and_print_stats(), true. And that func's name should be modified too.
pal/src/host/linux-sgx/host_thread.c line 29 at r13 (raw file):
bool g_sgx_enable_stats = false; /* this function is called only on thread/process exit (never in the middle of thread exec) */
I just noticed that this comment is wrong now, I'll just remove it
pal/src/host/linux-sgx/host_thread.c line 31 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think we didn't, it's just that Borys liked GCC atomics more and wrote his code using them.
I'd keep it unchanged, especially that it makes the code noticeably simpler.
Ok, then we can revert this particular change.
FWIW, I also prefer GCC atomics.
pal/src/host/linux-sgx/host_thread.c line 78 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But now we have a function named "update_and_print_stats", which, when called with
process_wide=true:
- prints both local and global stats
- resets global stats
- keeps local stats unchanged
That's quite inconsistent.
Ok, agreed, there's no need in a separate pal_host_tcb_reset_stats() function. Everything can be done in update_and_print_stats(), true. And that func's name should be modified too.
pal/src/host/linux-sgx/pal_tcb.h line 108 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
on->after an
Actually:
/* if true, dump SGX stats and reset them on next AEX event or after next enclave exit for an OCALL */
There was a problem hiding this comment.
Reviewable status: all files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @TejaswineeL)
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
That's not guaranteed anywhere.
Yes, looks like you're correct and it's just how these particular versions of GCC work. So yes, we can call it a bug in the code.
Also, what's in that rbp? Why do we need to save it?
We're in the AEX event's "landing pad". According to Intel SDM, Chapter 37.3.1 "Processor Synthetic State on Asynchronous Enclave Exit", RBP is restored from
SSA.uRBP. So it's important to save it, asSSA.uRBPmay be restored on EEXIT and thus should not be modified in the AEX code.
But we restore it manually after EEXIT?
pal/src/host/linux-sgx/host_entry.S line 136 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Legacy historic name. I think it comes from that weird convention that
sgx_stood for "anything to do with untrusted runtime", andenclave_stood for "anything to do with trusted enclave". So I would readsgx_entryas "entry to the untrusted runtime".But now of course this name is totally bogus. Noone bothered to rename it yet.
Could you fix this? In a separate PR, ideally.
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
We can fix this by checking if
g_stats_reset_leader_tid == current_tidright-before exiting the thread
This would still be racy, what if the signal arrives after the check?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But we restore it manually after EEXIT?
Yep, looks like we don't use SSA.uRBP in the current flows. Do we want to have this assumption though, that other places in our code don't use this particular register?
pal/src/host/linux-sgx/host_entry.S line 136 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you fix this? In a separate PR, ideally.
Done, see #1969
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
This would still be racy, what if the signal arrives after the check?
Yes, we need to additionally protect against this. We already block SIGTERM/SIGCONT on thread exiting, so we can do a similar thing for SIGUSR1:
gramine/pal/src/host/linux-sgx/host_thread.c
Lines 285 to 287 in 0c0971c
Therefore, we'll have this logic on thread_exit():
- Block SIGUSR1
- Check
g_stats_reset_leader_tid == current_tidand reset + print a warning if true - Finally exit thread
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
Signed-off-by: TejaswineeL <tejaswinee.ramdas.langhe@intel.com>
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
There was a problem hiding this comment.
Reviewed 9 of 9 files at r14, 13 of 13 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @TejaswineeL)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think this feature is quite useful, as knowing the approximate number of EENTER/EEXIT/AEX events is useful to pinpoint a potential bottleneck. And for many workloads, the noise of these events at startup time (when Gramine and SGX enclave are only starting and warming up) is significant, so it's quite informative to "flush" the startup-time events first, then start the actual workload (like start the clients against the Gramine app), and then dump the events/statistics again.
So to me, the feature is quite useful, and I've seen this PR (more like the code behind it) being useful for profiling some (proprietary) applications.
If @mkow insists, we could move this feature behind the DEBUG build of Gramine. Though please pay notice that this will break backwards compatibility, as SGX-stats feature is currently working also in RELEASE builds. UPDATE: Now I see that @mkow insists on this (see other comment), so let's move this SGX-stats feature under DEBUG builds (it is still fine because it will also work under DEBUGOPTIMIZED builds, so no issues with fidelity).
Done, new version is ready for review by @mkow
libos/test/regression/multi_pthread.manifest.template line 16 at r15 (raw file):
sgx.debug = true sgx.edmm_enable = {{ 'true' if env.get('EDMM', '0') == '1' else 'false' }} sgx.enable_stats = true
FYI: I removed sgx.enable_stats = true from 4 tests, since they are also run in release mode (where Gramine now prints an error and exits).
These sgx.enable_stats = true lines were anyway useless. I guess they were added to test this functionality manually using these tests. Now it's removed and instead added to one file which is only run in debug mode.
pal/src/host/linux-sgx/host_entry.S line 87 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yep, looks like we don't use
SSA.uRBPin the current flows. Do we want to have this assumption though, that other places in our code don't use this particular register?
Anyway, for this PR, I reverted all the changes, leaving only calls to the new function maybe_dump_and_reset_stats().
For easy review, please review from the first revision.
pal/src/host/linux-sgx/host_entry.S line 136 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done, see #1969
#1969 was merged, I'll need to rebase.
pal/src/host/linux-sgx/host_exception.c line 47 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Well, it's both on AEX and on OCALL, so the text should be:
* this thread's statistics are dumped and reset on the next AEX or right-before * executing the next OCALL in untrusted runtime.
Done
pal/src/host/linux-sgx/host_exception.c line 52 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Yup, please fix the comment then :)
Done
pal/src/host/linux-sgx/host_exception.c line 228 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, sure, we can do it. I also don't have any objections to allow this only in DEBUG builds. Let me put a blocking comment.
Done, everything is hidden behind ifdef DEBUG
pal/src/host/linux-sgx/host_exception.c line 231 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This would still be racy, what if the signal arrives after the check?
Yes, we need to additionally protect against this. We already block SIGTERM/SIGCONT on thread exiting, so we can do a similar thing for SIGUSR1:
gramine/pal/src/host/linux-sgx/host_thread.c
Lines 285 to 287 in 0c0971c
Therefore, we'll have this logic on
thread_exit():
- Block SIGUSR1
- Check
g_stats_reset_leader_tid == current_tidand reset + print a warning if true- Finally exit thread
Done
pal/src/host/linux-sgx/host_exception.c line 333 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yep, I think this is better. We can do this.
Done
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
To avoid the race condition we can add spinlock on
DBGINFO_ADDRplease let me know your opinion on this.No, I think @mkow is right and should not use DBGINFO_ADDR variable at all.
The point when, the
SIGUSR1signal is received,enclave_thread_mapdoes not have all the threads.This is surprising. Please double-check why that happened -- does it happen even if you're under the
g_enclave_thread_map_locklock?
Done, g_enclave_thread_map works.
pal/src/host/linux-sgx/host_exception.c line 334 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
@TejaswineeL: Please don't copy-paste long comments between discussions.
Done
pal/src/host/linux-sgx/host_exception.c line 338 at r13 (raw file):
Previously, TejaswineeL (Tejaswinee) wrote…
I will try with tgkill.
Done
pal/src/host/linux-sgx/host_exception.c line 345 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
SGTM
Done
pal/src/host/linux-sgx/host_exception.c line 363 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, the better approach would be:
- We introduce a new variable
g_stats_reset_epoch, and increment it on a successful wait-for-followers by the leader (in this code snippet here).- On any thread exit, we atomically increment
g_stats_reset_epoch(and also resetg_stats_reset_leader_tidif it was the TID of this exiting thread).- In this code snippet here, we check if the epoch is still the same as it was when we started the while loop. If it changed, then we know some change in the thread-number state happened, so we break the loop with a warning.
@mkow What about this?
Done
pal/src/host/linux-sgx/host_exception.c line 367 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, agreed, there's no need in a separate
pal_host_tcb_reset_stats()function. Everything can be done inupdate_and_print_stats(), true. And that func's name should be modified too.
Done
pal/src/host/linux-sgx/host_thread.c line 78 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, agreed, there's no need in a separate
pal_host_tcb_reset_stats()function. Everything can be done inupdate_and_print_stats(), true. And that func's name should be modified too.
Done
pal/src/host/linux-sgx/host_thread.c line 446 at r15 (raw file):
} size_t broadcast_signal_to_threads(int sig, int exclude_tid) {
FYI: I made a generic function. May be useful for other usages I guess.
pal/src/host/linux-sgx/pal_tcb.h line 108 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually:
/* if true, dump SGX stats and reset them on next AEX event or after next enclave exit for an OCALL */
Done (but shorter)
|
Jenkins, test this please |
Description of the changes
Currently Gramine does not support to display the stats values if user wants to collect only for a particular duration (e.g., only during perf benchmarking runs). This PR features displaying and resetting the stats data on demand using SIGUSR1 signal. That way user can collect data only for a particular duration. This also eliminates the noise added in the stats data during the start and stop of the process.
How to test this PR?
Please follow the instruction here to enable SGX stats. Now execute the workload in Gramine SGX. Send SIGUSR1 signal using command
kill -SIGUSR1 <pid>to dump and reset the stats. This will show the current stats values on the command line and then will reset the stats values. Example:1. Add below manifest options to gramine/CI-Examples/redis/redis-server.manifest.template.
sgx.debug = truesgx.enable_stats=true2. Build the redis example:
make clean && make SGX=13. Run redis server:
gramine-sgx redis-server &4. Get the redis server pid and run
kill -SIGUSR1 <pid>to dump and reset the stats.5. Immediately run the benchmarks using command
src/src/redis-benchmark6. Once benchmark run is complete, run
kill -SIGUSR1 <pid>again to get the stats values corresponding to only thebenchmark execution.
8. Summation(total) value of stats values of all the threads are displayed at the end.
9. That’s how we get the stats values and the summation of them corresponding to the benchmark run. The stats are
displayed on the command line.
Same steps can be tried with MongoDB, MySQL etc..
This change is