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

info: Add utc_offset info for external usage #1923

Merged
merged 1 commit into from
May 18, 2024

Conversation

honggyukim
Copy link
Collaborator

@honggyukim honggyukim commented May 10, 2024

In some cases, there is a requirement to get the time offset between UTC time and our timestamp from the given clock source, CLOCK_MONOTONIC by default.

This patch provides an additional info to provide UTC time offset in the output of uftrace info.

This number can be used to make time adjustment easier for external tools such as tracecompass[1].

Closes: #1916

[1] https://eclipse.dev/tracecompass
Requested-by: Matthew Khouzam matthew.khouzam@gmail.com
Signed-off-by: Honggyu Kim honggyu.kp@gmail.com

@honggyukim
Copy link
Collaborator Author

This patch is to make the UTC time adjustment easier than https://youtu.be/KtVdeHfD45o?si=hIMvE10XI87XqXIW.

@honggyukim
Copy link
Collaborator Author

The remaing job is to make this feature doesn't break previously recorded data to support backward compatibility.

cmds/info.c Outdated
Comment on lines 1042 to 1043
if (info_mask & UTC_OFFSET)
process(data, fmt, "utc offset", info->utc_offset);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the info->utc_offset is for internal information so better not to be printed in uftrace info output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's now removed after another update.

@honggyukim honggyukim force-pushed the check/utc-offset branch 3 times, most recently from 5d83c87 to d74c7d2 Compare May 12, 2024 03:54
@honggyukim
Copy link
Collaborator Author

I just fixed uftrace dump crash with https://github.com/namhyung/uftrace/compare/96da52d61136ba055102603644318eedee75562c..5d83c874643b4668f99092faf7ff8c23f8803fe5.

@namhyung @MatthewKhouzam I think it's good to be merged now. Please have a look.

Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

It's needed to correlate the uftrace trace with external trace data. Anyway it looks ok, only a nitpick below.

cmds/info.c Outdated
/* calculate time offset between UTC and clock_source. */
offset = difftime(current_time, ts.tv_sec);

dprintf(fha->fd, "utc_offset:%ld\n", (time_t)offset);
Copy link
Owner

Choose a reason for hiding this comment

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

For %ld, you'd better cast it to unsigned long.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I understand, time_t is long type so %ld is used instead of %lu.

# 143 "/usr/include/x86_64-linux-gnu/bits/types.h" 2 3 4
        ...
typedef long int __time_t;
        ...
typedef __time_t time_t;

Since this offset is used to adjust the other time_t value for UTC time, I made it casted to the same type time_t. Should I change it to unsigned long and print it with %lu instead?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, sorry. I meant long. But it could be different on different architecture or distro. So I think it's better to explicitly cast to long instead of time_t.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. I've just changed it to use long for offset type itself.

In some cases, there is a requirement to get the time offset between UTC
time and our timestamp from the given clock source, CLOCK_MONOTONIC by
default.

This patch provides an additional info to provide UTC time offset in the
output of uftrace info.

This number can be used to make time adjustment easier for external
tools such as tracecompass[1].

Closes: namhyung#1916

[1] https://eclipse.dev/tracecompass
Requested-by: Matthew Khouzam <matthew.khouzam@gmail.com>
Signed-off-by: Honggyu Kim <honggyu.kp@gmail.com>
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

LGTM

@namhyung namhyung merged commit fae85a8 into namhyung:master May 18, 2024
3 checks passed
@honggyukim honggyukim deleted the check/utc-offset branch May 18, 2024 00:50
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.

Give monotonic time offset from real time.
2 participants