Add checkpoint-and-restore support for sysfs

[vijaydhanraj]

Description of the changes

This PR adds checkpoint-and-restore support for sysfs. It addresses issue 3 listed as part of gramineproject/graphene#2105

How to test this PR?

Please run sysfs_common and proc_common regression tests to validate the PR.

---

This change is 

[dimakuv]

Reviewable status: 0 of 25 files reviewed, 6 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 @vijaydhanraj)

a discussion (no related file):
Very high-level comments on this PR:

• There is no need in introducing a separate DkGetPalTopologyInfo() function and the corresponding commit (i.e., commit [Pal,LibOS] Refactor PAL_TOPO_INFO struct usage should be dropped from this PR!)

• Instead, you can still start LibOS execution with the topology given to us by the PAL layer.

  • I understand that you think "Oh, but this way we'll do excessive work: we'll first load topology from PAL, and then overwrite this topology with LibOS checkpoint"
  • But first of all, this doesn't affect performance: creating new processes is a rare event, and it is already slow, so who cares. Also, we can always add your trick of if (!parent) { ... only then parse topo ... }.
  • And second of all, remember that theoretically PAL shouldn't have any assumptions on the LibOS. In other words, PAL code should not assume "Ok, LibOS will restore the topology from the LibOS checkpoint, I don't need to do anything". Otherwise in the future we'll write another LibOS implementation that doesn't do checkpointing, and the PAL assumption will break.

• Now, in the child enclave, when LibOS initialized itself (with a useless topology from the PAL), LibOS will go into the checkpoint-restore logic. In this restore logic, LibOS will rewire your PAL_TOPO_INFO* g_topo_info to the copy restored from the checkpoint.

• Note that for the main (parent) enclave, PAL_TOPO_INFO* g_topo_info can be initialized to simply: g_topo_info = g_pal_control->topo_info. No need to introduce DkGetPalTopologyInfo().

---

a discussion (no related file):
Actually, given my radical comments, you may want to create another, parallel PR to this one -- just in case if some reviewers will completely disagree with me. Then we will be able to choose between the two PRs, with two variants of the checkpoint-and-restore sysfs logic.

---

---

-- commits, line 35 at r1:
Remove this commit as I explained in the top-level comment.

---

LibOS/shim/src/shim_init.c, line 432 at r1 (raw file):

        /* Updated via checkpoint-and-restore on child processes. */
        g_topo_info = DkGetPalTopologyInfo();
        assert(g_topo_info);

You can remove this and simply do g_topo_info = g_pal_control->topo_info at the very beginning of this function. (Maybe you would need an explicit cast to remove the const qualifier, but it's not a problem -- it is meaningful in this context.)

---

LibOS/shim/src/fs/sys/fs.c, line 21 at r1 (raw file):

int64_t g_num_cores_online;
int64_t g_num_nodes_online;
int64_t g_num_cache_lvls;

What is the point in these three globals? You can simply use g_topo_info->online_logical_cores.resource_count instead of g_num_cores_online, for example. Please remove them.

---

LibOS/shim/src/fs/sys/fs.c, line 481 at r1 (raw file):

    g_topo_info = topo_info;
}
END_RS_FUNC(topo_info)

I haven't yet reviewed these checkpoint and restore functions deeply, but look good to me.

[vijaydhanraj]

Reviewable status: 0 of 25 files reviewed, 6 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 @dimakuv and @vijaydhanraj)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Very high-level comments on this PR:

• There is no need in introducing a separate DkGetPalTopologyInfo() function and the corresponding commit (i.e., commit [Pal,LibOS] Refactor PAL_TOPO_INFO struct usage should be dropped from this PR!)

• Instead, you can still start LibOS execution with the topology given to us by the PAL layer.

  • I understand that you think "Oh, but this way we'll do excessive work: we'll first load topology from PAL, and then overwrite this topology with LibOS checkpoint"
  • But first of all, this doesn't affect performance: creating new processes is a rare event, and it is already slow, so who cares. Also, we can always add your trick of if (!parent) { ... only then parse topo ... }.
  • And second of all, remember that theoretically PAL shouldn't have any assumptions on the LibOS. In other words, PAL code should not assume "Ok, LibOS will restore the topology from the LibOS checkpoint, I don't need to do anything". Otherwise in the future we'll write another LibOS implementation that doesn't do checkpointing, and the PAL assumption will break.

• Now, in the child enclave, when LibOS initialized itself (with a useless topology from the PAL), LibOS will go into the checkpoint-restore logic. In this restore logic, LibOS will rewire your PAL_TOPO_INFO* g_topo_info to the copy restored from the checkpoint.

• Note that for the main (parent) enclave, PAL_TOPO_INFO* g_topo_info can be initialized to simply: g_topo_info = g_pal_control->topo_info. No need to introduce DkGetPalTopologyInfo().

One of the reasons to create a new global is to remove topology info from g_pal_control. Secondly, g_pal_control is a const pointer and I didn't want to override it. I feel adding a new function makes it cleaner and more independent. Any strong reason not to add DkGetPalTopologyInfo()?

Suggest we still have this but to ensure that PAL doesn't have any assumptions on the LibOS, I can remove the if(!parent) checks and allow PAL to pass the host information on each fork but later LibOS can override it with CP and RS.

---

[dimakuv]

Reviewable status: 0 of 25 files reviewed, 6 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 @vijaydhanraj)

a discussion (no related file):

Any strong reason not to add DkGetPalTopologyInfo()?

Mainly because I'm very much against adding new Dk functions (i.e., increasing the interface between the LibOS and the PAL layers). We are trying hard to keep this interface minimal.

Also, we already have DkGetPalControl() API which should be usable for all information that is passed from PAL to LibOS on startup (that's the point of this function). CPU/NUMA topology falls into this category, so I would prefer it to be handled by this function, rather than something new.

Secondly, g_pal_control is a const pointer and I didn't want to override it.

But you won't override it. You'll only do g_topo_info = g_pal_control->topo_info.

---

[vijaydhanraj]

Reviewable status: 0 of 25 files reviewed, 6 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 @dimakuv and @vijaydhanraj)

a discussion (no related file):

You'll only do g_topo_info = g_pal_control->topo_info

Shouldn't it be g_topo_info = &g_pal_control->topo_info? And with this, compiler will throw an error assignment discards ‘const’ qualifier from pointer target type. PS: Here g_topo_info isn't const pointer as we update it during restore.

Another approach will be to do a deep copy, but it is a pain. Not sure if it is okay but we can also remove const from g_pal_control.
But why is it an issue to add just one Dk function?

---

[dimakuv]

Reviewable status: 0 of 25 files reviewed, 6 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 @vijaydhanraj)

a discussion (no related file):

assignment discards ‘const’ qualifier from pointer target type

This GCC error makes sense, yes. Well, you can just cast to a no-const type. Or drop const qualifier from g_topo_info, also fine (with a comment explaining why g_topo_info is not exactly const).

But why is it an issue to add just one Dk function?

Because Dk functions are the API between the LibOS and PAL. It should be as small as possible. Just like Linux maintainers are very much against adding new system calls without a good reason, Gramine maintainers are also against adding new Dk functions. What would be the reason of adding this new Dk function? To avoid a type cast?

---

[vijaydhanraj]

Reviewable status: 0 of 25 files reviewed, 6 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 @dimakuv and @vijaydhanraj)

a discussion (no related file):

Well, you can just cast to a no-const type.

But doesn't this cause an undefined behavior? Basically, we are overriding what we told the compiler earlier.

Or drop const qualifier from g_topo_info

Here did you mean to drop const qualifier forg_pal_control? Is this fine? This will be simple change.

What would be the reason of adding this new Dk function? To avoid a type cast?

yes, didn't like to override in the first place.

---

[dimakuv]

Reviewable status: 0 of 25 files reviewed, 6 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 @vijaydhanraj)

a discussion (no related file):

Here did you mean to drop const qualifier forg_pal_control? Is this fine? This will be simple change.

I don't mind this change. Feel free to do it (add a comment that it is not const because topo_info may be overriden by LibOS init code for the child process).

---

[vijaydhanraj]

Reviewable status: 0 of 25 files reviewed, 6 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 @dimakuv and @vijaydhanraj)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Here did you mean to drop const qualifier forg_pal_control? Is this fine? This will be simple change.

I don't mind this change. Feel free to do it (add a comment that it is not const because topo_info may be overriden by LibOS init code for the child process).

sounds good. Will do drop the const qualifier for g_pal_control and will add a comment as to why we are doing it.

---

[vijaydhanraj]

Reviewable status: 0 of 26 files reviewed, 6 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 @dimakuv and @vijaydhanraj)

---

-- commits, line 35 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Remove this commit as I explained in the top-level comment.

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 21 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is the point in these three globals? You can simply use g_topo_info->online_logical_cores.resource_count instead of g_num_cores_online, for example. Please remove them.

Done.

[dimakuv]

Reviewed 5 of 25 files at r1, 21 of 21 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 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 @vijaydhanraj)

---

LibOS/shim/src/shim_init.c, line 40 at r2 (raw file):

const toml_table_t* g_manifest_root = NULL;
/* We don't have const qualifier for this struct as topo_info struct part of this structure maybe
 * overriden during LibOS initialization for the child process. */

/* We don't have const qualifier for `g_pal_control` as its `topo_info` field may be overriden 
 * during LibOS initialization (for child processes which inherit `topo_info` from the parent
 * process). FIXME: We may need to rethink `g_pal_control` structure. */

---

LibOS/shim/src/fs/sys/fs.c, line 278 at r2 (raw file):

        for (uint64_t idx = 0; idx < num_nodes_online; idx++) {
            if (numa_topo[idx].cpumap.range_count > 0) {

Could you nullify the ranges before this first IF statement? Like this:

new_numa_topo[idx].cpumap.ranges = NULL;
new_numa_topo[idx].distance.ranges = NULL;

---

LibOS/shim/src/fs/sys/fs.c, line 320 at r2 (raw file):

        for (uint64_t idx = 0; idx < num_cache_lvls; idx++) {
            if (cache[idx].shared_cpu_map.range_count > 0) {

Could you nullify the ranges before this IF statement? Like this:

new_cache[idx].shared_cpu_map.ranges = NULL;

---

LibOS/shim/src/fs/sys/fs.c, line 355 at r2 (raw file):

        memcpy(new_core_topo, core_topo, core_topo_sz);

        for (uint64_t idx = 0; idx < num_cores_online; idx++) {

Could you nullify the ranges before this first IF statement? Like this:

new_core_topo[idx].core_siblings.ranges = NULL;
new_core_topo[idx].thread_siblings.ranges = NULL;

---

LibOS/shim/src/fs/sys/fs.c, line 400 at r2 (raw file):

        new_topo_info = (PAL_TOPO_INFO*)(base + off);
        *new_topo_info = *topo_info;

Could you nullify all pointer fields here? Like this:

new_topo_info->online_logical_cores.ranges = NULL;
new_topo_info->possible_logical_cores.ranges = NULL;
new_topo_info->nodes.ranges = NULL;

This will make it clear that these pointers will be overriden below (if the respective count > 0).

---

LibOS/shim/src/fs/sys/fs.c, line 468 at r2 (raw file):

        }
    }
    CP_REBASE(topo_info->numa_topology);

Please add an empty line before this line, for readability

---

LibOS/shim/src/fs/sys/fs.c, line 474 at r2 (raw file):

    }

    /* Restore topo_info from parent to the child */

Please remove this comment, it is obvious.

---

LibOS/shim/src/fs/sys/fs.c, line 482 at r2 (raw file):

    g_pal_control->topo_info.num_cache_index = topo_info->num_cache_index;
    g_pal_control->topo_info.core_topology = topo_info->core_topology;
    g_pal_control->topo_info.numa_topology = topo_info->numa_topology;

Why can't you simply do g_pal_control->topo_info = topo_info?

[vijaydhanraj]

Reviewable status: 24 of 26 files reviewed, 8 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 @dimakuv)

---

LibOS/shim/src/shim_init.c, line 40 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

/* We don't have const qualifier for `g_pal_control` as its `topo_info` field may be overriden 
 * during LibOS initialization (for child processes which inherit `topo_info` from the parent
 * process). FIXME: We may need to rethink `g_pal_control` structure. */

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 278 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you nullify the ranges before this first IF statement? Like this:

new_numa_topo[idx].cpumap.ranges = NULL;
new_numa_topo[idx].distance.ranges = NULL;

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 320 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you nullify the ranges before this IF statement? Like this:

new_cache[idx].shared_cpu_map.ranges = NULL;

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 355 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you nullify the ranges before this first IF statement? Like this:

new_core_topo[idx].core_siblings.ranges = NULL;
new_core_topo[idx].thread_siblings.ranges = NULL;

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 400 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you nullify all pointer fields here? Like this:

new_topo_info->online_logical_cores.ranges = NULL;
new_topo_info->possible_logical_cores.ranges = NULL;
new_topo_info->nodes.ranges = NULL;

This will make it clear that these pointers will be overriden below (if the respective count > 0).

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 468 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add an empty line before this line, for readability

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 474 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove this comment, it is obvious.

Done.

---

LibOS/shim/src/fs/sys/fs.c, line 482 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why can't you simply do g_pal_control->topo_info = topo_info?

Because topo_info is a pointer to struct PAL_TOPO_INFO whereas g_pal_control->topo_info is a struct of type PAL_TOPO_INFO.

[dimakuv]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, 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

---

LibOS/shim/src/fs/sys/fs.c, line 482 at r2 (raw file):

Previously, vijaydhanraj (Vijay Dhanraj) wrote…

Because topo_info is a pointer to struct PAL_TOPO_INFO whereas g_pal_control->topo_info is a struct of type PAL_TOPO_INFO.

OK, I see. It is embedded in g_pal_control, it's not a pointer. Makes sense.

[mkow]

Closing this one, as the implementation of it after sysfs topo rework will need to be different (hopefully simpler).