Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Update RingBufs API - allow referring individual maps #318

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

Conversation

nbaksalyar
Copy link
Contributor

@nbaksalyar nbaksalyar commented Apr 14, 2022

As was discussed in #251, the current ringbufs API has a disadvantage: in order to handle different maps in different async tasks, a user needs to redirect events to e.g. mpsc queues to handle them separately.

This change simplifies the API and implementation by allowing to refer to BPF maps individually through a corresponding key in a HashMap.

cc @yobiscus

This commit introduces a change to the way RingBuf maps are handled.

Previously, they were coalesced with PerfEventArray maps so if a user
wanted to handle different maps in different async tasks, they needed
to redirect events to e.g. mpsc queues to handle them separately.

This change simplifies the API and implementation by allowing to refer
to BPF maps individually through a corresponding key in a `HashMap`.

Signed-off-by: Nikita Baksalyar <[email protected]>
/// }
/// # };
/// ```
pub ringbufs: HashMap<String, RingBufMessageStream>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a reservation about how this is exposed in the API: there's a discrepancy in how ringbuf and perfmap events are handled.

I think it would make sense to use the same approach for PerfMaps too? This would be a breaking change though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our agreement with @rhdxmr we are going to bump major version for the next release due to the change in default LLVM. The time to break APIs is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! In this case, I'd advocate for adding a changelog - created an issue #319

@rsdy
Copy link
Collaborator

rsdy commented May 1, 2022

@nbaksalyar What's your plan here? Do you want to get this PR merged, or you'd rather amend this PR with the API changes you mentioned above?

@nbaksalyar
Copy link
Contributor Author

What's your plan here? Do you want to get this PR merged, or you'd rather amend this PR with the API changes you mentioned above?

Yes, I think it's better to address these within this PR because the API will change anyway. It is a bit non-trivial though. I'm trying to figure out how to make it universal and not a chore to use. Most likely it'll require using boxed traits or enums for different map types. I'll prioritise finishing this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants