Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map segments by name instead of access permissions, and allow publishers write access to multiple #2128

Open
gpalmer-latai opened this issue Dec 7, 2023 · 11 comments
Assignees
Labels
enhancement New feature

Comments

@gpalmer-latai
Copy link
Contributor

gpalmer-latai commented Dec 7, 2023

Brief feature description

Shared memory segments are currently mapped strictly by access controls. It would be beneficial to map instead by name or some other key so that processes aren't limited to writing to a single segment and we could better split up communication channels across multiple segments.

Detailed information

Currently, it is possible to creating multiple shared memory segments with different access permissions, as described in the configuration guide.

However, not only do the segments allow one to specify access controls, but they create really strong couplings based on those access controls. The implications include:

  • The name of shared memory segments is automatically deduced as the name of the cgroup that has write access to it
  • The above means that there can only be one segment that any given cgroup has write access to
  • Publishers are allowed write access to only one segment, which is deduced as the segment that matches the cgroup of the process the publisher is contained in.
  • It is therefore not possible to have a single publisher access multiple segments (This makes sense - it would be strange to do so).
  • It is also not possible to have multiple publishers in the same process which access different segments

The last point is of particular interest to me because I can consider a couple notable reasons why one would want to be able to create many shared memory segments and be able to access them from within the same process:

  • Splitting communication channels across separate shared memory segments ensures better Freedom From Interference because should a bad publisher or subscriber access data past the end of a memory pool, it will usually result in a segfault rather than simply a corrupted read/write of another memory pool.
  • We may wish to make use of the MemoryInfo member of a segment config (currently just defaulted) to differentiate segments based on memory type. This would allow a simpler, cleaner solution to the issue described in Expose the MemPoolInfo of matching memory pools to typed endpoints #2092 because we could specify that a segment is intended to support CUDA_PINNED_HOST memory, and then each publisher / subscriber that needs access to such memory would identify that segment and pin the memory of only that segment.

Possible Obstacles

I've found a curious comment in the code:

a user is allowed to be only in one writer group, as we currently only support one memory manager per process

I have not dug around enough in the code to understand why only one memory manager can be supported per process and what it might take to support more. I'd appreciate more context on this statement if anyone knows more.

From my perhaps naive standpoint - I would expect that the proper restriction be that we only have one memory manager per publisher, but that within the same process we may have multiple publishers writing to possibly different segments. For example - one publisher publishing images with the help of some GPU acceleration taking advantage of the pinned host memory, and another publisher publishing some small metrics data that does not need pinned host memory, so it uses a different memory segment.

Possible Implementation

I am imagining extending the Roudi config so that it could conceivably look like

[general]
version = 2

[[segment]]
name = "foobar"
writer = "foo"
reader = "bar"
memoryType = 0

[[segment.mempool]]
size = 32
count = 10000

[[segment]]
name = "pinned_foobar"
writer = "foo"
reader = "bar"
memoryType = 1

[[segment.mempool]]
size = 12000000
count = 10

And then when creating a publisher one can optionally specify the name of a segment:

BasePublisher(const capro::ServiceDescription& service, const PublisherOptions& publisherOptions, optional<IdString_t> shmSegmentName = {});

such that the name will be used directly to map the shared memory region rather than the access group of current process, as is currently done.

For backwards compatibility then, we could simply make the name default to user of the current process.

@gpalmer-latai
Copy link
Contributor Author

@elBoberido Because of the additional FFI benefits I'd be inclined to pursue this solution more over #2092 if it is deemed feasible. I'm also cautiously optimistic that it is a better fit with the current architecture, since segments already have the notion of MemoryInfo.

@elBoberido
Copy link
Member

A few year ago when we created the design for mixed criticality we also though about a more fine grained approach but dismissed it. I don't remember anymore why but in the meantime requirements also changed and maybe we need to rethink how to approach the problem. With the iceoryx Rust implementation we have this more fine grained access and it should be possible to port some of the changes to the C++ implementation.

Saying that, maybe we need to have a deep dive on the Rust design for the memory allocation. Could be done as special developer meetup. @elfenpiff what do you think?

I agree that this solution would be better than just exposing the MemPoolInfo.

Regarding the comment in the code. Since there is only one runtime and the publisher ctor does not allow to specify which segment to use, the only option was to restrict the writer segment to one per process. This is not a technical limitation though, just a design decision at that time. It wouldn't be a big problem to change it. We just need to adjust the API ... or even better, create an alternative API which also takes care of other things like having multiple runtimes in one process.

I like the idea but we should check with elfenpiff what he has for the Rust implementation.

@gpalmer-latai
Copy link
Contributor Author

gpalmer-latai commented Dec 9, 2023

Saying that, maybe we need to have a deep dive on the Rust design for the memory allocation.

I'd be thrilled to do this 🙂

FYI I've been working on a proof of concept. So far I've gotten to the point where I've successfully configured segments to be mapped by name instead of writer group, and I have a publisher example publishing onto three different segments.

I now need to look into the subscriber side of things to figure out what implications this will have. In theory nothing needs to be done there, but I might also want to support configuring subscribers to only listen on certain segments. This probably would make it easier to design a solution to only pin the segments we want to on the subscriber side.

@gpalmer-latai
Copy link
Contributor Author

I'm thinking something along the lines of an optional "filter" whereby a user can provide an optional list of shared memory segment names and the subscriber only maps those shared memory segments (assuming it also has read access, otherwise an error shall occur). The default would be the current behavior of listening on all segments where the subscriber has read access.

@elBoberido
Copy link
Member

What do you think of meetup on Tuesday 19th 5pm CET. This week we are kind of busy with the preparation for the initial Rust release.

Everybody is welcome to join but I would rather keep the round smaller so I don't announce the meeting on gitter. People interested in the topic should notice the meeting from the comments in this issue.

@gpalmer-latai
Copy link
Contributor Author

That time should work great for me 🙂

@elBoberido
Copy link
Member

Was a great talk like always. A pity @elfenpiff couldn't make it. Looking forward to a small design document and patches :)

@gpalmer-latai
Copy link
Contributor Author

Was a pleasure for me as well.

Regarding the design document and patches -

  • does it make sense to have multiple separate PRs referencing the same issue,
  • or do I create new issues
  • or do I have multiple commits in a single PR?

@elBoberido
Copy link
Member

Regarding the design document and patches -

* does it make sense to have multiple separate PRs referencing the same issue,

Definitely. The smaller the PRs the easier it is to review them.

* or do I create new issues

If it is something that might be a feature of its own but is a requirement to create this feature then it might make sense to have a new issue. Quite often we just have a list of task required to implement the feature in one issue and have multiple PRs. There can also be multiple PRs per task.

* or do I have multiple commits in a single PR?

You can have multiple commits in a PR and it is also appreciated to not squash commits once the PR is opened to make it easier to track the changes.

gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Dec 20, 2023
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Dec 20, 2023
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Dec 20, 2023
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Dec 20, 2023
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 9, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 9, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 9, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 9, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 10, 2024
…ure and allow multiple write-access segments to be mapped
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 10, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 10, 2024
…ure and allow multiple write-access segments to be mapped
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 10, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 11, 2024
…ure and allow multiple write-access segments to be mapped
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 11, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 11, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 11, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 12, 2024
…ure and allow multiple write-access segments to be mapped
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 12, 2024
…ure and allow multiple write-access segments to be mapped
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 12, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 16, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 16, 2024
@mossmaurice mossmaurice added the enhancement New feature label Jan 22, 2024
gpalmer-latai added a commit to gpalmer-latai/iceoryx that referenced this issue Jan 22, 2024
@gpalmer-latai
Copy link
Contributor Author

mossmaurice assigned gpalmer-latai yesterday

FYI I haven't forgotten about this. I've just been a bit bogged down with other matters lately.

@elBoberido
Copy link
Member

@gpalmer-latai no problem. It is also not inconvenient for me as I am having a lot of fun with the German bureaucracy because of the company setup :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

No branches or pull requests

3 participants