aya: Implement dedicated map pinning #783

astoycos · 2023-09-08T19:16:27Z

Align with libbpf's LIBBPF_PIN_BY_NAME map flag behavior. More specifically if map_pin_path is not provided but a program has a map defined with the flag, pin it in /sys/fs/bpf otherwise pin it in the path provided by map_pin_path(P).

Additionally, allow users to manually manage map pinning much like we do already for Programs, see commit messages for more details.

TODO support the pinning api for PerfEventArray's and AsyncPerfEventArray's

This change is

netlify · 2023-09-08T19:16:32Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`8203914`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/652586c00a7cd500081cbf7b
😎 Deploy Preview	https://deploy-preview-783--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mergify · 2023-09-08T19:43:15Z

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

aya/src/bpf.rs

test/integration-test/src/tests/rbpf.rs

test/integration-test/src/tests/load.rs

aya/src/maps/mod.rs

aya/src/programs/mod.rs

mergify · 2023-09-11T15:43:06Z

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

tamird

Did you forget to push?

aya/src/util.rs

test/integration-test/src/tests/rbpf.rs

astoycos · 2023-09-13T19:05:25Z

Pushed in a new commit @tamird so it's easier to check diff, I'll squash everything whenever you approve it

astoycos · 2023-09-13T19:05:53Z

Sorry usually I respond to comments as I fix things and then resolve them / re-request review after I push the changes.

astoycos · 2023-09-13T19:47:37Z

I'll bless the public API on squash 👍

tamird

Sorry usually I respond to comments as I fix things and then resolve them / re-request review after I push the changes.

You can "start a review" instead of publishing the comments as you go. That would be friendlier to reviewers.

aya/src/maps/mod.rs

aya/src/pin.rs

aya/src/util.rs

test/integration-test/src/tests/load.rs

test/integration-test/src/tests/rbpf.rs

aya/src/util.rs

tamird

Moving my review to Reviewable because GitHub reviews are so painful :(

Reviewed 13 of 19 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @alessandrod, @astoycos, and @dave-tucker)

aya/src/maps/mod.rs line 519 at r2 (raw file):

    pub fn create(
        obj: obj::Map,
        name: &str,

we should maybe take a String here so that the caller can control allocation.

aya/src/maps/mod.rs line 554 at r2 (raw file):

        path: P,
        obj: obj::Map,
        name: &str,

ditto

aya/src/maps/mod.rs line 654 at r2 (raw file):

        if paths.contains(&path.as_ref().to_path_buf()) {
            return Err(PinError::AlreadyPinned {
                name: name.to_string(),

nit: .clone() because the thing is already a String

aya/src/maps/mod.rs line 693 at r2 (raw file):

            let (paths, errors) = errors.into_iter().unzip();
            return Err(PinError::UnpinError {
                name: name.to_string(),

.clone

aya/src/maps/mod.rs

mergify · 2023-09-18T16:51:16Z

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

aya/src/maps/mod.rs

astoycos · 2023-09-19T13:13:57Z

Waiting for #790 to merge then I'll build back on top of it

tamird

Reviewed 2 of 19 files at r5, 17 of 17 files at r6, all commit messages.
Dismissed @dave-tucker from 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod, @astoycos, and @dave-tucker)

test/integration-test/src/tests/load.rs line 80 at r3 (raw file):

Previously, astoycos (Andrew Stoycos) wrote…

Join is a method on path but it turns it into an owned PathBuf https://doc.rust-lang.org/std/path/struct.Path.html#method.join

Then we get a bunch of "borrow of moved value" errors

using an &Path which is coerced into an &PathBuf on .join fixes this

Ah I missed the .join.

test/integration-test/src/tests/rbpf.rs line 64 at r6 (raw file):

            "map_2" => 1,
            "map_pin_by_name" => 2,
            _ => panic!("Unexpected map name: {}", name),

Nit: s/_/name/ to clarify that it's not discarded

mergify · 2023-09-25T01:24:46Z

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

astoycos

Reviewable status: 17 of 26 files reviewed, all discussions resolved (waiting on @alessandrod and @tamird)

test/integration-test/src/tests/rbpf.rs line 64 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Nit: s/_/name/ to clarify that it's not discarded

Done

astoycos · 2023-09-25T12:39:22Z

@alessandrod Just need a final pass on the API from you please 👼

tamird

Reviewed 9 of 9 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)

test/integration-test/src/tests/rbpf.rs line 64 at r6 (raw file):

Previously, astoycos (Andrew Stoycos) wrote…

Done

Sorry I meant replace the underscore with "name"

astoycos

Reviewable status: 23 of 26 files reviewed, all discussions resolved (waiting on @alessandrod and @tamird)

test/integration-test/src/tests/rbpf.rs line 64 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Sorry I meant replace the underscore with "name"

My bad fixed!

tamird

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)

astoycos · 2023-10-03T14:34:20Z

Poke @alessandrod For a quick look, TIA!

alessandrod

Just a few nits!

aya/src/maps/mod.rs

alessandrod · 2023-10-06T00:32:54Z

aya/src/maps/mod.rs

 }

+// Implements map pinning for different map implementations
+// TODO add support for PerfEventArrays and AsyncPerfEventArrays


Why this todo? What's missing to implement it?

A PerfEventArray keeps an Arc<MapData> instead of just MapData so we would need to write an explicit pin method for it rather than express it with the macro.

I can do that here if you want, or in a follow up!

I am not a fan of follow ups, because some of them tend to never happen. That said if landing this as is unblocks you I'm happy to make an exception 😋

aya/src/maps/mod.rs

aya/src/bpf.rs

Aligns with libbpf for the special LIBBPF_PIN_BY_NAME map flag. Specifically if the flag is provided without a pin path default to "/sys/fs/bpf". Signed-off-by: astoycos <astoycos@redhat.com>

aya/src/maps/mod.rs

tamird

Reviewed 10 of 10 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @astoycos)

tamird

Dismissed @alessandrod from 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @astoycos)

aya/src/maps/mod.rs line 358 at r10 (raw file):

            {
                    /// Pins the map to a BPF filesystem.

here too

- Adds new `maps_mut()` API to the BpfManager to allow us to iterate though and pin all of maps at the same time. - Adds new pin(Path)/unpin(Path) api to Maps so they can be generically pinned AFTER load. - Adds macro for pinning explicit map types in aya. Convert all explicit map types "inner" field to be pub crate in order to facilitate this. Signed-off-by: astoycos <astoycos@redhat.com>

Add coverage to the new public api's for map pinning (pin and unpin) which can be called on the generic aya::Map type OR explit map types. Additionally add coverage for the new libbpf LIBBPF_PIN_BY_NAME behavior. Signed-off-by: astoycos <astoycos@redhat.com>

astoycos · 2023-10-10T17:15:55Z

Dismissed @alessandrod from 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @astoycos)

aya/src/maps/mod.rs line 358 at r10 (raw file):
            {
                    /// Pins the map to a BPF filesystem.
here too

Done!

tamird

Reviewed 21 of 21 files at r12, all commit messages.
Dismissed @alessandrod from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @astoycos)

mergify bot added aya This is about aya (userspace) test A PR that improves test cases or CI labels Sep 8, 2023

astoycos force-pushed the map_pin2 branch 2 times, most recently from c58e3ec to d9cf8e1 Compare September 8, 2023 19:42

mergify bot requested a review from alessandrod September 8, 2023 19:43

mergify bot added the api/needs-review Makes an API change that needs review label Sep 8, 2023

astoycos force-pushed the map_pin2 branch 2 times, most recently from 4711618 to 1ae55ac Compare September 11, 2023 14:41

tamird reviewed Sep 11, 2023

View reviewed changes

dave-tucker reviewed Sep 11, 2023

View reviewed changes

aya/src/programs/mod.rs Outdated Show resolved Hide resolved

mergify bot added the needs-rebase label Sep 11, 2023

tamird reviewed Sep 13, 2023

View reviewed changes

aya/src/util.rs Outdated Show resolved Hide resolved

test/integration-test/src/tests/rbpf.rs Outdated Show resolved Hide resolved

astoycos force-pushed the map_pin2 branch from 1ae55ac to 9b5fce9 Compare September 13, 2023 19:03

mergify bot removed the needs-rebase label Sep 13, 2023

astoycos requested a review from tamird September 13, 2023 19:06

tamird requested changes Sep 13, 2023

View reviewed changes

tamird requested a review from dave-tucker September 13, 2023 21:08

tamird reviewed Sep 13, 2023

View reviewed changes

astoycos mentioned this pull request Sep 18, 2023

aya: Remove MapData::pinned #789

Closed

dave-tucker reviewed Sep 18, 2023

View reviewed changes

aya/src/maps/mod.rs Outdated Show resolved Hide resolved

mergify bot added the needs-rebase label Sep 18, 2023

dave-tucker requested changes Sep 19, 2023

View reviewed changes

aya/src/maps/mod.rs Outdated Show resolved Hide resolved

aya/src/maps/mod.rs Outdated Show resolved Hide resolved

tamird approved these changes Sep 23, 2023

View reviewed changes

mergify bot added the needs-rebase label Sep 25, 2023

dave-tucker approved these changes Sep 25, 2023

View reviewed changes

astoycos force-pushed the map_pin2 branch from 9d06a81 to 495a823 Compare September 25, 2023 12:38

astoycos commented Sep 25, 2023

View reviewed changes

astoycos requested a review from tamird September 25, 2023 12:38

mergify bot removed the needs-rebase label Sep 25, 2023

tamird approved these changes Sep 25, 2023

View reviewed changes

astoycos force-pushed the map_pin2 branch from 495a823 to 61fc471 Compare September 25, 2023 14:35

astoycos commented Sep 25, 2023

View reviewed changes

tamird approved these changes Sep 25, 2023

View reviewed changes

astoycos requested review from alessandrod and removed request for alessandrod September 29, 2023 15:45

astoycos mentioned this pull request Oct 2, 2023

aya: fix create pinned map path. #803

Closed

alessandrod reviewed Oct 6, 2023

View reviewed changes

aya/maps: fix libbpf_pin_by_name

0bf97eb

Aligns with libbpf for the special LIBBPF_PIN_BY_NAME map flag. Specifically if the flag is provided without a pin path default to "/sys/fs/bpf". Signed-off-by: astoycos <astoycos@redhat.com>

astoycos force-pushed the map_pin2 branch from 61fc471 to a1a188c Compare October 10, 2023 15:42

alessandrod reviewed Oct 10, 2023

View reviewed changes

aya/src/maps/mod.rs Outdated Show resolved Hide resolved

tamird approved these changes Oct 10, 2023

View reviewed changes

tamird requested changes Oct 10, 2023

View reviewed changes

astoycos added 2 commits October 10, 2023 13:11

astoycos force-pushed the map_pin2 branch from a1a188c to 8203914 Compare October 10, 2023 17:15

tamird reviewed Oct 10, 2023

View reviewed changes

astoycos merged commit ef27bce into aya-rs:main Oct 10, 2023
21 checks passed

astoycos mentioned this pull request Oct 20, 2023

aya/maps: pin for (async)perf_event_array #822

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aya: Implement dedicated map pinning #783

aya: Implement dedicated map pinning #783

astoycos commented Sep 8, 2023 •

edited

Loading

netlify bot commented Sep 8, 2023 •

edited

Loading

mergify bot commented Sep 8, 2023

mergify bot commented Sep 11, 2023

tamird left a comment

astoycos commented Sep 13, 2023

astoycos commented Sep 13, 2023 •

edited

Loading

astoycos commented Sep 13, 2023

tamird left a comment

tamird left a comment

mergify bot commented Sep 18, 2023

astoycos commented Sep 19, 2023

tamird left a comment

mergify bot commented Sep 25, 2023

astoycos left a comment

astoycos commented Sep 25, 2023

tamird left a comment

astoycos left a comment

tamird left a comment

astoycos commented Oct 3, 2023

alessandrod left a comment

alessandrod Oct 6, 2023

astoycos Oct 10, 2023 •

edited

Loading

alessandrod Oct 10, 2023

tamird left a comment

tamird left a comment

astoycos commented Oct 10, 2023

tamird left a comment

aya: Implement dedicated map pinning #783

aya: Implement dedicated map pinning #783

Conversation

astoycos commented Sep 8, 2023 • edited Loading

netlify bot commented Sep 8, 2023 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

mergify bot commented Sep 8, 2023

mergify bot commented Sep 11, 2023

tamird left a comment

Choose a reason for hiding this comment

astoycos commented Sep 13, 2023

astoycos commented Sep 13, 2023 • edited Loading

astoycos commented Sep 13, 2023

tamird left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 18, 2023

astoycos commented Sep 19, 2023

tamird left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 25, 2023

astoycos left a comment

Choose a reason for hiding this comment

astoycos commented Sep 25, 2023

tamird left a comment

Choose a reason for hiding this comment

astoycos left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

astoycos commented Oct 3, 2023

alessandrod left a comment

Choose a reason for hiding this comment

alessandrod Oct 6, 2023

Choose a reason for hiding this comment

astoycos Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

alessandrod Oct 10, 2023

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

astoycos commented Oct 10, 2023

tamird left a comment

Choose a reason for hiding this comment

astoycos commented Sep 8, 2023 •

edited

Loading

netlify bot commented Sep 8, 2023 •

edited

Loading

astoycos commented Sep 13, 2023 •

edited

Loading

astoycos Oct 10, 2023 •

edited

Loading