Hi Willem,
On 2026-11-01 at 09:57 -05, Willem de Bruijn <[email protected]> wrote: > Steffen Trumtrar wrote: > > This series picks up Willem de Bruijn's work on a virtio-net receive > > timestamp. The device puts the time of arrival (TAI, ns) into the > > virtio-net header so the guest can use it for PTP and RTT estimation > > without taking the timestamp itself after the host-to-guest latency. > > > > The series is an RFC. I am mostly interested in feedback on the > > approach and on the wire format: > > At a high level +1 from me. Thanks! > > Some comments inline. > > > > - The timestamp is a new field at the end of virtio_net_common_hdr > > after outer_nh_offset and ipsec, offset 32 (see virtio spec), > > negotiated with VIRTIO_NET_F_RX_TSTAMP (bit 49). Header size now 40 > > bytes. > > Willem noted on v3 that virtio_net_common_hdr was not meant to be > > extended this way; I kept it as the simplest approach, but a > > separate struct for the timestamp header is an option. > > - Timestamping is enabled at runtime with SIOCSHWTSTAMP; all > > supported rx filters behave the same for now. > > - XDP programs can read the timestamp with > > bpf_xdp_metadata_rx_timestamp(). > > - vhost-net passes the feature through; the timestamp itself is > > written by tun. The tun patch is only meant as a reference > > implementation for the device side. > > - The timestamp is in TAI. > > Is this a requirement? It is common for PHCs to be synced to TAI, > but not 100% the case. Does virtio need to specify this. > No, I don't think it has to be TAI. TAI is only what the tun reference implementation does (ktime_get_clocktai_ns()). What the guest really needs is to know which clock the timestamp comes from, so it can compare it with something. One idea for the spec change: don't fix the time base in virtio-net at all, only say the timestamp is in nanoseconds of some device clock, and let the device tell the driver which clock that is. virtio_rtc already has clock ids with a type (UTC, TAI, monotonic, ...), and the Linux driver registers a PHC for each of them (with VIRTIO_RTC_PTP). virtio-net could carry the virtio_rtc clock id in its config space, and the driver reports the index of that PHC in get_ts_info. That would also fix the phc_index == -1 problem you pointed out in 2/6. What I don't like about it: virtio-net then depends on a second device being there, and with more than one virtio_rtc device the clock id alone is not enough to find the right one. Without virtio_rtc the driver would not offer hardware timestamps at all. The alternative is a PHC in virtio-net itself (gettime over the control vq), which is a lot more spec for one timestamp field. Do you think the virtio_rtc way is reasonable, or is there something simpler I'm missing? > > The driver reports it as > > SOF_TIMESTAMPING_RAW_HARDWARE without a PTP clock (phc_index -1), > > so userspace cannot tell which clock it refers to and may compare > > it against CLOCK_REALTIME (currently off by 37s). Options I see: > > convert to the guest's system time in the driver (correct only if > > host and guest agree on the TAI offset), expose a PHC so the > > time base is explicit, or keep the raw TAI value and leave the > > conversion to userspace. Which one is preferred? > > - Like the existing rx_hash, the XDP timestamp callback locates the > > header relative to xdp->data, so it must be called before > > bpf_xdp_adjust_head(). > > > > A virtio spec change is needed as well and not yet proposed. > > > > Testing: the feature needs a QEMU that knows VIRTIO_NET_F_RX_TSTAMP and > > sets the matching vnet header size; see > > https://github.com/strumtrar/qemu/tree/v11.0.1/virtio-rx-stamps > > > > Performance: I measured the receive rate with 64 byte UDP packets in > > a nested QEMU/KVM setup with vhost-net. The variance between runs is > > larger than any difference between the variants (without the series, > > series without the feature, feature negotiated, feature enabled), so > > I cannot show a measurable cost or the absence of one. Numbers from > > bare metal will follow with a non-RFC version. > > This does not seem performance sensitive to me. And it is opt-in. > IMHO no need for extensive testing for performance regressions. > Performance measurements where requested in some of the previous versions because of the increased header size. But now, as this is really opt-in, I'll drop that part from the cover letter. Steffen -- Pengutronix e.K. | Dipl.-Inform. Steffen Trumtrar | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 |
