[PAL/Linux-SGX] Replace sgx.thread_num with sgx.max_threads

[dimakuv]

Description of the changes

Similarly, sgx.insecure__rpc_thread_num is replaced with sgx.insecure__rpc_max_threads. Note that only the manifest-option names are replaced, but the internal C code names are unmodified.

See #981 for the context and discussions.

How to test this PR?

CI. I modified tests/examples + I left one test using the deprecated old names.

---

This change is 

[mkow]

Reviewed 33 of 33 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

---

pal/src/host/linux-sgx/host_main.c line 667 at r1 (raw file):

    int64_t thread_num_int64;
    ret = toml_int_in(manifest_root, "sgx.max_threads", /*defaultval=*/0, &thread_num_int64);
    if (ret < 0) {

I don't think it works, which means the tests you added don't actually test it :P

---

pal/src/host/linux-sgx/host_main.c line 674 at r1 (raw file):

            ret = -EINVAL;
            goto out;
        }

there should also be a deprecation warning here

---

pal/src/host/linux-sgx/host_main.c line 695 at r1 (raw file):

    ret = toml_int_in(manifest_root, "sgx.insecure__rpc_max_threads", /*defaultval=*/0,
                      &rpc_thread_num_int64);
    if (ret < 0) {

ditto

---

pal/src/host/linux-sgx/host_main.c line 704 at r1 (raw file):

            ret = -EINVAL;
            goto out;
        }

ditto

---

pal/src/host/linux-sgx/pal_main.c line 711 at r1 (raw file):

    ret = toml_int_in(g_pal_public_state.manifest_root, "sgx.insecure__rpc_max_threads",
                      /*defaultval=*/0, &rpc_thread_num);
    if (ret < 0) {

ditto

[dimakuv]

Reviewable status: 31 of 33 files reviewed, 5 unresolved discussions, 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 (waiting on @mkow)

---

pal/src/host/linux-sgx/host_main.c line 667 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't think it works, which means the tests you added don't actually test it :P

Done.

This one is interesting. I modified the manifest.py script to forcefuly switch from thread_num -> max_threads (removing the former). That's why I couldn't detect this bug in the C code, even though I have one LibOS regression test with a deprecated syntax (i.e., the Python helper script removes sgx.thread_num so the C code doesn't even get into the if clause).

Now I wonder -- maybe I should just remove the check from the C code, and unconditionally use sgx.max_threads? Because to correctly sign the enclave, the user must run it through the manifest.py (which is used by gramine-sgx-sign) anyway.

Thus, using our Python tools, it's simply impossible to have the final manifest.sgx file with legacy sgx.thread_num...

So, should I just delete this second toml_int_in("sgx.thread_num")? Ditto for the RPC threads (exitless).

---

pal/src/host/linux-sgx/host_main.c line 674 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

there should also be a deprecation warning here

Done.

---

pal/src/host/linux-sgx/host_main.c line 695 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

---

pal/src/host/linux-sgx/host_main.c line 704 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

---

pal/src/host/linux-sgx/pal_main.c line 711 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

[boryspoplawski]

Reviewed 31 of 33 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, 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 (waiting on @dimakuv and @mkow)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

# app runs with 4 parallel threads + Gramine has couple internal threads
# TODO: legacy names just for testing, deprecated in v1.4, remove two versions after

I probably ask it for one too many time, but didn't we decide to make this cycle 2 releases, i.e. deprecate it in the next version (here v1.4) and the remove in the following (here v1.5) ?

---

pal/src/host/linux-sgx/host_main.c line 667 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

This one is interesting. I modified the manifest.py script to forcefuly switch from thread_num -> max_threads (removing the former). That's why I couldn't detect this bug in the C code, even though I have one LibOS regression test with a deprecated syntax (i.e., the Python helper script removes sgx.thread_num so the C code doesn't even get into the if clause).

Now I wonder -- maybe I should just remove the check from the C code, and unconditionally use sgx.max_threads? Because to correctly sign the enclave, the user must run it through the manifest.py (which is used by gramine-sgx-sign) anyway.

Thus, using our Python tools, it's simply impossible to have the final manifest.sgx file with legacy sgx.thread_num...

So, should I just delete this second toml_int_in("sgx.thread_num")? Ditto for the RPC threads (exitless).

IMO no, we are not forcing users to use our scripts. IMO remove the pop() from python (see a comment there)

---

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

    int64_t thread_num_int64;
    ret = toml_int_in(manifest_root, "sgx.max_threads", /*defaultval=*/-1, &thread_num_int64);
    if (ret < 0 || thread_num_int64 < 0) {

Shouldn't this be 2 checks? ret < 0 -> always fail, thread_num_int64 < 0 -> check deprecated sgx.thread_num

---

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

        ret = toml_int_in(manifest_root, "sgx.thread_num", /*defaultval=*/0, &thread_num_int64);
        if (ret < 0) {
            log_error("Cannot parse 'sgx.max_threads' (legacy name 'sgx.thread_num')");

What's the point of this (legacy name ...)? If you use deprecated version, you get a warning with both names...

---

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

    ret = toml_int_in(manifest_root, "sgx.insecure__rpc_max_threads", /*defaultval=*/-1,
                      &rpc_thread_num_int64);
    if (ret < 0 || rpc_thread_num_int64 < 0) {

ditto

---

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

    ret = toml_int_in(g_pal_public_state.manifest_root, "sgx.insecure__rpc_max_threads",
                      /*defaultval=*/-1, &rpc_thread_num);
    if (ret < 0 || rpc_thread_num < 0) {

ditto

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

        # TODO: sgx.thread_num and sgx.insecure__rpc_thread_num are deprecated in v1.4,
        #       simplify below logic two versions after
        sgx.setdefault('max_threads', sgx.setdefault('thread_num', DEFAULT_THREAD_NUM))

Maybe just add a simple check here:

if 'max_threads' in sgx and 'thread_num' in sgx:
    print_error_message_and_exit

and then set both defaults without pop?

[dimakuv]

Reviewable status: all files reviewed, 11 unresolved discussions, 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 (waiting on @boryspoplawski and @mkow)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I probably ask it for one too many time, but didn't we decide to make this cycle 2 releases, i.e. deprecate it in the next version (here v1.4) and the remove in the following (here v1.5) ?

No, from what I see, we have the following rule:

1. Deprecate in the upcoming release (v1.4).
2. Do nothing in the next release (v1.5). This gives users time to migrate.
3. Remove in the next-next release (v1.6).

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

and then set both defaults without pop?

How exactly do you want to do it? If we do this:

sgx.setdefault('max_threads', sgx.setdefault('thread_num', DEFAULT_THREAD_NUM))

or this:

sgx.setdefault('max_threads', DEFAULT_THREAD_NUM)
sgx.setdefault('thread_num', DEFAULT_THREAD_NUM)

without popping (removing) sgx.thread_num, then we can have this problematic manifest:

sgx.max_threads = 8  # because it was set by the user in the template
sgx.thread_num = 4   # taken from default value because it was not set by the user in the template

In this case, Gramine will correctly use the "newer" sgx.max_threads = 8 at startup, but if the user ever looks inside the manifest.sgx file, she will be confused by the two aliases of the same option but with different values. I wanted to prevent this behavior, that's why I added pop -- then the final manifest.sgx always has one correct sgx.max_threads option.

[boryspoplawski]

Reviewable status: all files reviewed, 11 unresolved discussions, 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 (waiting on @dimakuv and @mkow)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, from what I see, we have the following rule:

1. Deprecate in the upcoming release (v1.4).
2. Do nothing in the next release (v1.5). This gives users time to migrate.
3. Remove in the next-next release (v1.6).

But users have time to migrate already in point one (i.e. they have one release cycle - 3-4 months - to migrate).

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

and then set both defaults without pop?

How exactly do you want to do it? If we do this:

sgx.setdefault('max_threads', sgx.setdefault('thread_num', DEFAULT_THREAD_NUM))

or this:

sgx.setdefault('max_threads', DEFAULT_THREAD_NUM)
sgx.setdefault('thread_num', DEFAULT_THREAD_NUM)

without popping (removing) sgx.thread_num, then we can have this problematic manifest:

sgx.max_threads = 8  # because it was set by the user in the template
sgx.thread_num = 4   # taken from default value because it was not set by the user in the template

In this case, Gramine will correctly use the "newer" sgx.max_threads = 8 at startup, but if the user ever looks inside the manifest.sgx file, she will be confused by the two aliases of the same option but with different values. I wanted to prevent this behavior, that's why I added pop -- then the final manifest.sgx always has one correct sgx.max_threads option.

Look at the snipped I posed above

[boryspoplawski]

Reviewable status: all files reviewed, 11 unresolved discussions, 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 (waiting on @dimakuv and @mkow)

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Look at the snipped I posed above

Ah, sorry, you meant manifest.sgx, not original one.
Then you can do:

if 'thread_num' not in sgx:
    sgx.setdefault('max_threads', DEFAULT_THREAD_NUM)

i.e. only set default value for the non-deprecated version

[dimakuv]

Reviewable status: all files reviewed, 11 unresolved discussions, 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 (waiting on @boryspoplawski, @mkow, and @woju)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

But users have time to migrate already in point one (i.e. they have one release cycle - 3-4 months - to migrate).

@mkow @woju

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Ah, sorry, you meant manifest.sgx, not original one.
Then you can do:

if 'thread_num' not in sgx:
    sgx.setdefault('max_threads', DEFAULT_THREAD_NUM)

i.e. only set default value for the non-deprecated version

This will pose a problem because in this case the manifest file we'll have sgx.thread_num but won't have sgx.max_threads, and the sgx_sign.py will fail because it only expects sgx.max_threads (see this file in this PR).

Do you want me to still do your change + modify sgx_sign.py such that this file does smth like:

attr.get('max_threads', attr['thread_num'])

?

[mkow]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions, 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 (waiting on @boryspoplawski, @dimakuv, and @woju)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@mkow @woju

I think it was rather as Borys said, but I'm not 100% sure.

---

pal/src/host/linux-sgx/host_main.c line 667 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

IMO no, we are not forcing users to use our scripts. IMO remove the pop() from python (see a comment there)

In general I like the idea of handling deprecated stuff externally and simplify our trusted C code. But here, as Borys said, the problem is that not everyone may be using our templating scripts and only use the signer.
But I'd gladly hear if you have a good idea how to redesign the flow to allow handling deprecation outside of C code.

---

pal/src/host/linux-sgx/host_main.c line 674 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

But now you print it also when none of the two options was given in the manifest.

---

pal/src/host/linux-sgx/host_main.c line 704 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

ditto

---

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

        }
        log_warning("Detected deprecated syntax: 'sgx.insecure__rpc_thread_num'. Consider "
                    "switching to 'sgx.insecure__rpc_max_threads'.");

ditto

[boryspoplawski]

Reviewable status: all files reviewed, 10 unresolved discussions, 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 (waiting on @dimakuv and @woju)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think it was rather as Borys said, but I'm not 100% sure.

We definitely write everywhere (i.e. in code where we have such TODOs) that it's after two versions, but I also think we talked about it at some point and the conclusion was: next version deprecates, next-next removes, but not sure

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This will pose a problem because in this case the manifest file we'll have sgx.thread_num but won't have sgx.max_threads, and the sgx_sign.py will fail because it only expects sgx.max_threads (see this file in this PR).

Do you want me to still do your change + modify sgx_sign.py such that this file does smth like:

attr.get('max_threads', attr['thread_num'])

?

Idk, I trust you can come up with a version that works and makes sense :) But I guess there are 2 options (?) - add support for both in sgx_sign.py or always force users to use max_threads - both are not ideal.

[dimakuv]

Reviewable status: 28 of 33 files reviewed, 10 unresolved discussions, 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 (waiting on @boryspoplawski, @mkow, and @woju)

---

libos/test/regression/multi_pthread_exitless.manifest.template line 12 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

We definitely write everywhere (i.e. in code where we have such TODOs) that it's after two versions, but I also think we talked about it at some point and the conclusion was: next version deprecates, next-next removes, but not sure

Done.

---

pal/src/host/linux-sgx/host_main.c line 667 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

In general I like the idea of handling deprecated stuff externally and simplify our trusted C code. But here, as Borys said, the problem is that not everyone may be using our templating scripts and only use the signer.
But I'd gladly hear if you have a good idea how to redesign the flow to allow handling deprecation outside of C code.

Ok, did as you both wanted.

---

pal/src/host/linux-sgx/host_main.c line 674 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But now you print it also when none of the two options was given in the manifest.

We actually didn't have any logic for "the option is not given in the manifest" before... I fixed it.

---

pal/src/host/linux-sgx/host_main.c line 704 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

Shouldn't this be 2 checks? ret < 0 -> always fail, thread_num_int64 < 0 -> check deprecated sgx.thread_num

Done.

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

What's the point of this (legacy name ...)? If you use deprecated version, you get a warning with both names...

Done.

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done

---

pal/src/host/linux-sgx/host_main.c line 689 at r3 (raw file):

        thread_num_option = "sgx.thread_num";
        log_error("Detected deprecated syntax: 'sgx.thread_num'. Consider switching to "
                  "'sgx.max_threads'.");

FYI: Note that this is now log_error(). Previously it was log_warning(), but this is printed even before we parse and set the log level, so it was always skipped (because default log level is error). So I changed to error log level to make sure it definitely prints, and verified via the multithread_exitless LibOS test.

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

---

python/graminelibos/manifest.py line 92 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Idk, I trust you can come up with a version that works and makes sense :) But I guess there are 2 options (?) - add support for both in sgx_sign.py or always force users to use max_threads - both are not ideal.

Done. Ok, going with the first option, as you and Michal both requested smth like this.

[boryspoplawski]

Reviewed 5 of 5 files at r3, 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 (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

pal/src/host/linux-sgx/host_main.c line 674 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We actually didn't have any logic for "the option is not given in the manifest" before... I fixed it.

We had logic of defaulting to 1. Now you changed it to fail if no option given, but change 0 -> 1. Is that what we want? I would rather fail on 0.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

I meant why not always print the current name? If you use legacy, you get a warning about switching to the new version (so you know both names and seeing the new one won't be a surprise).

---

pal/src/host/linux-sgx/host_main.c line 719 at r3 (raw file):

            goto out;
        }
        if (rpc_thread_num_int64 < 0) {

How does it work? Each manifest must have this option now and if it doesn't want this feature it must specify 0?

---

pal/src/host/linux-sgx/host_main.c line 732 at r3 (raw file):

    if (enclave_info->rpc_thread_num > MAX_RPC_THREADS) {
        log_error("Too large '%s', maximum allowed is %d", rpc_thread_num_option, MAX_RPC_THREADS);

ditto (IMO just print the new name)

---

pal/src/host/linux-sgx/pal_main.c line 724 at r3 (raw file):

        }
        if (rpc_thread_num < 0) {
            log_error("'sgx.insecure__rpc_max_threads' not found in the manifest");

ditto

[dimakuv]

Reviewable status: 31 of 33 files reviewed, 8 unresolved discussions, 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 (waiting on @boryspoplawski and @mkow)

---

pal/src/host/linux-sgx/host_main.c line 674 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

We had logic of defaulting to 1. Now you changed it to fail if no option given, but change 0 -> 1. Is that what we want? I would rather fail on 0.

Done.

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

I meant why not always print the current name? If you use legacy, you get a warning about switching to the new version (so you know both names and seeing the new one won't be a surprise).

Done.

---

pal/src/host/linux-sgx/host_main.c line 719 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

How does it work? Each manifest must have this option now and if it doesn't want this feature it must specify 0?

Done. Now explicitly set to zero if no option was provided.

Previously it worked because our Python scripts always specify this option (if not specified by user, then the Python script sets it to zero). So that's why I didn't notice this bug.

---

pal/src/host/linux-sgx/host_main.c line 732 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto (IMO just print the new name)

Done.

---

pal/src/host/linux-sgx/pal_main.c line 724 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

[boryspoplawski]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)

[mkow]

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

---

pal/src/host/linux-sgx/host_main.c line 725 at r4 (raw file):

        }
        log_error("Detected deprecated syntax: 'sgx.insecure__rpc_thread_num'. Consider "
                  "switching to 'sgx.insecure__rpc_max_threads'.");

Actually... isn't "num" the correct term in this case? For exitless we just spawn all these threads, it's not their maximal number, but their exact number?

[dimakuv]

Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

pal/src/host/linux-sgx/host_main.c line 725 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Actually... isn't "num" the correct term in this case? For exitless we just spawn all these threads, it's not their maximal number, but their exact number?

Hm. Well, yes, but these threads do not spin all the time, they sleep periodically (when there's no new requests):

gramine/pal/src/host/linux-sgx/host_ocalls.c

Line 815 in f512b33

(void)DO_SYSCALL(nanosleep, &tv, /*rem=*/NULL);

I'm not sure we want to keep the old name, because then we'll have a discrepancy in naming (max_threads vs rpc_thread_num). Also, maybe we'll change Exitless to spawn new RPC threads only when needed -- then rpc_max_threads will be a maximal number.

[mkow]

Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

pal/src/host/linux-sgx/host_main.c line 725 at r4 (raw file):
They sleep a bit, but it's still just a busy spinning. Anyways, there's exactly sgx.insecure__rpc_thread_num of them, so "max" doesn't seem appropriate.

then we'll have a discrepancy in naming

That's intended IMO, because both options have different semantics (one is max, the other is precise number).

Also, maybe we'll change Exitless to spawn new RPC threads only when needed -- then rpc_max_threads will be a maximal number.

Then we can do the interface change you are proposing here ;)

I won't be blocking on this, but let's also hear from @boryspoplawski.

[boryspoplawski]

Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

pal/src/host/linux-sgx/host_main.c line 725 at r4 (raw file):
Ah, true, missed that.

I'm not sure we want to keep the old name, because then we'll have a discrepancy in naming (max_threads vs rpc_thread_num). Also, maybe we'll change Exitless to spawn new RPC threads only when needed -- then rpc_max_threads will be a maximal number.

then we'll have a discrepancy in naming

But the discrepancy isn't undue, in RPC we spawn each thread on the host upfront, in max_threads case we spawn them on demand.

then we'll have a discrepancy in naming

That's intended IMO, because both options have different semantics (one is max, the other is precise number).

Yes, exactly.

So I think the old name can stay (i.e. max)

[dimakuv]

Reviewable status: 25 of 33 files reviewed, 3 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 @boryspoplawski and @mkow)

---

-- commits line 5 at r4:
I will remove the first sentence, because we decided to revert the change with RPC (Exitless) threads.

---

pal/src/host/linux-sgx/host_main.c line 725 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Ah, true, missed that.

I'm not sure we want to keep the old name, because then we'll have a discrepancy in naming (max_threads vs rpc_thread_num). Also, maybe we'll change Exitless to spawn new RPC threads only when needed -- then rpc_max_threads will be a maximal number.

then we'll have a discrepancy in naming

But the discrepancy isn't undue, in RPC we spawn each thread on the host upfront, in max_threads case we spawn them on demand.

then we'll have a discrepancy in naming

That's intended IMO, because both options have different semantics (one is max, the other is precise number).

Yes, exactly.

So I think the old name can stay (i.e. max)

Done.

---

pal/src/host/linux-sgx/host_main.c line 723 at r5 (raw file):

        goto out;
    }
    enclave_info->rpc_thread_num = rpc_thread_num_int64;

FYI: Just switched these two statements places. I think it's more readable.

[boryspoplawski]

Reviewed 7 of 8 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

pal/src/host/linux-sgx/host_main.c line 723 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

FYI: Just switched these two statements places. I think it's more readable.

hmm? I don't see any changes here

[dimakuv]

Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

pal/src/host/linux-sgx/host_main.c line 723 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

hmm? I don't see any changes here

I mean, I reverted all changes to rpc_thread_num in this PR, except this particular change (where I swapped the two statements places). You can see this change when you look at the full range of commits in this file.

[mkow]

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

---

Documentation/manifest-syntax.rst line 1000 at r6 (raw file):

    sgx.thread_num = [NUM]

This name was ambigious and was replaced with ``sgx.max_threads``.

Suggestion:

ambiguous

[dimakuv]

Reviewable status: 27 of 33 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski and @mkow)

---

-- commits line 5 at r4:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I will remove the first sentence, because we decided to revert the change with RPC (Exitless) threads.

Done

---

Documentation/manifest-syntax.rst line 1000 at r6 (raw file):

    sgx.thread_num = [NUM]

This name was ambigious and was replaced with ``sgx.max_threads``.

Done.

[mkow]

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved