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

link: support bpf perf event link, add bpf_cookies #565

Merged
merged 7 commits into from Mar 11, 2022

Conversation

mmat11
Copy link
Collaborator

@mmat11 mmat11 commented Feb 11, 2022

This PR adds bpf_cookie and bpf_perf_link. bpf_perf_link is used automatically, if supported, in place of perfEvent. bpf_cookie can be specified only if the link is implemented via bpf_perf_link; if this is not the case, an error will be returned

@mmat11 mmat11 marked this pull request as draft February 11, 2022 09:48
@mmat11 mmat11 force-pushed the matt/bpf-perf-link branch 3 times, most recently from 7c690ea to 4023616 Compare February 11, 2022 10:21
@mmat11 mmat11 requested review from lmb and ti-mo February 11, 2022 11:03
@mmat11 mmat11 marked this pull request as ready for review February 11, 2022 11:03
internal/sys/types.go Show resolved Hide resolved
internal/sys/types.go Outdated Show resolved Hide resolved
link/perf_event.go Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
internal/cmd/gentypes/main.go Show resolved Hide resolved
Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
@mmat11 mmat11 changed the title link/perf_event: attach via bpf_link if available link: support bpf perf event link, add bpf_cookies Feb 14, 2022
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

I did another pass, thanks for clarifying my questions re bpf.h pointers. Regarding Link and how to implement that: I think we should split the code that does perf event set up from the code that does perf event attachment. Rough outline:

  • Add type perfEventIoctl struct and type perfEventLink struct. The former has stub functions to implement Link, the latter just wraps a RawLink similar to rawTracepoint (maybe we can just use RawLink directly?)
  • perfEvent.attach becomes a free function attachPerfEvent or similar which returns a Link, either perfEventIoctl or perfEventLink.

link/kprobe.go Outdated Show resolved Hide resolved
…ptions for kprobes

Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
@mmat11
Copy link
Collaborator Author

mmat11 commented Feb 21, 2022

@lmb I did some changes, not sure if this is what you asked (I needed to reimplement certain RawLink methods). Also, do you know where I could check which operations for which link type are supported? I am asking because I kept getting EINVAL in all testLink()/update

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Please take another look at the changes I outlined in #565 (review). My idea for the final state is as follows:

  • perfEvent: encapsulates handling tracefs / pmu, creating the perf event really.
  • perfEventLink: attaches a bpf program to a perfEvent using BPF_LINK_CREATE.
  • perfEventIoctl: attaches a bpf program to a perfEvent using ioctl().

perfEventLink and perfEventIoctl will wrap perfEvent so that they can do clean up. perfEventLink will additionally embed RawLink.

perfEvent should not implement Link, only perfEventLink and perfEventIoctl do. This is because we want a Link to always represent the result of attaching a program.

If we follow this schema we don't have to change kprobePmu, etc. and can keep attachPerfEvent more straighforward.

link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
@mmat11 mmat11 marked this pull request as draft March 4, 2022 00:11
@mmat11
Copy link
Collaborator Author

mmat11 commented Mar 4, 2022

@lmb I set this as draft as I am still figuring out what to do with pinning

@mmat11 mmat11 marked this pull request as ready for review March 4, 2022 11:29
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Looks good, I left a few nits. Thanks for working in the feedback.

I'm currently traveling, so I'll probably merge this Friday if you're not in a hurry.

link/link.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
link/perf_event.go Show resolved Hide resolved
link/perf_event.go Outdated Show resolved Hide resolved
Signed-off-by: Mattia Meleleo <mattia.meleleo@elastic.co>
@lmb
Copy link
Collaborator

lmb commented Mar 11, 2022

Thanks a lot!

@lmb lmb merged commit bf256fd into cilium:master Mar 11, 2022
@mmat11 mmat11 deleted the matt/bpf-perf-link branch November 1, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants