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

Make print() builtin synchronous #3028

Open
ajor opened this issue Mar 1, 2024 · 4 comments
Open

Make print() builtin synchronous #3028

ajor opened this issue Mar 1, 2024 · 4 comments
Labels
enhancement New feature or request, changes on existing features

Comments

@ajor
Copy link
Member

ajor commented Mar 1, 2024

print(@map) is designed to print all entries in a map. It currently does this by passing a MapID reference to userspace, where bpftrace iterates over the map entries to print them out.

The problem is that this makes print(@map) an asynchronous operation, which can produce unexpected results:

# bpftrace -e 'BEGIN { @x[0] = 1; print(@x); @x[0] = 2; exit() }'
Attaching 1 probe...
@x[0]: 2

Now that BPF has support for iterating over map elements in the kernel, we will be able to reimplement print(@map) as a synchronous operation by writing all the map keys+values to the ring buffer instead of a single MapID reference.

I think this could be implemented with minimal new codegen once #3003 is merged - by rewriting the AST of any print(@map) call into for ($kv : @map) { print($kv) }. Some work will be needed to ensure the entries are still formatted and sorted correctly.

The previous script should produce a more expected result in the future:

# bpftrace -e 'BEGIN { @x[0] = 1; print(@x); @x[0] = 2; exit() }'
Attaching 1 probe...
@x[0]: 1

This also applies to some other builtin functions which operate on maps asynchronously: clear(), zero()

@ajor ajor added the enhancement New feature or request, changes on existing features label Mar 1, 2024
@danobi
Copy link
Member

danobi commented Mar 1, 2024

While it's true changing from async to sync is backwards compatible, I'm a little worried about large maps overflowing the perg/ring buffer. In theory the user could be relying on the ability to print a huge map.

We could always add some way for the user to opt in, but I also dislike these kinds of knobs in general.

Seems like a tradeoff and I don't know the right answer atm.

@danobi
Copy link
Member

danobi commented Mar 1, 2024

Tangentially related: rust and golang have a edition/version mechanisms for these kinds of subtle changes. Basically you tell the compiler which edition/version you want to build against, and compiler will maintain semantics of that version. Newer code, when you cargo new or go mod init will target the latest version.

A mechanism like that could help us here. But it's probably overkill. Something to think about though.

@danobi
Copy link
Member

danobi commented Mar 1, 2024

(Sorry for stream of consciousness)

Maybe we could add an aprint() alias for the old behavior? And print() gets sync by default. I imagine this is beneficial for nearly everyone while also giving pathological users a way out.

@ajor
Copy link
Member Author

ajor commented Mar 20, 2024

Maybe we could add an aprint() alias for the old behavior? And print() gets sync by default. I imagine this is beneficial for nearly everyone while also giving pathological users a way out.

In general, I'm more in favour of adding new syntax instead of new bulitin functions as I think it tends to be more flexible. What about introducing a way to take references to maps and allowing them to be printed, e.g.:

$map_ref = &@mymap;
print($map_ref);

It could also be useful in other scenarios, e.g. for double buffering:

$map_ref = $cond ? &@map1 : &@map2;
$map_ref[blah] = foo;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request, changes on existing features
Projects
None yet
Development

No branches or pull requests

2 participants