[PAL/Linux-SGX] Remove printing of thread-local SGX statistics #1995

dimakuv · 2024-09-04T07:53:18Z

Description of the changes

Previously, when manifest contained sgx.enable_stats = true, Gramine-SGX printed not the process-wide SGX statistics, but also statistics for each thread. These thread-local SGX statistics only litter the output as (1) there are helper Gramine threads that do not correspond to any application thread, (2) there is no way to correlate the printed thread-local stats to application threads/workload execution (as stats simply print the opaque TID of the thread).

This PR removes thread-local stats and keeps only process-wide ones.

How to test this PR?

The Helloworld CI example (with sgx.enable_stats = true):

With this PR:

gramineproject/gramine/CI-Examples/helloworld$ gramine-sgx helloworld
Gramine is starting. Parsing TOML manifest file, this may take some time...
----- SGX enclave loading time =     412964 microseconds -----
-----------------------------------------------------------------------------------------------------------------------
Gramine detected the following insecure configurations:

  - sgx.debug = true                           (this is a debug enclave)

Gramine will continue application execution, but this configuration must not be used in production!
-----------------------------------------------------------------------------------------------------------------------

Hello, world
----- Total SGX stats for process 273134 -----
  # of EENTERs:        317
  # of EEXITs:         241
  # of AEXs:           2175
  # of sync signals:   72
  # of async signals:  0

Without this PR:

gramineproject/gramine/CI-Examples/helloworld$ gramine-sgx helloworld
Gramine is starting. Parsing TOML manifest file, this may take some time...
----- SGX enclave loading time =     410795 microseconds -----
-----------------------------------------------------------------------------------------------------------------------
Gramine detected the following insecure configurations:

  - sgx.debug = true                           (this is a debug enclave)

Gramine will continue application execution, but this configuration must not be used in production!
-----------------------------------------------------------------------------------------------------------------------

----- SGX stats for thread 323750 -----
  # of EENTERs:        15
  # of EEXITs:         14
  # of AEXs:           19
  # of sync signals:   0
  # of async signals:  0
----- SGX stats for thread 323751 -----                                                                               [0/1860]
  # of EENTERs:        15
  # of EEXITs:         14
  # of AEXs:           0
  # of sync signals:   0
  # of async signals:  0
----- SGX stats for thread 323753 -----
  # of EENTERs:        15
  # of EEXITs:         14
  # of AEXs:           0
  # of sync signals:   0
  # of async signals:  0
Hello, world
----- SGX stats for thread 323754 -----
  # of EENTERs:        3
  # of EEXITs:         2
  # of AEXs:           0
  # of sync signals:   0
  # of async signals:  0
----- SGX stats for thread 323683 -----
  # of EENTERs:        269
  # of EEXITs:         197
  # of AEXs:           2162
  # of sync signals:   72
  # of async signals:  0
----- Total SGX stats for process 323683 -----
  # of EENTERs:        317
  # of EEXITs:         241
  # of AEXs:           2181
  # of sync signals:   72
  # of async signals:  0

This change is

dimakuv

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

a discussion (no related file):
@mkow @woju @jkr0103 @kailun-qin @mwkmwkmwk What do you think about this PR? It changes user-facing (profiling) output, do we care? I am 99.9% sure no users rely on thread-local outputs.

dimakuv

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@mkow @woju @jkr0103 @kailun-qin @mwkmwkmwk What do you think about this PR? It changes user-facing (profiling) output, do we care? I am 99.9% sure no users rely on thread-local outputs.

Feedback from @aneessahib: there were workloads where thread-local statistics were useful in debugging. However, hopefully with #1996 (the ability to reset stats on a user signal), the debugging with process-wide-only statistics will be similarly useful. So @aneessahib is Ok with both options: merging this PR and removing it.

kailun-qin

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, @jkr0103, @mkow, @mwkmwkmwk, and @woju)

a discussion (no related file):

there were workloads where thread-local statistics were useful in debugging

Yep, I guess so. But I'm fine w/ this PR.

-- commits line 5 at r2:
-> not only?

Code quote:

not

mkow

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, @jkr0103, @mwkmwkmwk, and @woju)

-- commits line 8 at r2:

Suggestion:

there is no easy way

pal/src/host/linux-sgx/host_ocalls.c line 63 at r2 (raw file):

    if (ocall_exit_args->is_exitgroup) {
        process_exit((int)ocall_exit_args->exitcode);
        die_or_inf_loop();

process_exit() is already noreturn

pal/src/host/linux-sgx/pal_tcb.h line 118 at r2 (raw file):

extern bool g_sgx_enable_stats;
void update_sgx_stats(bool print_process_wide_stats);

Suggestion:

do_print

mkow

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, @jkr0103, @mwkmwkmwk, and @woju)

a discussion (no related file):

Enabling per-thread and process-wide SGX stats

manifest-syntax.rst also needs an update.

mkow

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, @jkr0103, @mwkmwkmwk, and @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Enabling per-thread and process-wide SGX stats

manifest-syntax.rst also needs an update.

ah, and performance.rst also

dimakuv

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions, 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 @dimakuv, @kailun-qin, @mkow, and @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

ah, and performance.rst also

Done. Good catch.

-- commits line 5 at r2:

Previously, kailun-qin (Kailun Qin) wrote…

-> not only?

Will do during final rebase

-- commits line 8 at r2:
Will do during final rebase

pal/src/host/linux-sgx/host_ocalls.c line 63 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

process_exit() is already noreturn

Done.

pal/src/host/linux-sgx/pal_tcb.h line 118 at r2 (raw file):

extern bool g_sgx_enable_stats;
void update_sgx_stats(bool print_process_wide_stats);

Done.

mkow

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)

kailun-qin

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)

Previously, when manifest contained `sgx.enable_stats = true`, Gramine-SGX printed not only the process-wide SGX statistics, but also statistics for each thread. These thread-local SGX statistics only litter the output as (1) there are helper Gramine threads that do not correspond to any application thread, (2) there is no easy way to correlate the printed thread-local stats to application threads/workload execution (as stats simply print the opaque TID of the thread). This commit removes thread-local stats and keeps only process-wide ones. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

dimakuv

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @kailun-qin, @mkow, and @woju)

-- commits line 5 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during final rebase

Done.

-- commits line 8 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during final rebase

Done.

mkow

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @woju)

kailun-qin

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @woju)

dimakuv commented Sep 4, 2024

View reviewed changes

dimakuv mentioned this pull request Sep 4, 2024

[PAL/Linux-SGX] Print SGX stats on SIGUSR1 and reset them #1996

Merged

dimakuv commented Sep 4, 2024

View reviewed changes

kailun-qin requested review from mkow and woju September 11, 2024 00:09

kailun-qin previously approved these changes Sep 11, 2024

View reviewed changes

mkow requested changes Sep 16, 2024

View reviewed changes

mkow requested changes Sep 17, 2024

View reviewed changes

dimakuv dismissed kailun-qin’s stale review via f66ba21 September 18, 2024 10:45

dimakuv commented Sep 18, 2024

View reviewed changes

mkow approved these changes Sep 18, 2024

View reviewed changes

kailun-qin approved these changes Sep 19, 2024

View reviewed changes

dimakuv force-pushed the dimakuv/sgx-stats-rm-thread-local-prints branch from f66ba21 to 2ff0ab1 Compare September 24, 2024 14:15

dimakuv commented Sep 24, 2024

View reviewed changes

mkow approved these changes Sep 24, 2024

View reviewed changes

kailun-qin approved these changes Sep 24, 2024

View reviewed changes

dimakuv merged commit 2ff0ab1 into master Sep 24, 2024
18 checks passed

dimakuv deleted the dimakuv/sgx-stats-rm-thread-local-prints branch September 24, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAL/Linux-SGX] Remove printing of thread-local SGX statistics #1995

[PAL/Linux-SGX] Remove printing of thread-local SGX statistics #1995

dimakuv commented Sep 4, 2024 •

edited by mkow

Loading

dimakuv left a comment

dimakuv left a comment

kailun-qin left a comment

mkow left a comment

mkow left a comment

mkow left a comment

dimakuv left a comment

mkow left a comment

kailun-qin left a comment

dimakuv left a comment

mkow left a comment

kailun-qin left a comment

[PAL/Linux-SGX] Remove printing of thread-local SGX statistics #1995

[PAL/Linux-SGX] Remove printing of thread-local SGX statistics #1995

Conversation

dimakuv commented Sep 4, 2024 • edited by mkow Loading

Description of the changes

How to test this PR?

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv commented Sep 4, 2024 •

edited by mkow

Loading