Re: [PATCH RFC v4 0/6] virtio-net: add receive timestamp support

[Willem de Bruijn]

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 <s.trumtrar@xxxxxxxxxxxxxx>
> ---
> 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-3350851c4f88@xxxxxxxxxxxxxx
> 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-cac92b2d8532@xxxxxxxxxxxxxx
> 
> To: Michael S. Tsirkin <mst@xxxxxxxxxx>
> To: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> To: David S. Miller <davem@xxxxxxxxxxxxx>
> To: Jakub Kicinski <kuba@xxxxxxxxxx>
> To: Paolo Abeni <pabeni@xxxxxxxxxx>
> To: Richard Cochran <richardcochran@xxxxxxxxx>
> To: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> To: Andrew Lunn <andrew+netdev@xxxxxxx>
> To: Eugenio Pérez <eperezma@xxxxxxxxxx>
> To: Jason Wang <jasowangio@xxxxxxxxx>
> To: Alexei Starovoitov <ast@xxxxxxxxxx>
> To: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> To: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
> To: John Fastabend <john.fastabend@xxxxxxxxx>
> To: Stanislav Fomichev <sdf@xxxxxxxxxxx>
> To: Eric Dumazet <edumazet@xxxxxxxxxx>
> Cc: virtualization@xxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: bpf@xxxxxxxxxxxxxxx
> Cc: kvm@xxxxxxxxxxxxxxx
> 
> ---
> 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 <s.trumtrar@xxxxxxxxxxxxxx>
>