[LibOS] Rework IDs management

[boryspoplawski]

Description of the changes

More description in commit message and comments.

Only caveat is that listing /proc does not show remote processes (but they are still accessible via /proc/[pid]/*, you just need to know the pid).

Closes #2107

---

This change is 

[boryspoplawski]

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

a discussion (no related file):
@pwmarcz please take a look (at least) at fs related changes

---

---

common/include/api.h, line 109 at r1 (raw file):

#endif

#define IS_SIGNED(T) ((T)-1 < (T)1)

kudos to @mkow for coming up with this :)

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 211 at r1 (raw file):

}

static void release_id_range(IDTYPE start, IDTYPE end) {

start and end must denote an existing range, which means that if you change ownership of an ID, you need to release all parts separately (in case the range was split in multiple parts). This comes naturally with the current code.
If somebody has something against it, now is the time.
Maybe this requires more comments?

---

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

     * lying around, so nothing bad should happen™. Hopefully...
     */
    release_id(get_cur_thread()->tid);

I don't know how to handle this better.

[pwmarcz]

Reviewed 5 of 24 files at r1, 1 of 1 files at r2.
Reviewable status: 6 of 24 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

@pwmarcz please take a look (at least) at fs related changes

Filesystem changes look good. The rest looks too scary for me to reliably review :)

---

---

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

    struct pseudo_node* ipc_thread_pid = pseudo_add_dir(root, /*name=*/NULL);
    ipc_thread_pid->name_exists = &proc_ipc_thread_pid_name_exists;
    /* Listing remote processes is not currently supported. */

Thanks for the comment!

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 24 at r2 (raw file):

        return false;

    if (pid == g_process.pid) {

I wonder if get_all_pid_status() returned the local PID as well... I haven't checked when I wrote that.

[boryspoplawski]

Reviewable status: 6 of 24 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Filesystem changes look good. The rest looks too scary for me to reliably review :)

Thanks!

---

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 24 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I wonder if get_all_pid_status() returned the local PID as well... I haven't checked when I wrote that.

I think it did not and hat's how I discovered it - I was failing to access /proc/[pid]/maps of local process, because ipc routines took precedence of local routines (afaik they are just checked iteratively).

[boryspoplawski]

Jenkins, retest this please (just making sure).

[boryspoplawski]

Reviewable status: 6 of 24 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 56 at r2 (raw file):

        return -ENOMEM;
    }
#ifdef DO_TESTS

This is intended be removed before merging. It allows for some testing of the implementation: just add a couple of ranges with owner 0 (invalid) and change g_last_id.

[mkow]

Reviewed 14 of 24 files at r1.
Reviewable status: 16 of 24 files reviewed, 20 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski)

a discussion (no related file):
Typo in the commit message: "proces"

---

---

common/include/api.h, line 109 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

kudos to @mkow for coming up with this :)

I'm always happy to provide more hacks like this :)

---

LibOS/shim/include/shim_ipc.h, line 23 at r2 (raw file):

    IPC_MSG_RESP = 0,
    IPC_MSG_CHILDEXIT,          /*!< Child exit/death information. */
    IPC_MSG_NEW_ID_RANGE,       /*!< Request new IDs range. */

I'd add "GET" to this one to make it less confusing

---

LibOS/shim/include/shim_ipc.h, line 159 at r2 (raw file):

 *
 * \param start[out] start of the new ID range
 * \param end[out] end of the new ID range

I'd start using the out_ prefix convention. Ditto for the whole PR.

---

LibOS/shim/include/shim_ipc.h, line 159 at r2 (raw file):

 *
 * \param start[out] start of the new ID range
 * \param end[out] end of the new ID range

I think [out] should be after \param?

---

LibOS/shim/include/shim_ipc.h, line 193 at r2 (raw file):

parami

typo?

---

LibOS/shim/include/shim_thread.h, line 137 at r2 (raw file):

preaload

typo

---

LibOS/shim/include/shim_thread.h, line 146 at r2 (raw file):

 * \param remove_from_owned if true, ID is removed from owned and locally tracked IDs

Hmm, I don't understand this semantic. Could you explain? If remove_from_owned is true, then this ID will be used somehow in a different process?

---

LibOS/shim/include/shim_thread.h, line 152 at r2 (raw file):

IDTYPE get_new_id(bool remove_from_owned);
/*!
 * \brief Releases (frees) previously allocated ID

Does this release the ID to the leader, or only mark it as non-owned in current process?

---

LibOS/shim/src/bookkeep/shim_pid.c, line 22 at r2 (raw file):

    IDTYPE start;
    IDTYPE end;
    unsigned int taken_count;

Shouldn't this also be IDTYPE? OTOH, I see that this is capped by MAX_RANGE_SIZE which is small, but I don't like such a convoluted assumption.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 43 at r2 (raw file):

static struct shim_lock g_ranges_lock;

int init_id_ranges(IDTYPE preaload_tid) {

ditto (typo)

---

LibOS/shim/src/bookkeep/shim_pid.c, line 70 at r2 (raw file):

    lock(&g_ranges_lock);
    if (!g_last_range) {
        g_last_range = malloc(sizeof(*g_last_range));

I'd feel better if allocations were outside the locked region if this won't make this function too complex.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 86 at r2 (raw file):

        assert(start <= end);
        assert(end - start + 1 <= MAX_RANGE_SIZE);
        assert(start > 0);

So, ID == 0 is somehow a reserved/magic one? Or it's banned only to simplify the calculations below and allow them to safely use - 1 on it? (and to not have to return the ID using an out arg from here?)

---

LibOS/shim/src/bookkeep/shim_pid.c, line 94 at r2 (raw file):

    }
    assert(g_last_used_id < g_last_range->end);
    assert(g_last_range->end - g_last_range->start + 1 > g_last_range->taken_count);

Would be more natural to read if you flipped the sides (it's taken_count which we assert that is still below the expected limit).

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 8 at r2 (raw file):

/*
 * Implementation of `/proc/<remote-pid>`. Currently supports only `root`, `cwd` and `exe` symlinks,
 * does not support process listsing (you need to know the pid in advance) and does not do any

listsing -> listing

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 32 at r2 (raw file):

    struct shim_ipc_pid_retmeta* retmeta = NULL;
    int ret = ipc_pid_getmeta(pid, PID_META_CRED, &retmeta);
    if (ret < 0) {

One problem with this is that we silence potential errors (i.e. different than ENOENT), but to fix this we'd need to change the interface and add more checks to the callsites, so I'm not blocking. I unfortunately missed this problem in the pseudo PR.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 12 at r2 (raw file):

#include "shim_lock.h"
#include "shim_types.h"

What the difference between this file and shim_pid.c? Especially between the IDs stored in there, I miss a comment which would explain what are they.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 79 at r2 (raw file):

/* If a free range was found, sets `*start` and `*end` and returns `true`, if nothing was found
 * returns `false`. */

What range does it return? Maximum free range which is not in g_id_owners_tree? A single-item range? I'm really lost and have to reverse this from the implementation.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 104 at r2 (raw file):

        assert(next_id <= range->end);
        if (range->end == IDTYPE_MAX) {
            /* No ids available in range `[g_last_id + 1, IDTYPE_MAX]`. */

Shouldn't we wrap around here and start searching from 1?
Update: ah, I see that you do this in the callsite. Could you add a comment here?

[dimakuv]

Reviewed 23 of 24 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 39 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski and @mkow)

a discussion (no related file):
What will be the plan to add Sys-V semaphores and other shared resources back to Graphene? Previously they were also considered as IDs (and re-used the same code), but now your implementation looks very PID/TID centric.

UPDATE: After reading the implementation, it doesn't look PID/TID centric. So this could be re-used for Sys-V stuff (if we decide to do this).

---

a discussion (no related file):
We finally deleted shim_ipc_ranges.c. Graphene feels different now.

---

---

LibOS/shim/include/shim_ipc.h, line 172 at r2 (raw file):

 * \param end end of the ID range
 *
 * \p start and \p end must denote a full range.

What do you mean by this full range? Do you mean "inclusive"? In that case, please append (inclusive).

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

 *
 * This operation effectively splits an existing ID range. Each (if any) of the range parts must be
 * later on freed separately.

This is vague. What exactly happens if we have this:

• existing ID range is 1..10, belongs to ID pid1
• we call this function with id=5, new_owner=pid2

Do we have three ranges then? 1..4 -> pid1, 5 -> pid2, 6..10 -> pid1? And do we need to free each of these three ranges?

UPDATE: The answer is in change_id_owner(). My example is correct, we create three ranges if the ID is somewhere in the middle of an existing range. I suggest to add some comment or maybe just this example.

---

LibOS/shim/include/shim_ipc.h, line 193 at r2 (raw file):

 *
 * \param id id to find the owner of
 * \parami[out] owner contains vmid of the process owning \p id

What happens if there is no owner? Do we return some error code?

---

LibOS/shim/include/shim_ipc.h, line 254 at r2 (raw file):

} __attribute__((packed));

/* PID_RETSTATUS: return status of pid(s) */

What was this one used for? And why is it removed now?

---

LibOS/shim/include/shim_thread.h, line 144 at r2 (raw file):

/*!
 * \brief Allocates new ID

Do these new functions really belong to shim_thread.h? They are more than just for TID management; at some point they will also be used for other shared resources (I guess?). Maybe create a new header file for them or at least move them to shim_internal.h?

---

LibOS/shim/include/shim_thread.h, line 146 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

 * \param remove_from_owned if true, ID is removed from owned and locally tracked IDs

Hmm, I don't understand this semantic. Could you explain? If remove_from_owned is true, then this ID will be used somehow in a different process?

I also don't understand the semantics.

UPDATE: Ok, remove_from_owned == true for the case of spawning a new child process. In this case, the new ID is owned by the child process, not by this parent process. So the ID must be "hidden" from this process (= not in g_used_ranges_tree and not in g_last_range). Please add some comment like this.

---

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

        ret = ipc_change_id_owner(g_process.pid, g_self_vmid);
        if (ret < 0) {
            log_debug("shim_init: failed to change child process PID ownership: %d\n", ret);

Why debug, not error?

---

LibOS/shim/src/bookkeep/shim_pid.c, line 1 at r2 (raw file):

/* SPDX-License-Identifier: LGPL-3.0-or-later */

The name shim_pid.c is inconsistent with other names. Please change to shim_id.c.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 32 at r2 (raw file):

     * ```return a->start <= b->start;```
     * because overlapping ranges in one tree are disallowed, but it also enables easy lookups of
     * ranges overlapping any given point.

This is confusing. Why not just use a->start <= b->start and make sure that "any given point" always has b->start == b->end?

---

LibOS/shim/src/bookkeep/shim_pid.c, line 49 at r2 (raw file):

    if (!preaload_tid) {
        return 0;

Shouldn't you destroy the lock as part of cleanup?

---

LibOS/shim/src/bookkeep/shim_pid.c, line 54 at r2 (raw file):

    struct id_range* range = malloc(sizeof(*range));
    if (!range) {
        return -ENOMEM;

Shouldn't you destroy the lock as part of cleanup?

---

LibOS/shim/src/bookkeep/shim_pid.c, line 77 at r2 (raw file):

        IDTYPE start;
        IDTYPE end;
        int ret = ipc_alloc_id_range(&start, &end);

This should also be moved out of the lock.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 143 at r2 (raw file):

    lock(&g_ranges_lock);
    if (g_last_range && g_last_range->start <= id && id <= g_last_range->end) {
        assert(g_last_range->taken_count > 0);

Why can't this never happen? Isn't it possible that all IDs from g_last_range are released? I was expecting a clause like if (g_last_range->taken_count == 0) /*free it*/.

---

LibOS/shim/src/bookkeep/shim_thread.c, line 224 at r2 (raw file):

        return NULL;
    }

Can you add a comment that the caller is responsible for setting thread->tid?

---

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

    struct pseudo_node* ipc_thread_pid = pseudo_add_dir(root, /*name=*/NULL);
    ipc_thread_pid->name_exists = &proc_ipc_thread_pid_name_exists;
    /* Listing remote processes is not currently supported. */

Is this simply because you didn't want to deal with it in this PR? Or is there some design issue in supporting this?

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 211 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

start and end must denote an existing range, which means that if you change ownership of an ID, you need to release all parts separately (in case the range was split in multiple parts). This comes naturally with the current code.
If somebody has something against it, now is the time.
Maybe this requires more comments?

I already left a request to add a comment on this. Other than that, I'm fine.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 12 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What the difference between this file and shim_pid.c? Especially between the IDs stored in there, I miss a comment which would explain what are they.

Borys may explain better but:

• shim_pid.c (I suggest a rename to shim_id.c) is for local (this-process-internal) handling of ID ranges
• shim_ipc_id.c is for inter-process communication to handle ID ranges
  • most of this logic executes on the leader (main) Graphene process
  • rest of this logic is "I am asking the leader for ID management" on the follower processes

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 41 at r2 (raw file):

     * ranges overlapping any given point.
     */
    return a->start <= b->end;

ditto

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 252 at r2 (raw file):

}

int ipc_alloc_id_range(IDTYPE* start, IDTYPE* end) {

In all below functions you don't have any logging. Is this intentional? Maybe we want to add logging similar to other IPC places?

---

LibOS/shim/src/ipc/shim_ipc_worker.c, line 51 at r2 (raw file):

    [IPC_MSG_CHILDEXIT]         = ipc_cld_exit_callback,
    [IPC_MSG_NEW_ID_RANGE]      = ipc_alloc_id_range_callback,
    [IPC_MSG_FREE_ID_RANGE]     = ipc_release_id_range_callback,

I would prefer to have a one-to-one correspondence between names. So I suggest to rename IPC_MSG_ALLOC_ID_RANGE and IPC_MSG_RELEASE_ID_RANGE.

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

I don't know how to handle this better.

Why not just do ipc_release_id_range() here and ignore proper clean-up? Hm, or maybe it's equivalently dangerous situation (the code below this line may still assume that it is "known" to other processes, so still bad).

I also don't know how to handle it, so not blocking.

[mkow]

Reviewed 9 of 24 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 45 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski and @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We finally deleted shim_ipc_ranges.c. Graphene feels different now.

I agree :)

---

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 300 at r2 (raw file):

    };
    size_t msg_size = get_ipc_msg_size(sizeof(range));
    struct shim_ipc_msg* msg = __alloca(msg_size);

Sometimes you use alloca and sometimes malloc (e.g. in line 258 in this very file). Any rule behind this?

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 322 at r2 (raw file):

        return -ENOMEM;
    }
    init_ipc_msg(msg, IPC_MSG_FREE_ID_RANGE, msg_size);

Would be good if the function name matched the message name more closely ("free" vs "release").

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 330 at r2 (raw file):

}

int ipc_release_id_range_callback(IDTYPE src, void* data, uint64_t seq) {

All these callbacks are used only in the IPC leader, right? Maybe it would be a good idea to rename them to something like "ipc_<op_name>_leader_callback"?

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 377 at r2 (raw file):

    if (!g_process_ipc_ids.leader_vmid) {
        *owner = find_id_owner(id);
        return 0;

Shouldn't we return failure if owner == 0? The callsites seem to not check for this value.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 405 at r2 (raw file):

int ipc_get_id_owner_callback(IDTYPE src, void* data, uint64_t seq) {
    IDTYPE* id = data;
    IDTYPE owner = find_id_owner(*id);

Who's checking for failure? (i.e. owner == 0)

---

LibOS/shim/test/regression/multi_pthread.c, line 30 at r2 (raw file):

            if (x != 0) {
                printf("pthread_create: %d\n", x);
                ret = 1;

Shouldn't you break from the loops here? Otherwise we'll try to join this uninitialized thread structure.

[boryspoplawski]

Reviewable status: 15 of 24 files reviewed, 44 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Typo in the commit message: "proces"

TODO at next rebase

---

a discussion (no related file):

What will be the plan to add Sys-V semaphores and other shared resources back to Graphene?

I had no such plan unless users requested it.
But I also thought a bit about it :) More details below.

After reading the implementation, it doesn't look PID/TID centric. So this could be re-used for Sys-V stuff (if we decide to do this).

I would rather not go this way, it's designed to handle PID ranges, not SYSV-IPC IDs. There's a change we would be able to use the sync engine merged recently.
The problems with using these ID ranges for SYSV-IPC:

• we would mix PIDs with IPC IDs; not a real problem, but sounds weird,
• there ranges are given in batches, to decrease amount of IPC messages on thread creation; there is no such need for SYSV stuff,
• I do not think that IPC IDs needs to be sequential, but here PIDs are and it adds a some otherwise unnecessary complexity to the implementation.

---

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We finally deleted shim_ipc_ranges.c. Graphene feels different now.

Hopefully in a good way :)

---

---

LibOS/shim/include/shim_ipc.h, line 23 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd add "GET" to this one to make it less confusing

Done.

---

LibOS/shim/include/shim_ipc.h, line 159 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd start using the out_ prefix convention. Ditto for the whole PR.

Done, if I did not miss something.

---

LibOS/shim/include/shim_ipc.h, line 159 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think [out] should be after \param?

Oops.

---

LibOS/shim/include/shim_ipc.h, line 172 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What do you mean by this full range? Do you mean "inclusive"? In that case, please append (inclusive).

I meant it must be a range that was previously allocated as a whole, not part of it.
Example: /*snip*/
Update: this exactly the case you described in the comment below.

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is vague. What exactly happens if we have this:

• existing ID range is 1..10, belongs to ID pid1
• we call this function with id=5, new_owner=pid2

Do we have three ranges then? 1..4 -> pid1, 5 -> pid2, 6..10 -> pid1? And do we need to free each of these three ranges?

UPDATE: The answer is in change_id_owner(). My example is correct, we create three ranges if the ID is somewhere in the middle of an existing range. I suggest to add some comment or maybe just this example.

Added some comments, please let me know how to describe it better.

---

LibOS/shim/include/shim_ipc.h, line 193 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

parami

typo?

Done.

---

LibOS/shim/include/shim_ipc.h, line 193 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What happens if there is no owner? Do we return some error code?

Added a comment.
Generally 0 is considered an invalid ID in all of this code. Maybe that deserves a comment somewhere, but I do not really know where, since all of the code assumes that.
Also, ipc_* functions return errors only on real errors (like OOM or sending failure). It's responsibility of the caller to handle logical errors (like missing IDs - this function returns 0, in others it would be a implementation bug).

---

LibOS/shim/include/shim_ipc.h, line 254 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What was this one used for? And why is it removed now?

It was used by get_all_pid_status to get list of all pids (other fields were not used). Only usage of get_all_pid_status was to list /proc entries, which is currently disabled (we can add it later on if needed, but I doubt there will be any real usage except testing).

---

LibOS/shim/include/shim_thread.h, line 137 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

preaload

typo

Done.

---

LibOS/shim/include/shim_thread.h, line 144 at r2 (raw file):

Do these new functions really belong to shim_thread.h?

Yes, currently they allocate PID/TID explicitly.

t some point they will also be used for other shared resources (I guess?).

Probably not.

Maybe create a new header file for them or at least move them to shim_internal.h?

I didn't want to create another header for just 2 functions, since they are tightly couple with pid/tid atm.

---

LibOS/shim/include/shim_thread.h, line 146 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I also don't understand the semantics.

UPDATE: Ok, remove_from_owned == true for the case of spawning a new child process. In this case, the new ID is owned by the child process, not by this parent process. So the ID must be "hidden" from this process (= not in g_used_ranges_tree and not in g_last_range). Please add some comment like this.

If it's true, the ID is no longer on local IDs list, but the IPC leader still thinks it's owner by this process. The caller of this function can then call ipc_change_id_owner to pass this ID to another process. For details please check shim_clone.c.

---

LibOS/shim/include/shim_thread.h, line 152 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Does this release the ID to the leader, or only mark it as non-owned in current process?

The idea is that the caller should not have to worry about such details, but unfortunately I had to move ownership changing out of this function, so the caller needs to worry, but only just a bit.
The truth is: depends. IDs are release in batches just like they were allocated, so in most cases it only marks the ID as used locally and releases it together with others only as the whole ranges is free.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why debug, not error?

Because I thought the not-leaking-things-on-log_error PR will be merged before this, but seems it's not so obvious.
As a rule of thumb I assumed all dynamically build error messages leak stuff (even it these are seemingly unimportant things like error codes).

---

LibOS/shim/src/bookkeep/shim_pid.c, line 1 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The name shim_pid.c is inconsistent with other names. Please change to shim_id.c.

The idea was that this file deals with PIDs (/TIDs) and shim_ipc_id.c can handle arbitrary IDs (e.g. also SYSV IPC stuff). In the meantime I came to the conclusion that handling them together is not a good idea.
If you want to rename it anyway, let's do that on the final rebase.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 22 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't this also be IDTYPE? OTOH, I see that this is capped by MAX_RANGE_SIZE which is small, but I don't like such a convoluted assumption.

In theory IDTYPE count be signed. This field counts things and as such should be unsigned. Added an assert which should also server as a comment.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 32 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is confusing. Why not just use a->start <= b->start and make sure that "any given point" always has b->start == b->end?

This is a copy paste from ipc/shim_ipc_id.c and probably does not matter in this file.
Once these two file were merged together but it turned out the only similarity is this tree, so I've split them and just copy pasted the comparison function.
I see 3 options:

1. Change to a->start <= b->start
2. Remove the comment
3. Leave it as it is
4. Change the comment
   I don't like 1) because what we have currently just basically checks whether a and b overlap and I don't like 2) because a binary tree cannot hold arbitrarily overlapping ranges. Probably 4) would be the best.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 43 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (typo)

This must have been a lot of copy pasting ;)

---

LibOS/shim/src/bookkeep/shim_pid.c, line 49 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't you destroy the lock as part of cleanup?

Wait what? Why? This is return 0; it means the call was successful.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 54 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't you destroy the lock as part of cleanup?

I thought we have a convention not to bother with cleanup in shim init functions.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 70 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd feel better if allocations were outside the locked region if this won't make this function too complex.

See the comment below. I see no point in moving this malloc outside when we also have ipc_alloc_id_range in here. Also in most cases g_last_range will exist, so we would have to malloc and then immediately free for no real reason.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 77 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This should also be moved out of the lock.

It cannot be moved out of the lock because it prevents multiple threads from allocating a new range in parallel. Handing such cases would be a mess.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 86 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

So, ID == 0 is somehow a reserved/magic one? Or it's banned only to simplify the calculations below and allow them to safely use - 1 on it? (and to not have to return the ID using an out arg from here?)

It's considered an invalid ID, since it simplifies stuff in all layers and parts of this PR.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 94 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Would be more natural to read if you flipped the sides (it's taken_count which we assert that is still below the expected limit).

Done.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 143 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why can't this never happen? Isn't it possible that all IDs from g_last_range are released? I was expecting a clause like if (g_last_range->taken_count == 0) /*free it*/.

We just checked that id, which we are releasing right now is part of g_last_range which means that taken_count must be > 0 (since id is taken and in that range).

---

LibOS/shim/src/bookkeep/shim_thread.c, line 224 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can you add a comment that the caller is responsible for setting thread->tid?

Done (in shim_thread.h)

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is this simply because you didn't want to deal with it in this PR? Or is there some design issue in supporting this?

Both. Note that this feature is not useful outside of manually inspecting proc (debugging/testing only, I guess?) and tools like ps and pgrep, which does not work anyway (we do not support /proc/[pid]/stat{,us}).
The problem is that currently we have no way of obtaining all pids list, so either such support would have to be added (maybe using broadcast, maybe adding a dedicated list in IPC leader) or hacked in the current code (e.g. IPC worker in IPC leader has connections to all processes, maybe it could store their pids alongside). Since there is no clear indication that such feature would be useful, it's not present in this PR.

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 8 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

listsing -> listing

Done.

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 32 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

One problem with this is that we silence potential errors (i.e. different than ENOENT), but to fix this we'd need to change the interface and add more checks to the callsites, so I'm not blocking. I unfortunately missed this problem in the pseudo PR.

Yes, but this is completely unrelated to this PR. Added a comment tho.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 12 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Borys may explain better but:

• shim_pid.c (I suggest a rename to shim_id.c) is for local (this-process-internal) handling of ID ranges
• shim_ipc_id.c is for inter-process communication to handle ID ranges
  • most of this logic executes on the leader (main) Graphene process
  • rest of this logic is "I am asking the leader for ID management" on the follower processes

Just as @dimakuv explained with small difference that shim_pid.c currently handles just PIDs/TIDs (no other IDs are supported).
There is a small comment in the commit message; I've added some to the tops of those two files too.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 41 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

To be fair I got lost in all of this and have no idea what was the original reasoning. Maybe some intermediate version needed it...
Anyway, see the other comment with description of the current state and next steps.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 79 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What range does it return? Maximum free range which is not in g_id_owners_tree? A single-item range? I'm really lost and have to reverse this from the implementation.

It returns a range that is free. It could be a single item, it could be multiple items.
Added a comment describing only assumption that callers can make.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 104 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we wrap around here and start searching from 1?
Update: ah, I see that you do this in the callsite. Could you add a comment here?

The first version wrapped around here, but the code detecting when to stop a loop was ugly, so I decided to write it this way (which has only one caveat of possibly going through the whole tree twice, if all pids are taken, but such scenario is highly unlikely).

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 252 at r2 (raw file):

Is this intentional?

No, but I didn't have to debug anything, so did not add them ;)

Maybe we want to add logging similar to other IPC places?

Done.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 300 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Sometimes you use alloca and sometimes malloc (e.g. in line 258 in this very file). Any rule behind this?

Generally this is a work for another PR (to make some official rule and use it everywhere).
But yes, the rule I used (which might or might not be the one I'll propose later) is malloc prefered, but __alloca allowed if the struct is small (which I define as not having a VLA) and dealing with OOM errors is cumbersome (like in IPC callbacks).

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 322 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Would be good if the function name matched the message name more closely ("free" vs "release").

Done.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 330 at r2 (raw file):

All these callbacks are used only in the IPC leader, right?

Yes, they are only used in IPC leader.

Maybe it would be a good idea to rename them to something like "ipc_<op_name>_leader_callback"?

Maybe, maybe not, I have no preference. Which one would you prefer? cc @dimakuv

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 377 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we return failure if owner == 0? The callsites seem to not check for this value.

These were issues at callsites (because both of them were not much touched by me, at least yet). The interface documents that owner might be 0 (IDK if I added this now or before, too much changes).
Anyway, fixed at callsites.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 405 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Who's checking for failure? (i.e. owner == 0)

ditto

---

LibOS/shim/src/ipc/shim_ipc_worker.c, line 51 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would prefer to have a one-to-one correspondence between names. So I suggest to rename IPC_MSG_ALLOC_ID_RANGE and IPC_MSG_RELEASE_ID_RANGE.

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why not just do ipc_release_id_range() here and ignore proper clean-up? Hm, or maybe it's equivalently dangerous situation (the code below this line may still assume that it is "known" to other processes, so still bad).

I also don't know how to handle it, so not blocking.

release_id does all necessary cleanups, including ipc_release_id_range if needed. We cannot use ipc_release_id_range directly here. Even if we could, it would not change anything.

---

LibOS/shim/test/regression/multi_pthread.c, line 30 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't you break from the loops here? Otherwise we'll try to join this uninitialized thread structure.

I think the original idea of this code was to check how many threads we can create (no matter what errors occur), but you are right, this is wrong.

[mkow]

Reviewed 9 of 9 files at r3.
Reviewable status: all files reviewed, 24 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Added some comments, please let me know how to describe it better.

Much better now, thanks! One question thought, to the last point in the example: who's supposed to call which release func? I.e. which of these two processes? Both all of them? Or 1, 2, 1?

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

Because I thought the not-leaking-things-on-log_error PR will be merged before this, but seems it's not so obvious.
As a rule of thumb I assumed all dynamically build error messages leak stuff (even it these are seemingly unimportant things like error codes).

I think this one will be ready first. I can update this place afterwards.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 330 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

All these callbacks are used only in the IPC leader, right?

Yes, they are only used in IPC leader.

Maybe it would be a good idea to rename them to something like "ipc_<op_name>_leader_callback"?

Maybe, maybe not, I have no preference. Which one would you prefer? cc @dimakuv

I'd prefer to rename them, would be less confusing IMO. But let's wait for @dimakuv's opinion.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 348 at r3 (raw file):

%u, %u

I think %u..%u would look nicer in the logs, where you don't have names of these arguments and may not remember what they mean exactly (e.g. maybe it's start + size?)

[dimakuv]

Reviewed 9 of 9 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

What will be the plan to add Sys-V semaphores and other shared resources back to Graphene?

I had no such plan unless users requested it.
But I also thought a bit about it :) More details below.

After reading the implementation, it doesn't look PID/TID centric. So this could be re-used for Sys-V stuff (if we decide to do this).

I would rather not go this way, it's designed to handle PID ranges, not SYSV-IPC IDs. There's a change we would be able to use the sync engine merged recently.
The problems with using these ID ranges for SYSV-IPC:

• we would mix PIDs with IPC IDs; not a real problem, but sounds weird,
• there ranges are given in batches, to decrease amount of IPC messages on thread creation; there is no such need for SYSV stuff,
• I do not think that IPC IDs needs to be sequential, but here PIDs are and it adds a some otherwise unnecessary complexity to the implementation.

OK, I agree on all points. I was also thinking of using the sync engine from Pawel to implement SYS-V stuff (if at all).

---

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Much better now, thanks! One question thought, to the last point in the example: who's supposed to call which release func? I.e. which of these two processes? Both all of them? Or 1, 2, 1?

LGTM. As for Michal's question, each process must send a release-range message with its owned range (so 1,2,1) and the actual remove-from-tree/free happens in the leader process.

---

LibOS/shim/include/shim_thread.h, line 144 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Do these new functions really belong to shim_thread.h?

Yes, currently they allocate PID/TID explicitly.

t some point they will also be used for other shared resources (I guess?).

Probably not.

Maybe create a new header file for them or at least move them to shim_internal.h?

I didn't want to create another header for just 2 functions, since they are tightly couple with pid/tid atm.

OK, makes sense.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 1 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

The idea was that this file deals with PIDs (/TIDs) and shim_ipc_id.c can handle arbitrary IDs (e.g. also SYSV IPC stuff). In the meantime I came to the conclusion that handling them together is not a good idea.
If you want to rename it anyway, let's do that on the final rebase.

Yeah, then I would prefer shim_ipc_pid.c name.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 32 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This is a copy paste from ipc/shim_ipc_id.c and probably does not matter in this file.
Once these two file were merged together but it turned out the only similarity is this tree, so I've split them and just copy pasted the comparison function.
I see 3 options:

1. Change to a->start <= b->start
2. Remove the comment
3. Leave it as it is
4. Change the comment
   I don't like 1) because what we have currently just basically checks whether a and b overlap and I don't like 2) because a binary tree cannot hold arbitrarily overlapping ranges. Probably 4) would be the best.

I got confused by your options, but yes, please do option 4 then.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 49 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wait what? Why? This is return 0; it means the call was successful.

Yep, copy-paste error :)

---

LibOS/shim/src/bookkeep/shim_pid.c, line 54 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I thought we have a convention not to bother with cleanup in shim init functions.

OK.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 77 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

It cannot be moved out of the lock because it prevents multiple threads from allocating a new range in parallel. Handing such cases would be a mess.

OK. Doesn't look like a perf problem anyway.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 41 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

To be fair I got lost in all of this and have no idea what was the original reasoning. Maybe some intermediate version needed it...
Anyway, see the other comment with description of the current state and next steps.

OK, resolving this since it's not a big issue for me.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 330 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd prefer to rename them, would be less confusing IMO. But let's wait for @dimakuv's opinion.

I didn't have any issue with these names. So if you want a majority vote, then it's 2:1 for keeping as-is.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 348 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

%u, %u

I think %u..%u would look nicer in the logs, where you don't have names of these arguments and may not remember what they mean exactly (e.g. maybe it's start + size?)

+1 to Michal's suggestion. Same in all other places where you print a range.

[boryspoplawski]

Reviewable status: 22 of 24 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

LGTM. As for Michal's question, each process must send a release-range message with its owned range (so 1,2,1) and the actual remove-from-tree/free happens in the leader process.

It does not matter which process does it, but:

1. a single range must be freed by only one process (otherwise it would be a double free!, well, it wold be caught and IPC leader would die loudly)
2. it really does not make any sense to do it from a process other than the current owner of the range.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 1 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yeah, then I would prefer shim_ipc_pid.c name.

TODO: at final rebase.

---

LibOS/shim/src/bookkeep/shim_pid.c, line 32 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I got confused by your options, but yes, please do option 4 then.

After thinking about this a bit: the current comment is correct and a->start <= b->start does not allow for overlapping range queries, even if one of them is a point.
Example:
Range R = [1..10]
Point p = [5..5]
R <= p is true (because 1 <= 5), but p <= R is false (because 5 <= 1 is false). If we did a lower_bound on a tree containing only R with p as argument, it would return nothing, where as in the current (correct) version, it will return R.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 330 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I didn't have any issue with these names. So if you want a majority vote, then it's 2:1 for keeping as-is.

Strictly speaking it's 1:1 as I do not prefer any of these options :)
But keeping the current version is easier, so...

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 348 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1 to Michal's suggestion. Same in all other places where you print a range.

Done.

[boryspoplawski]

Reviewable status: 22 of 24 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/src/ipc/shim_ipc_signal.c, line 33 at r4 (raw file):

            /* No process owns `dest_pid`... */
            if (is_zombie_process(dest_pid)) {
                /* ... but it's a zombie! */

This made me realize that the current semantics are a bit off: a child releases it's pid on exit, yet it might still be a zombie in the parent. In a unlikely case that the parents does not wait for the child for a long time and pids overlap we could have a nasty conflict.
The problem is that solving this is hard: we would need to make the parent own the pid of the child, but that would require "reparenting" in case the parent dies but the child is still alive. Such solution would also have some nasty consequences: Graphene pid 1 (which I guess would become the new parent) might not be expecting to have more children than it spawned (normal apps do not expect that, as init process is pretty special).
Generally speaking: there is a problem and there is no good solution to it.

[mkow]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

It does not matter which process does it, but:

1. a single range must be freed by only one process (otherwise it would be a double free!, well, it wold be caught and IPC leader would die loudly)
2. it really does not make any sense to do it from a process other than the current owner of the range.

Could you explain this in the comment? It's really not obvious to me which options are valid and which are currently used / make sense to be used.

---

LibOS/shim/src/ipc/shim_ipc_signal.c, line 33 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This made me realize that the current semantics are a bit off: a child releases it's pid on exit, yet it might still be a zombie in the parent. In a unlikely case that the parents does not wait for the child for a long time and pids overlap we could have a nasty conflict.
The problem is that solving this is hard: we would need to make the parent own the pid of the child, but that would require "reparenting" in case the parent dies but the child is still alive. Such solution would also have some nasty consequences: Graphene pid 1 (which I guess would become the new parent) might not be expecting to have more children than it spawned (normal apps do not expect that, as init process is pretty special).
Generally speaking: there is a problem and there is no good solution to it.

I think this comment deserves landing in the code ;)

[boryspoplawski]

Reviewable status: 22 of 24 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you explain this in the comment? It's really not obvious to me which options are valid and which are currently used / make sense to be used.

Sure I could, but I'm not sure what to put where. Could you specify what comment you would like to have and where?
To me it's quite obvious that releasing/freeing the same range from 2 processes is wrong and other than that there are no real requirements. We do it in a certain way atm (owner releases it's own ranges), but it's not a requirement (parent or IPC leader could release them as well). As long as everything is released once and only once, it's all good.
But I'm the author of this code - I have a biased opinion - so it would be better if you described what requires a comment and where.

---

LibOS/shim/src/ipc/shim_ipc_signal.c, line 33 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this comment deserves landing in the code ;)

Done in shim_exit.c

[mkow]

Reviewed 2 of 2 files at r5.
Reviewable status: all 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Sure I could, but I'm not sure what to put where. Could you specify what comment you would like to have and where?
To me it's quite obvious that releasing/freeing the same range from 2 processes is wrong and other than that there are no real requirements. We do it in a certain way atm (owner releases it's own ranges), but it's not a requirement (parent or IPC leader could release them as well). As long as everything is released once and only once, it's all good.
But I'm the author of this code - I have a biased opinion - so it would be better if you described what requires a comment and where.

So, for me this wasn't clear if there's any requirement about such things or not. So, I'd just say here that technically any process is allowed to free any range, but in current implementation processes always free only the ranges they own. Ad where: just right below this example maybe?

[boryspoplawski]

Jenkins, retest Jenkins-Debug-20.04 please (fsync02 from LTP timedout, known issue)

[boryspoplawski]

Reviewable status: 23 of 24 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

So, for me this wasn't clear if there's any requirement about such things or not. So, I'd just say here that technically any process is allowed to free any range, but in current implementation processes always free only the ranges they own. Ad where: just right below this example maybe?

Added, please check.

[mkow]

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 184 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Added, please check.

Looks good, thanks!

[dimakuv]

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

[boryspoplawski]

Reviewable status: 21 of 24 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: ITL) (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

TODO at next rebase

Done.

---

---

LibOS/shim/src/bookkeep/shim_pid.c, line 1 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

TODO: at final rebase.

Done.

---

LibOS/shim/src/ipc/shim_ipc_id.c, line 56 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This is intended be removed before merging. It allows for some testing of the implementation: just add a couple of ranges with owner 0 (invalid) and change g_last_id.

Done.

[mkow]

Reviewed 3 of 3 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)

[dimakuv]

Reviewed 2 of 3 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved