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

Added options to support socket timestamping #504

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tglane
Copy link

@tglane tglane commented Apr 15, 2024

This PR adds the options for a socket to generate and report timestamps when a packet was received by the hardware/kernel.

The platforms supported by socket2 vary in the support for the timestamping options but I tried to implemented this as platform independent as possible. Unfortunately all the cfg attributes produce a lot of noise in the code.

For documentation check:

Closes #415

@tglane
Copy link
Author

tglane commented Apr 15, 2024

One note: The timestamps are reported as control messages/ancillary data as a field in a struct cmsghdr. socket2 implements the MsgHdr type which could be used to access those control messages but I think it would be out of scope of this lib so I did not implement that. I would like to hear other opinions on that.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Sorry for taking a little while @tglane to review this, but I'm afraid due to the API surface this will take some time to review.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

I'm not yet convinced of this design.

I think it would make more sense using an API like TcpKeepalive.

target_os = "vita",
target_os = "hurd",
))))
)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these lists are so long, can we make it "opt-in" instead of excluding basically everything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, since it's not available on all OS, we need to use the all feature.

Copy link
Author

@tglane tglane Apr 24, 2024

Choose a reason for hiding this comment

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

Yeah you are right. I will change that to reduce the overall noise in the code.

Copy link
Author

Choose a reason for hiding this comment

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

In 08e843b I changed the cfgattributes to be opt-in. Thanks for this remark, its so much cleaner now.

)]
pub fn timestamp_ns(&self) -> io::Result<bool> {
unsafe {
getsockopt::<c_int>(self.as_raw(), sys::SOL_SOCKET, sys::SO_TIMESTAMPNS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any value in having both SO_TIMESTAMPNS and SO_TIMESTAMP? Ideally we can make a wrapper that maps it to Duration regardless of the underlying option.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if I understand what you are proposing here. This API should not allow the user to set timestamps or some duration. The timestamping API allows the user to receive query timestamps for when packets were send or received (depending on the flags).

With SO_TIMESTAMP you can query packet timestamps as struct timeval with microsecond accuracy while SO_TIMESTAMPNS allows for query packet timestamps as struct timespec with nanosecond accuracy. So yes, we could probably merge them into one function with an enum to specify the accuracy but we definitely need some way to set either of these.

@tglane
Copy link
Author

tglane commented May 5, 2024

@Thomasdezeeuw I just revisited this PR and thought about your comment regarding the API design. I still don't think that a API similar to TcpKeepalive would make much sense here because for the SO_TIMESTAMPING option we really need the bitmap encoded into an unsigned 32 bit integer for configuration, while SO_TIMESTAMP and SO_TIMESTAMPNS only require a boolean parameter.

What we could do a enum Timestamp which holds a variant for reporting milliseconds, a variant for reporting nanoseconds like so:

enum Timestamp {
    ReportMs(bool), // represents SO_TIMESTAMP
    ReportNs(bool), // represents SO_TIMESTAMPNS
}

But I would argue that the SO_TIMESTAMPING option should be handled separate, because it allows customize the source of the generated timestamps (reception, transmission and hardware or software timestamps). This is what I tried to encode in the current option struct Timestamping and I don't really see how that would fit into the same function as the other two settings.

I`d like to hear your opinion on that one.

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.

Socket Timestamping Support
2 participants