-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 access to CLOCK_MONOTONIC with nsecs(monotonic) #2718
Conversation
Also remove some optional parameters to make it easier to understand which test() overload is being used.
This lets users explicitly request CLOCK_MONOTONIC when it is desired to get a timestamp exclusive of suspend time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits...
(I forgot the submit this review, maybe it is too late now...)
@@ -1337,18 +1332,17 @@ void CodegenLLVM::visit(Call &call) | |||
if (call.type.ts_mode == TimestampMode::boot || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be:
if (call.type.ts_mode == TimestampMode::sw_tai)
{
uint64_t delta = bpftrace_.delta_taitime_->tv_sec * 1e9 + bpftrace_.delta_taitime_->tv_nsec;
expr_ = b_.CreateAdd(b_.CreateGetNs(TimestampMode::boot, call.loc), b_.getInt64(delta));
}
else
{
expr_ = b_.CreateGetNs(call.type.ts_mode, call.loc);
}
case TimestampMode::boot: | ||
fn = bpftrace_.feature_->has_helper_ktime_get_boot_ns() | ||
? libbpf::BPF_FUNC_ktime_get_boot_ns | ||
: libbpf::BPF_FUNC_ktime_get_ns; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we let user to be explicit about timestamp mode, should this fallback behavior to be documented, or change it into a loud fatal error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we might want to change some stuff around nsecs()
/nsecs(boot)
, but I'm not sure exactly in what way and wanted to keep this PR simple. We can do this later if any good ideas come up
@@ -159,7 +159,7 @@ class IRBuilderBPF : public IRBuilder<> | |||
const bool inverse, | |||
const location &loc, | |||
MDNode *metadata); | |||
CallInst *CreateGetNs(bool boot_time, const location &loc); | |||
CallInst *CreateGetNs(TimestampMode ts, const location &loc); | |||
CallInst *CreateGetTaiNs(const location &loc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateGetTaiNs() should be removed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sloppy! Thank you
This lets users explicitly request CLOCK_MONOTONIC when it is desired to get a timestamp exclusive of suspend time.
Includes some refactoring of the semantic analyser
test()
overloads and deleted some redundant and out-of-date sections of the reference guide.