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: add guardrails for valid combinations of perf_event type and config fields #1038

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

Conversation

tyrone-wu
Copy link
Contributor

@tyrone-wu tyrone-wu commented Sep 25, 2024

Add guardrails for when setting event type and config for perf_event program. The added PerfEventConfig enum now defines the event type and config of interest.

Idiomatic Rust types are added for:

  • perf_hw_id => HardwareEvent
  • perf_sw_ids => SoftwareEvent
  • perf_hw_cache_id => HwCacheEvent
  • perf_hw_cache_op_id => HwCacheOp
  • perf_hw_cache_op_result_id => HwCacheResult

The motivation behind this is mainly for the type and config fields of bpf_link_info.perf_event.event (in a separate branch). I plan to use these newly added enums bpf_link_info metadata. https://elixir.bootlin.com/linux/v6.10/source/include/uapi/linux/bpf.h#L6714

One thing to note is that although Breakpoint/PERF_TYPE_BREAKPOINT variant exists, it is not fully implemented at the moment (due to some additional fields need in perf_event_attr like bp_type, etc.). It's only usage currently will be in retrieving link info.


I have it named as PerfEventConfig as the moment. I'm not too sure on the naming tho. :/


This change is Reviewable

Copy link

netlify bot commented Sep 25, 2024

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c5be267
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/6713c1cfe7ccf900088ef0a0
😎 Deploy Preview https://deploy-preview-1038--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

mergify bot commented Sep 25, 2024

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

@mergify mergify bot added the api/needs-review Makes an API change that needs review label Sep 25, 2024
@mergify mergify bot requested a review from alessandrod September 25, 2024 00:46
@mergify mergify bot added aya This is about aya (userspace) test A PR that improves test cases or CI labels Sep 25, 2024
@tyrone-wu tyrone-wu force-pushed the aya/perf-event-type-config branch 6 times, most recently from d33e54e to dec30be Compare September 25, 2024 22:55
@tyrone-wu tyrone-wu marked this pull request as ready for review September 25, 2024 23:22
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

LGTM

}

/// The "generalized" hardware CPU events provided by the kernel.
#[repr(u64)]
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this repr?

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 had it originally for converting the enum for config (which expects a u64), but I just confirmed that it wouldn't benefit here.

Off topic, but apparently it would only "benefit" when doing https://users.rust-lang.org/t/c-style-enums-reference-conversion/15501 or other wacky mem::transmute stuff.

Copy link

mergify bot commented Oct 2, 2024

@tyrone-wu, this pull request is now in conflict and requires a rebase.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @alessandrod and @tyrone-wu)


-- commits line 2 at r1:
nit: get this down to 50 columns plz (just the subject)


aya/src/programs/perf_event.rs line 89 at r1 (raw file):

    /// The total CPU cycles.
    #[doc(alias = "PERF_COUNT_HW_CPU_CYCLES")]
    CpuCycles = perf_hw_id::PERF_COUNT_HW_CPU_CYCLES as isize,

it'd be better to use repr(foo) if it means we can avoid these casts (especially from an unsigned type to this signed type).


aya/src/programs/perf_event.rs line 327 at r1 (raw file):

        let (event_type, config) = match perf_type {
            PerfEventConfig::Hardware(hw_event) => (PERF_TYPE_HARDWARE as u32, hw_event as u64),

would it be possible to avoid these as casts? In other words can we align the types we use such that we match the types of these constants? More precisely we should use the type perf_type_id instead of u32 and

Copy link
Contributor Author

@tyrone-wu tyrone-wu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @alessandrod and @tamird)


-- commits line 2 at r1:

Previously, tamird (Tamir Duberstein) wrote…

nit: get this down to 50 columns plz (just the subject)

👍 👍


aya/src/programs/perf_event.rs line 89 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

it'd be better to use repr(foo) if it means we can avoid these casts (especially from an unsigned type to this signed type).

Correct me if I'm wrong, but I believe casting is unavoidable since they're being coerced from enum type. isize would let the compiler handle its layout, so I don't think it would differ with a different repr since casting happens either way.


aya/src/programs/perf_event.rs line 327 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

would it be possible to avoid these as casts? In other words can we align the types we use such that we match the types of these constants? More precisely we should use the type perf_type_id instead of u32 and

🫡 The PerfEventConfig::Pmu variant not belonging to perf_type_id, but I was sorta able to align it for a single u32 cast.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @alessandrod and @tyrone-wu)


aya/src/programs/perf_event.rs line 89 at r1 (raw file):

Previously, tyrone-wu (Tyrone Wu) wrote…

Correct me if I'm wrong, but I believe casting is unavoidable since they're being coerced from enum type. isize would let the compiler handle its layout, so I don't think it would differ with a different repr since casting happens either way.

Could we use https://github.com/rust-num/num-derive to derive ToPrimitive on perf_hw_id and then use that instead of these as casts?

Copy link
Contributor Author

@tyrone-wu tyrone-wu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @alessandrod and @tamird)


aya/src/programs/perf_event.rs line 89 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Could we use https://github.com/rust-num/num-derive to derive ToPrimitive on perf_hw_id and then use that instead of these as casts?

I gave num-derive a try , it turns out the functions implemented from ToPrimitive aren't public ☹️. And also, the functions (.to_u32, etc.) return Options. https://docs.rs/num-traits/0.2.19/src/num_traits/cast.rs.html#17

There is https://github.com/illicitonion/num_enum, however, it looks like it just implements From<repr(num)> based on provided #[repr()], which wouldn't be too different from as. :/

@tyrone-wu tyrone-wu force-pushed the aya/perf-event-type-config branch 4 times, most recently from 1fdaa16 to d098fa4 Compare October 15, 2024 15:18
Add guardrails for when setting event type and config for perf_event
programs. The `PerfEventConfig` enum now defines the event `type` and
`config` of interest.

Remove public re-exports, and add idiomatic Rust types for:
- perf_hw_id => HardwareEvent
- perf_sw_ids => SoftwareEvent
- perf_hw_cache_id => HwCacheEvent
- perf_hw_cache_op_id => HwCacheOp
- perf_hw_cache_op_result_id => HwCacheResult

The motivation behind this is mainly for the `type` and `config` fields
of `bpf_link_info.perf_event.event`. The newly added enums are planned
to also be used in the `bpf_link_info` metadata.

Although `Breakpoint`/`PERF_TYPE_BREAKPOINT` variant exists, it is not
fully implemented. It's only usage at the moment is in link info.
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alessandrod and @tyrone-wu)


aya/src/programs/perf_event.rs line 89 at r1 (raw file):

Previously, tyrone-wu (Tyrone Wu) wrote…

I gave num-derive a try , it turns out the functions implemented from ToPrimitive aren't public ☹️. And also, the functions (.to_u32, etc.) return Options. https://docs.rs/num-traits/0.2.19/src/num_traits/cast.rs.html#17

There is https://github.com/illicitonion/num_enum, however, it looks like it just implements From<repr(num)>, which wouldn't be too different as. :/

Implementing From is better than as because it guarantees that there's no precision loss. Let's use num_enum?


aya/src/programs/perf_event.rs line 327 at r1 (raw file):

Previously, tyrone-wu (Tyrone Wu) wrote…

🫡 The PerfEventConfig::Pmu variant not belonging to perf_type_id, but I was sorta able to align it for a single u32 cast.

I'd love to see these casts go away


aya/src/programs/perf_event.rs line 26 at r3 (raw file):

/// The type of perf event and their respective configuration.
#[doc(alias = "perf_type_id")]
#[derive(Debug, Clone)]

should this be Copy too?


aya/src/programs/perf_event.rs line 274 at r3 (raw file):

/// #     Ebpf(#[from] aya::EbpfError)
/// # }
/// use aya::{

since you're shuffling, put this at the top?


aya/src/programs/perf_event.rs line 288 at r3 (raw file):

/// for cpu in online_cpus().map_err(|(_, error)| error)? {
///     prog.attach(
///         perf_type.clone(),

consider removing clone this if you make the type Copy


aya/src/programs/perf_event.rs line 310 at r3 (raw file):

    /// Attaches to the given perf event.
    ///
    /// The [`perf_type`](PerfEventConfig) defines the event `type` and `config` of interest.

did you want want to add the word "argument"? you have it in the next sentence.


aya/src/programs/perf_event.rs line 344 at r3 (raw file):

                    PerfEventConfig::Raw { event_id } => (PERF_TYPE_RAW, event_id),
                    PerfEventConfig::Breakpoint => (PERF_TYPE_BREAKPOINT, 0),
                    PerfEventConfig::Pmu { .. } => unreachable!(), // not possible due to earlier match

yuck. can we avoid this? the only shared code is as u32 =/

Copy link

mergify bot commented Nov 18, 2024

@tyrone-wu, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/needs-review Makes an API change that needs review aya This is about aya (userspace) needs-rebase test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants