Skip to content

Adding packet receipt timestamp collection into RecvMeta for linux #2151

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+116 −2
Open

Adding packet receipt timestamp collection into RecvMeta for linux #2151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion quinn-udp/src/cmsg/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ impl<M: MsgHdr> Drop for Encoder<'_, M> {
/// `cmsg` must refer to a native cmsg containing a payload of type `T`
pub(crate) unsafe fn decode<T: Copy, C: CMsgHdr>(cmsg: &impl CMsgHdr) -> T {
assert!(mem::align_of::<T>() <= mem::align_of::<C>());
debug_assert_eq!(cmsg.len(), C::cmsg_len(mem::size_of::<T>()));
debug_assert_eq!(
cmsg.len(),
C::cmsg_len(mem::size_of::<T>()),
"cmsg truncated -- you might need to raise the CMSG_LEN constant for this platform"
);
ptr::read(cmsg.cmsg_data() as *const T)
}

Expand Down
2 changes: 2 additions & 0 deletions quinn-udp/src/fallback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ impl UdpSocketState {
addr: addr.as_socket().unwrap(),
ecn: None,
dst_ip: None,
#[cfg(not(wasm_browser))]
timestamp: None,
};
Ok(1)
}
Expand Down
9 changes: 9 additions & 0 deletions quinn-udp/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ pub struct RecvMeta {
/// Populated on platforms: Windows, Linux, Android (API level > 25),
/// FreeBSD, OpenBSD, NetBSD, macOS, and iOS.
pub dst_ip: Option<IpAddr>,
/// A timestamp for when the given packet was received by the operating system.
/// Controlled by the `set_recv_timestamping` option on the source socket where available.
///
/// Populated on platforms with varying clock sources, as follows:
/// - Linux: `CLOCK_REALTIME` (see `SO_TIMESTAMPNS` in `man 7 socket` for more information)
Copy link
Collaborator

@mxinden mxinden Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To me the main benefit of quinn-udp is its single interface abstraction over the various concrete OS-specific UDP mechanisms. Thus I would like to strive for a single interface to the user, abstracting over many potentially different OS-specific implementations. Strive, as in not always achievable.

I am fine with RecvMeta::timestamp only being available on certain OSs, e.g. via #[cfg(. Bugs on the user side will be detected at compile-time.

What I would like to prevent is the value of timestamp to have different meanings across OSs, but no difference in its type signature. Bugs on the user side will only be detected at runtime.

If I understand the above correctly, the Duration embedded in RecvMeta::timestamp should be interpreted as the time since UNIX_EPOCH on Linux. What would we expose on other OSs in the future, also Duration? What is the interpretation of that Duration? Can we find a single abstraction?

Copy link
Contributor Author

@recatek recatek Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's unfortunately unavoidable that it will have different meanings across operating systems. It can even have different meanings within the same OS depending on how you configure the socket (assuming future support) -- FreeBSD can be configured to report timestamps in CLOCK_REALTIME, CLOCK_MONOTONIC, or CLOCK_BOOTTIME (and potentially others). MacOS has similar support as well. Some of these sources aren't trivially convertible to CLOCK_REALTIME/SystemTime.

That was the idea behind the original RecvTime enum I made to differentiate the clock sources, since we do (usually) know what clock source it's from based on the control message we took the timestamp from. The idea would be that that RecvTime enum could expand to something like:

pub enum RecvTime {
    Realtime(SystemTime),
    Monotonic(MonotonicTime),
    Boottime(BootTime),
}

...and so on. Or could be simplified and use Duration for each, but still be differentiated by the enum discriminant.

However, that makes it a bit of a pain to work with between quinn-proto and quinn-udp since I'd need to replicate that structure in both, and have conversion functions between them. After discussing it with @Ralith on Discord we agreed that going back to a Duration would be the much simpler plan. The user has to configure whether to receive timestamps, and according to what clock, on the socket, and so the user should know based on the OS and their own configuration what time clock source they're getting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the above explanation.

I still have concerns with this type signature.

  • Representing a timestamp as a Duration is counter-intuitive to me.
  • Given that quinn-udp controls the socket, does quinn-udp also control the timer configuration, i.e. CLOCK_REALTIME, CLOCK_MONOTONIC, or CLOCK_BOOTTIME?
  • While MacOS and FreeBSD might provide these different time sources, do we as quinn-udp have to support them? Asked differently, for what use-case are we designing this API? Does this use-case need to support all of these different timers?
  • Is one of these timer configurations the de-facto standard, while others are rather niche?

I don't have a strong opinion on this matter. I don't have experience with the syscalls. I don't see myself using it in the next couple of months, though I am eying it for future use in Firefox. Thus I will leave the final decision to the other maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

So @mxinden argued above that semantic differences (that is, in the value of timestamp) should be represented in the type system, which on the one hand seems reasonable, but on the other hand it seems unlikely that people are going to want to compare these values across operating systems?

I do agree that at least conceptually, the use of Duration as a timestamp is not the most intuitive. On the other hand, it seems like a somewhat reasonable representation of a timestamp relative to any kind of epoch. A u64 newtype (or even a secs: u64, nsecs: u64 newtype) doesn't really seem like an improvement, except for the name? I suppose we could wrap Duration in a newtype and implement Deref for it which seems like a low-complexity way to make this look a little more obvious?

As for which configuration/source is used, it seems like quinn-udp would want to select the single best available source for the platform -- which assumes there is a single metric for "best" that works for all relevant use cases; not sure I can think of a counterexample where that is not the case (maybe range vs accuracy?).

Copy link
Member

Choose a reason for hiding this comment

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

@recatek @mxinden should we find a way forward on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine moving forward here. My opinions above still stand. None of them are strong, i.e. please don't block on any of them.

There seems to be lots of open questions above, I assume due to us designing an API for all the use-cases instead of a specific one. May I suggest designing for a single use-case for now? Does a concrete use-case already exist? If so, would you mind sharing (some of) the source code using this API @recatek?

#[cfg(not(wasm_browser))]
pub timestamp: Option<Duration>,
}

impl Default for RecvMeta {
Expand All @@ -126,6 +133,8 @@ impl Default for RecvMeta {
stride: 0,
ecn: None,
dst_ip: None,
#[cfg(not(wasm_browser))]
timestamp: None,
}
}
}
Expand Down
32 changes: 31 additions & 1 deletion quinn-udp/src/unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ use std::{
time::Instant,
};

#[cfg(target_os = "linux")]
use std::time::Duration;

use socket2::SockRef;

use super::{
Expand Down Expand Up @@ -96,6 +99,11 @@ impl UdpSocketState {
unsafe { libc::CMSG_SPACE(mem::size_of::<libc::in6_pktinfo>() as _) as usize };
}

if cfg!(target_os = "linux") {
cmsg_platform_space +=
unsafe { libc::CMSG_SPACE(mem::size_of::<libc::timeval>() as _) as usize };
}

assert!(
CMSG_LEN
>= unsafe { libc::CMSG_SPACE(mem::size_of::<libc::c_int>() as _) as usize }
Expand Down Expand Up @@ -257,6 +265,18 @@ impl UdpSocketState {
self.may_fragment
}

/// Sets the socket to receive packet receipt timestamps from the operating system.
/// These can be accessed via [`RecvMeta::timestamp`] on packets when enabled.
#[cfg(target_os = "linux")]
pub fn set_recv_timestamping(&self, sock: UdpSockRef<'_>, enabled: bool) -> io::Result<()> {
let enabled = match enabled {
true => OPTION_ON,
false => OPTION_OFF,
};

set_socket_option(&*sock.0, libc::SOL_SOCKET, libc::SO_TIMESTAMPNS, enabled)
}

/// Returns true if we previously got an EINVAL error from `sendmsg` syscall.
fn sendmsg_einval(&self) -> bool {
self.sendmsg_einval.load(Ordering::Relaxed)
Expand Down Expand Up @@ -543,7 +563,7 @@ fn recv(io: SockRef<'_>, bufs: &mut [IoSliceMut<'_>], meta: &mut [RecvMeta]) ->
Ok(1)
}

const CMSG_LEN: usize = 88;
const CMSG_LEN: usize = 96;

fn prepare_msg(
transmit: &Transmit<'_>,
Expand Down Expand Up @@ -681,6 +701,8 @@ fn decode_recv(
let mut dst_ip = None;
#[allow(unused_mut)] // only mutable on Linux
let mut stride = len;
#[allow(unused_mut)] // only mutable on Linux
let mut timestamp = None;

let cmsg_iter = unsafe { cmsg::Iter::new(hdr) };
for cmsg in cmsg_iter {
Expand Down Expand Up @@ -725,6 +747,11 @@ fn decode_recv(
(libc::SOL_UDP, gro::UDP_GRO) => unsafe {
stride = cmsg::decode::<libc::c_int, libc::cmsghdr>(cmsg) as usize;
},
#[cfg(target_os = "linux")]
(libc::SOL_SOCKET, libc::SCM_TIMESTAMPNS) => {
let tv = unsafe { cmsg::decode::<libc::timespec, libc::cmsghdr>(cmsg) };
timestamp = Some(Duration::new(tv.tv_sec as u64, tv.tv_nsec as u32));
}
_ => {}
}
}
Expand Down Expand Up @@ -759,6 +786,7 @@ fn decode_recv(
addr,
ecn: EcnCodepoint::from_bits(ecn_bits),
dst_ip,
timestamp,
}
}

Expand Down Expand Up @@ -907,6 +935,8 @@ fn set_socket_option(
}
}

#[allow(dead_code)]
const OPTION_OFF: libc::c_int = 0;
const OPTION_ON: libc::c_int = 1;

#[cfg(not(any(target_os = "linux", target_os = "android")))]
Expand Down
1 change: 1 addition & 0 deletions quinn-udp/src/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ impl UdpSocketState {
addr: addr.unwrap(),
ecn: EcnCodepoint::from_bits(ecn_bits as u8),
dst_ip,
timestamp: None,
};
Ok(1)
}
Expand Down
68 changes: 68 additions & 0 deletions quinn-udp/tests/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use std::{
net::{IpAddr, Ipv4Addr, Ipv6Addr, UdpSocket},
slice,
};
#[cfg(target_os = "linux")]
use std::{mem::MaybeUninit, time::Duration};

use quinn_udp::{EcnCodepoint, RecvMeta, Transmit, UdpSocketState};
use socket2::Socket;
Expand Down Expand Up @@ -186,6 +188,72 @@ fn gso() {
);
}

#[test]
#[cfg(target_os = "linux")]
fn receive_timestamp() {
let send = UdpSocket::bind((Ipv6Addr::LOCALHOST, 0))
.or_else(|_| UdpSocket::bind((Ipv4Addr::LOCALHOST, 0)))
.unwrap();
let recv = UdpSocket::bind((Ipv6Addr::LOCALHOST, 0))
.or_else(|_| UdpSocket::bind((Ipv4Addr::LOCALHOST, 0)))
.unwrap();
let dst_addr = recv.local_addr().unwrap();

let send_state = UdpSocketState::new((&send).into()).unwrap();
let recv_state = UdpSocketState::new((&recv).into()).unwrap();

recv_state
.set_recv_timestamping((&recv).into(), true)
.expect("failed to set sockopt -- unsupported?");

// Reverse non-blocking flag set by `UdpSocketState` to make the test non-racy
recv.set_nonblocking(false).unwrap();

let mut buf = [0; u16::MAX as usize];
let mut meta = RecvMeta::default();

let mut time_start = MaybeUninit::<libc::timespec>::uninit();
let mut time_end = MaybeUninit::<libc::timespec>::uninit();

// Use the actual CLOCK_REALTIME clock source in linux, rather than relying on SystemTime
let errno = unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, time_start.as_mut_ptr()) };
assert_eq!(errno, 0, "failed to read CLOCK_REALTIME");
let time_start = unsafe { time_start.assume_init() };
let time_start = Duration::new(time_start.tv_sec as u64, time_start.tv_nsec as u32);

send_state
.try_send(
(&send).into(),
&Transmit {
destination: dst_addr,
ecn: None,
contents: b"hello",
segment_size: None,
src_ip: None,
},
)
.unwrap();

recv_state
.recv(
(&recv).into(),
&mut [IoSliceMut::new(&mut buf)],
slice::from_mut(&mut meta),
)
.unwrap();

let errno = unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, time_end.as_mut_ptr()) };
assert_eq!(errno, 0, "failed to read CLOCK_REALTIME");
let time_end = unsafe { time_end.assume_init() };
let time_end = Duration::new(time_end.tv_sec as u64, time_end.tv_nsec as u32);

// Note: there's a very slim chance that the clock could jump (via ntp, etc.) and throw off
// these two checks, but it's still better to validate the timestamp result with that risk
let timestamp = meta.timestamp.unwrap();
assert!(time_start <= timestamp);
assert!(timestamp <= time_end);
}

fn test_send_recv(send: &Socket, recv: &Socket, transmit: Transmit) {
let send_state = UdpSocketState::new(send.into()).unwrap();
let recv_state = UdpSocketState::new(recv.into()).unwrap();
Expand Down