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. 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. > 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. > Signed-off-by: Steffen Trumtrar <[email protected]> > --- > Changes in v4: > - zero the header beyond the known fields on tx and in vhost > - grow padded_vnet_hdr for the 40 byte header (big packets) > - install xdp_metadata_ops if only RX_TSTAMP is negotiated > - rename VIRTIO_NET_F_TSTAMP to VIRTIO_NET_F_RX_TSTAMP > - move tstamp to offset 32 (header size 40) to match virtio spec layout > - drop XDP restriction for SIOCSHWTSTAMP, add xmo_rx_timestamp > - add static_asserts for header sizes > - add support in vhost > - move virtio_net_common_hdr to virtio_net.h > - Link to v3: > https://patch.msgid.link/20260921-v6-7-topic-virtio-net-ptp-v3-0-3350851c4...@pengutronix.de > Changes in v3: > - rework to use virtio_net_common_hdr instead of adding new struct > Changes in v2: > - rework patches to use flow filter instead of feature flag > - Link to v1: > https://lore.kernel.org/r/20231218-v6-7-topic-virtio-net-ptp-v1-0-cac92b2d8...@pengutronix.de > > To: Michael S. Tsirkin <[email protected]> > To: Xuan Zhuo <[email protected]> > To: David S. Miller <[email protected]> > To: Jakub Kicinski <[email protected]> > To: Paolo Abeni <[email protected]> > To: Richard Cochran <[email protected]> > To: Willem de Bruijn <[email protected]> > To: Andrew Lunn <[email protected]> > To: Eugenio Pérez <[email protected]> > To: Jason Wang <[email protected]> > To: Alexei Starovoitov <[email protected]> > To: Daniel Borkmann <[email protected]> > To: Jesper Dangaard Brouer <[email protected]> > To: John Fastabend <[email protected]> > To: Stanislav Fomichev <[email protected]> > To: Eric Dumazet <[email protected]> > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > > --- > Steffen Trumtrar (6): > virtio-net: move virtio_net_common_hdr to virtio_net.h > virtio-net: support receive timestamp > virtio-net: assert header sizes > vhost-net: support VIRTIO_NET_F_RX_TSTAMP > virtio-net: add XDP rx timestamp metadata > tun: add rx timestamp to the vnet header > > drivers/net/tun.c | 14 ++-- > drivers/net/tun_vnet.h | 18 +++-- > drivers/net/virtio_net.c | 171 > ++++++++++++++++++++++++++++++++++------ > drivers/vhost/net.c | 17 +++- > include/linux/virtio_net.h | 30 +++++++ > include/uapi/linux/virtio_net.h | 1 + > 6 files changed, 214 insertions(+), 37 deletions(-) > --- > base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222 > change-id: 20231218-v6-7-topic-virtio-net-ptp-3df023bc4f4d > > Best regards, > -- > Steffen Trumtrar <[email protected]> >
