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

Unroll improvements #1286

Merged
merged 2 commits into from
Apr 26, 2020
Merged

Unroll improvements #1286

merged 2 commits into from
Apr 26, 2020

Conversation

mmisono
Copy link
Collaborator

@mmisono mmisono commented Apr 19, 2020

Add support of using positional params in unroll and increase the unroll limit to 100.

Example:

% sudo ./src/bpftrace -e 'BEGIN { unroll($1) { printf("hi\n"); } }' 5
Attaching 1 probe...
hi
hi
hi
hi
hi
^C

@danobi
Copy link
Member

danobi commented Apr 19, 2020

Do you think it would work to rewrite unroll() as a while loop (based on #1066)? I think it would result in less byte code. I can't think of any reasons why it would break existing programs.

@danobi
Copy link
Member

danobi commented Apr 19, 2020

Hmm except on systems without loop support.

Copy link
Contributor

@fbs fbs left a comment

Choose a reason for hiding this comment

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

Other than this lgtm

src/parser.yy Outdated Show resolved Hide resolved
@mmisono
Copy link
Collaborator Author

mmisono commented Apr 20, 2020

Fixed parser.yy. Thanks @fbs
probe.interval_order tests failed for some reason, but I believe it wasn't related to this PR.


Do you think it would work to rewrite unroll() as a while loop (based on #1066)?

I think it would, but potentially unrolling optimize an execution speed so I guess keeping unrolling as is is good.

@fbs
Copy link
Contributor

fbs commented Apr 20, 2020

probe.interval_order tests failed for some reason, but I believe it wasn't related to this PR.

It always was a bit flaky but the new CI seems to have made it worse, got a commit to remove it in #1066

@fbs
Copy link
Contributor

fbs commented Apr 20, 2020

Bit curious where the 100 came from (or 20 for that matter). What is your usecase here? :)

@mmisono
Copy link
Collaborator Author

mmisono commented Apr 20, 2020

Actually there is no particular reason for 100, I just want to benchmark bpftrace (and BPF program) performance and unrolling is an easy way to generate long instructions. I feel 20 is a bit small. Maybe introducing BPFTRACE_MAX_UNROLL env variables is better. I think the limiting is useful because it helps us to prevent unrolling a large number of loop which consequently causes a stack overflow or something (this is especially useful when fuzzing since fuzzer generates unexpected inputs)

By the way, it is easy to circumvent unroll limitation: unroll(10) { unroll(10) { $i++; } }.

@fbs
Copy link
Contributor

fbs commented Apr 22, 2020

Reminder to add a changelog entry for this (#1255) before merging

This enables using positional params in unroll. For example:

```
% sudo ./src/bpftrace -e 'BEGIN { unroll($1) { printf("hi\n"); } }' 5
Attaching 1 probe...
hi
hi
hi
hi
hi
^C
```
Now that unrolling does not rely on LLVM's unroll, 20 is too restrictive
for the maximum unroll limit.
@mmisono
Copy link
Collaborator Author

mmisono commented Apr 25, 2020

Rebased onto master and updated a changelog.

@fbs fbs merged commit 8e833e7 into bpftrace:master Apr 26, 2020
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