Adding packet receipt timestamp collection into RecvMeta for linux

[recatek]

No description provided.

[djc]

Mostly LGTM, please squash into a single commit.

[mxinden]

I am in favor of this feature. Thanks for the work.

[recatek]

I changed the timestamp type a bit to be an enum now based on the clock and type it's from. This way it'll ZST out on platforms that don't support it, but be easier to add going forward (Windows kinda supports a form of this, and FreeBSD supports both realtime and monotonic clock timestamping).

[djc]

(And don't forget to squash your changes.)

[mxinden]

Using SystemTime sounds like the reasonable choice to me.

I don't understand the benefit of nesting a SystemTime in an additional enum. Can you expand @recatek?

[recatek]

I don't understand the benefit of nesting a SystemTime in an additional enum. Can you expand @recatek?

I'm eyeing future support for FreeBSD (which I am considering using for one use case). FreeBSD has a SO_TS_CLOCK sockopt that lets you set SO_TS_MONOTONIC to receive timestamps from CLOCK_MONOTONIC instead of CLOCK_REALTIME. In this case we'd want a different type in there (ideally Instant, but probably another time type like MonotonicTime since Instant "can't" be constructed).

So if/when I propose that change, it would look like

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

It's a good call to make RecvTime #[non_exhaustive] -- I'll do that as well.

[Ralith]

Would it be simpler and sufficient to document that the clock/epoch is unspecified? Is comparing receipt times to times obtained by other means important? IIRC your application only needed to compare receipt times to eachother.

[recatek]

I'm using the receipt times to modify the Instant that I provide to quinn-proto's now Instant argument, to get more accurate ack_delay calculation. Example here. This isn't the best example since Instant::now() will be pretty accurate in this case due to using mio, but the idea going forward is to use this value to correct for dead times once my recv() calls happen less frequently.

[Ralith]

I'm not sure if that's necessary or correct. ACK delay describes the time that the QUIC endpoint deliberately waits, not delays forced on it by lower layers or intermediaries. Any time you pass a now in the past into a Quinn API, you're risking that Quinn will expect to be able to schedule additional work at a time that it thinks is the future, but which is actually the past, causing obscure performance degradation elsewhere.

[recatek]

Ah, yes, you're right. I missed the distinction between intentional delays and unintentional delays in the spec. However, it's still important to be able to compare the timestamps with a now() call (be it SystemTime, Instant, or some clock_gettime). You need to be able to compare against now() at send time if you're doing your own receipt-timestamp-aware RTT calculations on top of QUIC. With that in mind, it's important to control/specify what clock those packet timestamps are sourced from rather than an arbitrary epoch.

[recatek]

After discussion on Discord, I converted the timestamp back to a Duration. It'll make it easier to interface with quinn-proto in my next PR for getting the timestamp info into Datagrams

[mxinden]

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?

[recatek]

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.

[recatek]

Added a test as well.