Re: [PATCH RESEND 3/4] virtio-net: support transmit ptp timestamps

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

Hi,

On 2024-06-25 at 05:25 GMT, Parav Pandit <parav@nvidia.com> wrote:

> Hi Steffen,
> 
> > From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Sent: Monday, June 24, 2024 4:39 PM
> > To: virtio-comment@lists.linux.dev; kernel@pengutronix.de
> > Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Subject: [PATCH RESEND 3/4] virtio-net: support transmit ptp timestamps
> > 
> > Add optional PTP hardware tx timestamp offload for virtio-net.
> > 
> Very good initiate. Thanks for bringing timestamping to virtio.
> 
> > Accurate RTT measurement requires timestamps close to the wire.
> > Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit equivalent
> > to VIRTIO_NET_F_RX_TSTAMP.
> > 
> > The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
> > returned on completion. If the feature is negotiated, the device either places
> > the timestamp or clears the feature bit.
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >  device-types/net/description.tex | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/device-types/net/description.tex b/device-
> > types/net/description.tex
> > index 09d9c28..c438b0b 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -88,6 +88,8 @@ \subsection{Feature bits}\label{sec:Device Types /
> > Network Device / Feature bits  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)]
> > Set MAC address through control
> >      channel.
> > 
> > +\item[VIRTIO_NET_F_TX_TSTAMP(47)] Device sends TAI transmit time
> > +
> The feature bit is not helpful.
> The reason is, the user needs to enable timestamp much later using ethtool (Linux) SIOCSHWTSTAMP.
> And it is desired to not reset the full device when changing this feature.
> 
> And when the feature is offered, it is also not desire to always performing timestamping as it has trade-offs.
> 
> Therefore, enablement of this functionality should be done using capabilities or other means dynamically.
> Dynamic means = without re-initializing the device and features.

okay, I understand that.

> 
> >  \item[VIRTIO_NET_F_RX_TSTAMP(48)] Device sends TAI receive time
> > 
> >  \item[VIRTIO_NET_F_TX_HASH(49)] Driver sends hash report @@ -532,6
> > +534,12 @@ \subsubsection{Packet Transmission}\label{sec:Device Types /
> > Network Device / De  \item The header and packet are added as one output
> > descriptor to the
> >    transmitq, and the device is notified of the new entry
> >    (see \ref{sec:Device Types / Network Device / Device
> > Initialization}~\nameref{sec:Device Types / Network Device / Device
> > Initialization}).
> > +
> > +\item If the driver negotiated the VIRTIO_NET_F_TX_TSTAMP and hardware
> > +  timestamping is supported on the device, the
> > VIRTIO_NET_HDR_F_TSTAMP
> > +  flag is set. One writable input descriptor with the size of a
> > +timestamp,
> > +  i.e. \field{tstamp} in virtio_net_hdr_hash_ts,
> > +  is appended to the output descriptor.
> >  \end{enumerate}
> > 
> This does not work any effectively for following reasons.
> 1. a TS is 8B, it is pointless to pass another 16B descriptor for 8B of data. This is 200% overhead.
> 2. a TS write of 8B also creates a cache line bounding due to partial writes even in modern cpus
> 3. Apart from the short write, because it is a dedicated address, the DMA cannot be merged by the device for it with something else.
> 
> In short, a dedicated descriptor for TS timestamp or including this TS in vnet_hdr is not good idea.

And this sounds reasonable, too. Understood.
> 
> Many of us already discuss the new descriptor format that solves many of these issues.
> i.e. to have a transmit completion which contains all needed fields including TS that also overcomes above 3 issues.
> 
> We captured these requirements at [1]. Please refer to 3.5.1 point 3.
> It would be good if you can extend the descriptors this way that can work for hw.
> 
> [1] https://lore.kernel.org/virtio-comment/20230818043557.496964-7-parav@nvidia.com/
> 

Here I'm a little bit confused. So you want me to describe the feature in terms of the vnet_rx_completion or vnet_tx_completion that is, which is not described yet? For the spec part this seems "easy" but the actual implementation sounds a "little bit" more involved.

Do you know, if there are already patches floating around for virtio-net that implement this new specification. I'd like to test it in code before writing the documentation/spec. 


Best regards,
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    |