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

Add jiffies builtin with bpf_jiffies64 #2769

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

hyptrap
Copy link
Contributor

@hyptrap hyptrap commented Sep 19, 2023

Add jiffies builtin, one of the use case is to calculate the sched_clock indirectly within some old kernels.

Result:

bpftrace --unsafe -e 'BEGIN { system("grep jiffies: /proc/timer_list | head -1"); print(jiffies); exit() }'
Attaching 1 probe...
jiffies: 4325204698
4325204697
Checklist
  • Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

Copy link
Contributor

@viktormalik viktormalik left a comment

Choose a reason for hiding this comment

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

Thanks a lot for implementing this!

Looks good, a couple of things are missing, though:

  • detection if the helper is present in the kernel, it should go to BPFFeature and then be called from semantic analyser with a proper error message if the helper is missing,
  • semantic analyser tests,
  • runtime tests, although I'm not sure if we can come up with something useful here.

See e.g. my recent PR #2692 for inspiration how to implement the above points.

man/adoc/bpftrace.adoc Outdated Show resolved Hide resolved
src/ast/irbuilderbpf.cpp Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@hyptrap
Copy link
Contributor Author

hyptrap commented Sep 19, 2023

Thanks a lot for implementing this!

Looks good, a couple of things are missing, though:

* detection if the helper is present in the kernel, it should go to `BPFFeature` and then be called from semantic analyser with a proper error message if the helper is missing,

* semantic analyser tests,

* runtime tests, although I'm not sure if we can come up with something useful here.

See e.g. my recent PR #2692 for inspiration how to implement the above points.

I'm quite unfamiliar here, thanks for the advising! Pushed a new commit, fixed those things.

As for the runtime tests, I want to compare the jiffies between bpftrace and /proc/timer_list, but neither the test engine nor the bpftrace can achieve that easily (the output of system() call in bpftrace cannot be extracted, and putting the comparison outside bpftrace will cause a huge jiffies delay between them), so yes, nothing useful idea has came up 😶‍🌫️.

@viktormalik
Copy link
Contributor

Please fix the problems reported by clang-format.

@viktormalik
Copy link
Contributor

Thanks!

Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

tests/runtime/builtin Outdated Show resolved Hide resolved
@hyptrap hyptrap requested a review from danobi September 22, 2023 03:17
@danobi
Copy link
Member

danobi commented Sep 22, 2023

Thanks for the cleanup!

@viktormalik
Copy link
Contributor

Could you squash the first two commits together? Thanks!

@hyptrap
Copy link
Contributor Author

hyptrap commented Sep 22, 2023

Could you squash the first two commits together? Thanks!

Done!

@viktormalik viktormalik merged commit 4629b3c into bpftrace:master Sep 22, 2023
21 checks passed
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

3 participants