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

aya: Allows sharing Map between different eBPFs #540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marysaka
Copy link
Contributor

@marysaka marysaka commented Mar 10, 2023

Allows to share maps between eBPFs without relying on pinned maps.

@netlify
Copy link

netlify bot commented Mar 10, 2023

Deploy Preview for aya-rs-docs failed.

Built without sensitive environment variables

Name Link
🔨 Latest commit a04d836
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64afc6a17b5fce0007b08821

@alessandrod
Copy link
Collaborator

I think that conceptually we want this API, but not in the current form.

My first thought is that "shared" isn't the best name: maybe the map isn't shared, I just happen to have it loaded from a previous operation or something, or maybe I created it in some special way with custom flags etc. Maybe BpfLoader::map("name", map)? Not sure.

The bigger thing is that I'm not sure we should be exposing MapData at all. Is there a reason why it's public? Seems like it shouldn't be public to me.

Is there a reason why we can't pass &Map to the loader (we don't even need the map to be owned do we)? Then the loader would copy with some internal function, not Clone, as I don't think we have a reason to encourage people to clone maps even if technically they're a fd.

@dave-tucker
Copy link
Member

Is there a reason why we can't pass &Map to the loader (we don't even need the map to be owned do we)? Then the loader would copy with some internal function, not Clone, as I don't think we have a reason to encourage people to clone maps even if technically they're a fd.

Yep that makes sense.
We already have the first part of this with https://docs.aya-rs.dev/user/aya/struct.bpf#method.take_map which gets you an owned map from a Bpf.

So BpfLoader::with_maps() would make sense - would take a hashmap of name/&Map pairs.
We'd just need to make sure they only get used for relocations though, and we didn't accidentally make an Owned version again where it would be detach on drop.

@alessandrod
Copy link
Collaborator

Is there a reason why we can't pass &Map to the loader (we don't even need the map to be owned do we)? Then the loader would copy with some internal function, not Clone, as I don't think we have a reason to encourage people to clone maps even if technically they're a fd.

Yep that makes sense. We already have the first part of this with https://docs.aya-rs.dev/user/aya/struct.bpf#method.take_map which gets you an owned map from a Bpf.

So BpfLoader::with_maps() would make sense - would take a hashmap of name/&Map pairs. We'd just need to make sure they only get used for relocations though, and we didn't accidentally make an Owned version again where it would be detach on drop.

I don't think it would take a hashmap - say I have only 1 map, why would I be forced to create a hashmap? set_global also doesn't work with hashmaps, you do set_global("Foo", v).set_global("BAR", t) and I think that's the better API.

I'm also not sure wrt ownership. I was making your opposite assumption: you give the loader the map and you transfer ownership to the loader. It's not clear to me why you'd want to retain ownership and it complicates the implementation of Bpf (one more lifetime to track).

@marysaka marysaka force-pushed the feature/shared_map branch 4 times, most recently from fc52c6a to a0613a3 Compare March 30, 2023 09:50
@marysaka marysaka changed the title aya: Allows sharing MapData between different eBPFs aya: Allows sharing Map between different eBPFs Mar 30, 2023
@dave-tucker
Copy link
Member

I'm also not sure wrt ownership. I was making your opposite assumption: you give the loader the map and you transfer ownership to the loader. It's not clear to me why you'd want to retain ownership and it complicates the implementation of Bpf (one more lifetime to track).

Ok so here's the deal, you can get a map by:

  1. Creating one from userspace (with the API I added for LPM Trie)
  2. Having Bpf/BpfLoader create one for you, and taking ownership of it with take_map
  3. Opening a map from a pinned location on bpffs

To use that map again:

  1. You own the map, so you could pass it to BpfLoader
  2. You can take_map to get ownership of the map
  3. There is already a BpfLoader::map_pin_path API for this, but you could also use Map::from_pin

In all of those cases, you OWN the map, so this API, IMHO, would want a Borrow. Reason being something like this:

let logs_map = PerfEventArray::new(); // just assume this is a thing for now....
let first_prog = BpfLoader::new().map("AYA_LOG",  logs_map).load([]);
let second_prog = BpfLoader::new().map("AYA_LOG", logs_map).load([]);

Essentially I create a map I want shared across multiple programs...
Doing this Bpf taking ownership becomes painful - what if first_prog and second_prog are loaded in different threads? and I have a third thread that's reading data from the map?

Allows to share maps between eBPFs without relying on pinned maps.
@mergify mergify bot added the aya This is about aya (userspace) label Sep 14, 2023
@mergify
Copy link

mergify bot commented Sep 14, 2023

@marysaka, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Sep 14, 2023
Copy link

mergify bot commented Feb 6, 2024

@marysaka, this pull request is now in conflict and requires a rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace) needs-rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants