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.

Sign up for GitHub

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

Jump to bottom

profiler: Add batch ebpf map operations (reloaded) #602

Closed
wants to merge 3 commits into from
Closed

profiler: Add batch ebpf map operations (reloaded) #602

wants to merge 3 commits into from

Conversation

kakkoyun
Copy link
Member

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

@kakkoyun
Add batch ebpf map operations (reloaded)
c277ed1 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun mentioned this pull request Jul 19, 2022
Open
@kakkoyun
Clean comments
fcd6c17 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Bump needed Kernel version
c9ea337 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun marked this pull request as ready for review July 19, 2022 15:01
@kakkoyun
Copy link
Member Author

cc @javierhonduco @v-thakkar

v-thakkar
v-thakkar reviewed Jul 19, 2022
View reviewed changes
pkg/profiler/profiler.go
// We are getting and deleting the whole map, so there should not be a next batch.
if nextCountKey != uintptr(0) {
level.Debug(p.logger).Log("msg", "Next batch should be null", "nextBatch", nextCountKey)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're checking this 🎉

@v-thakkar
Copy link
Contributor

LGTM!

javierhonduco
javierhonduco approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@javierhonduco javierhonduco left a comment

Choose a reason for hiding this comment

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

LGTM! 😄

Do we need to update the minikube ISO somewhere or it's automatically updated? Before merging, could you run an e2e test with the cpp binary if you haven't already?

@kakkoyun
Copy link
Member Author

Do we need to update the minikube ISO somewhere or it's automatically updated?

We only document it and don't actually force any minikube iso version (except e2e tests cc @Sylfrena). I have already updated the documentation in repo and I have a PR to update parca.dev

Before merging, could you run an e2e test with the cpp binary if you haven't already?

Will do 👍

@kakkoyun kakkoyun added this to the v0.11.0 milestone Aug 1, 2022
@kakkoyun kakkoyun marked this pull request as draft August 1, 2022 14:59
@javierhonduco
Copy link
Contributor

Was thinking of batch BPF operations in the context of cleaning up maps. Let's not forget to take a look at the profiles and see if it could be worth it to batch-delete our maps 😄

@javierhonduco
Copy link
Contributor

Some quick data (https://pprof.me/3f07325):

  • pkg/profiler/cpu.(*bpfMaps).clean: 2.8% of the cycles
  • github.com/aquasecurity/libbpfgo.(*BPFMapIterator).Next 0.9% of the cycles each
  • pkg/profiler/cpu.(*bpfMaps).readKernelStack, readKernelStack, readUserStackWithDwarf : 0.9% of the cycles each

@kakkoyun kakkoyun removed this from the v0.12.0 milestone Mar 15, 2023
@kakkoyun
Copy link
Member Author

#74 (comment)

@kakkoyun kakkoyun closed this Apr 24, 2023
@kakkoyun kakkoyun deleted the batch_map_updates branch May 17, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v-thakkar v-thakkar v-thakkar left review comments

@javierhonduco javierhonduco javierhonduco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kakkoyun @v-thakkar @javierhonduco