aya: use NonZeroU32 for perf event process scope

[swananan]

Summary

Follow-up on the recent PR #1543

Testings

perf_event_open_trace_point is only reachable on kernels < 4.17. aya CI runs ≥ 5.10, so uprobe e2e tests on CI never enter this branch. Coverage is provided by a mocked-syscall unit test driving all three pid-scope cases (None / Some(0) / Some(non-zero)) through override_syscall.

Added/updated tests?

We strongly encourage you to add a test for your changes.

• Yes
• No, and this is why: please replace this line with details on why tests
  have not been included
• I need help with writing tests

Checklist

• Rust code has been formatted with cargo +nightly fmt.
• All clippy lints have been fixed.
  You can find failing lints with cargo xtask clippy.
• Unit tests are passing locally with cargo test.
• The Integration tests are passing locally.
• I have blessed any API changes with cargo xtask public-api --bless.

(Optional) What GIF best describes this PR or how it makes you feel?

---

This change is 

[netlify]

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	1a6d44e
🔍 Latest deploy log	https://app.netlify.com/projects/aya-rs-docs/deploys/69f618f825a0690008bd60b5
😎 Deploy Preview	https://deploy-preview-1547--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

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

[tamird]

@tamird reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on swananan).

---

aya/src/sys/perf_event.rs line 273 at r1 (raw file):

    const EVENT_ID: u64 = 123;

    // `override_syscall` takes a `fn` pointer that can't capture, so per-case

would life be easier if you could change it to take Box<Fn<....>>?

---

test/integration-test/src/tests/perf_event_bp.rs line 124 at r1 (raw file):

    let pid = std::process::id();
    let scope_pid = NonZeroU32::new(pid).unwrap();

can this just be called pid?

[swananan]

@swananan made 2 comments.
Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on tamird).

---

aya/src/sys/perf_event.rs line 273 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

would life be easier if you could change it to take Box<Fn<....>>?

Done.

---

test/integration-test/src/tests/perf_event_bp.rs line 124 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can this just be called pid?

Done.

[tamird]

@tamird reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on swananan).

---

aya/src/sys/perf_event.rs line 170 at r2 (raw file):

pub(crate) fn perf_event_open_trace_point(
    event_id: u64,
    pid: Option<u32>,

how come we aren't plumbing this all the way up?

[swananan]

@swananan made 1 comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on tamird).

---

aya/src/sys/perf_event.rs line 170 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

how come we aren't plumbing this all the way up?

Right — UProbe::attach is the only public API that exposes pidtoday. I assume you mean adding pid to TracePoint::attach.

Looking at libbpf as reference, bpf_program__attach_tracepoint doesn't expose pid either; perf-level pid filtering doesn't constrain BPF execution on the tracepoint path anyway, so users would filter with bpf_get_current_pid_tgid() inside the program. I'd leave TracePoint::attach as-is. WDYT?

[swananan]

@swananan made 1 comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on tamird).

---

aya/src/sys/perf_event.rs line 170 at r2 (raw file):

Previously, swananan (swananan) wrote…

Right — UProbe::attach is the only public API that exposes pidtoday. I assume you mean adding pid to TracePoint::attach.

Looking at libbpf as reference, bpf_program__attach_tracepoint doesn't expose pid either; perf-level pid filtering doesn't constrain BPF execution on the tracepoint path anyway, so users would filter with bpf_get_current_pid_tgid() inside the program. I'd leave TracePoint::attach as-is. WDYT?

I rethought this a bit. Plumbing a scope only into perf_event_open_trace_point doesn’t seem to buy us much: plain tracepoints don’t need it, uprobes already translate their scope before this point, and kprobes don’t need this distinction. So I’d prefer to keep this helper scoped to the raw perf_event pid mapping.

[tamird]

@tamird made 1 comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on swananan).

---

aya/src/sys/perf_event.rs line 170 at r2 (raw file):

Previously, swananan (swananan) wrote…

I rethought this a bit. Plumbing a scope only into perf_event_open_trace_point doesn’t seem to buy us much: plain tracepoints don’t need it, uprobes already translate their scope before this point, and kprobes don’t need this distinction. So I’d prefer to keep this helper scoped to the raw perf_event pid mapping.

I wasn't clear enough. What I really meant was that UProbe::attach takes a UProbeScope, translates it to an Option<u32> and then here we translate it again to PerfEventScope. My suggestion is to plumb PerfEventScope up so that the conversion from UProbeScope to PerfEventScope can happen directly.

[swananan]

@swananan made 1 comment.
Reviewable status: 5 of 10 files reviewed, 1 unresolved discussion (waiting on tamird).

---

aya/src/sys/perf_event.rs line 170 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I wasn't clear enough. What I really meant was that UProbe::attach takes a UProbeScope, translates it to an Option<u32> and then here we translate it again to PerfEventScope. My suggestion is to plumb PerfEventScope up so that the conversion from UProbeScope to PerfEventScope can happen directly.

Got it, that makes sense.

[tamird]

LGTM. @alessandrod had thoughts on NonZero in the public API, so will let him voice those here.

@tamird reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on swananan).

[alessandrod]

As I mentioned on discord, I'm not a fan of NonZeroU32 in public APIs.

If we're fixing up this corner of the API, I'd do something like:

pub struct Pid(NonZeroU32);

impl Pid {
    pub fn new(pid: u32) -> Option<Self> {
        NonZeroU32::new(pid).map(Self)
    }

    pub fn current() -> Self {
        Self(NonZeroU32::new(std::process::id()).unwrap())
    }

    pub fn get(self) -> u32 {
        self.0.get()
    }
}

and then the scope APIs also look weird, instead of having CallingProcess OneProcess etc we could have

pub struct PerfEventScope {
    pub process: ProcessScope,
    pub cpu: Option<u32>,
}

pub enum ProcessScope {
   All,
   Pid(Pid)
}

and I think this covers all combinations?

[tamird]

The kernel exposes "calling process", IMO we should retain the ability to express that. I don't have strong feelings on the newtype wrapper around NonZeroU32, but if we introduce it then we should also use it in UProbeScope.

Maybe we should make these enums more opaque and control ingress with constructors. But IMO something like pub fn new(pid: u32) -> Option<Self> is worse than NonZero* because it's not clear from the signature or the name when it would return None.

[alessandrod]

is worse than NonZero* because it's not clear from the signature or the name when it would return None.

By this argument no fallible APIs are good because in the name they don’t express why they’re fallible.

NonZeroU32 is bad for a number of reasons, starting from the name: the fact that a pid is a u32 is an implementation detail.

Ack on having a Current variant, but seems like the process and the cpu id should be decoupled

[tamird]

You can't decouple them because when you say "all processes" you must specify a CPU.

By this argument no fallible APIs are good because in the name they don’t express why they’re fallible.

No, they are just worse than the ones that say in the name why they are fallible. But I struggle to understand your beef with NonZeroU32; I agree u32 is an implementation detail, but even your sketch has u32 in the API. Can't get away from this implementation detail unless you completely hide which we can't do because there's no way to get an opaque PID of another process.

[alessandrod]

You can't decouple them because when you say "all processes" you must specify a CPU.

By this argument no fallible APIs are good because in the name they don’t express why they’re fallible.

No, they are just worse than the ones that say in the name why they are fallible. But I struggle to understand your beef with NonZeroU32

not sure we need to rehash the history of PID handling, even posix does pid_t in the year of the lord 2026. It's "just" a typedef but it clearly communicates that you're dealing with a pid and not an arbitrary integer. Abstractions are cool!

You can't decouple them because when you say "all processes" you must specify a CPU.

Right now I remember this curse

[tamird]

not sure we need to rehash the history of PID handling, even posix does pid_t in the year of the lord 2026. It's "just" a typedef but it clearly communicates that you're dealing with a pid and not an arbitrary integer. Abstractions are cool!

of course they are, but std::process::id() does not abstract, it returns u32

[alessandrod]

of course they are, but std::process::id() does not abstract, it returns u32

And so do we in master. If we're trying to abstract more, the next step is a pid abstraction, not "still u32 but not zero ok!!"

[tamird]

Right but it is not an abstraction! If the user wants to target any pid that isn't the current process, there is no way for us to do that without exposing u32 in the API.

[swananan]

Thanks, I missed the some of the discord context earlier😂, now I updated the PR accordingly:

1. Introduced a Pid wrapper
2. Updated the UProbeScope::OneProcess to use Pid as well