> > From: Stefan Lässer [mailto:stefan.laes...@omicronenergy.com] > > Sent: Friday, 6 September 2024 08.23 > > > > > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > > > Sent: Tuesday, 3 September 2024 18.22 > > > > > > > > On Tue, 3 Sep 2024 13:43:06 +0200 > > > > Stefan Laesser <stefan.laes...@omicronenergy.com> wrote: > > > > > > > > > Add the packet timestamp from TPACKET_V2 to the mbuf dynamic rx > > > > > timestamp register if offload RTE_ETH_RX_OFFLOAD_TIMESTAMP is > > > > > enabled. > > > > > > > > > > TPACKET_V2 provides the timestamp with nanosecond resolution. > > > > > > > > > > Signed-off-by: Stefan Laesser <stefan.laes...@omicronenergy.com> > > > > > --- > > > > > .mailmap | 1 + > > > > > doc/guides/nics/af_packet.rst | 8 ++++-- > > > > > drivers/net/af_packet/rte_eth_af_packet.c | 34 > > > > > +++++++++++++++++++++- > > > > - > > > > > 3 files changed, 38 insertions(+), 5 deletions(-) > > > > > > > > Adding timestamp is good, but it would be better if the timestamp > > > > field was generic. The pcap PMD also has a timestamp, and pdump > > > > API could/should use timestamp as well. > > > > > > As far as I can see, this patch does use the existing > > > cross-driver/generic timestamp dynamic field, like the pcap driver. > > > > Yes, I use the generic timestamp dynamic field as used in all the > > other PMDs I have looked at. > > > > > > > > > > > > > What makes sense is for there to be a standard dynamic field for > > > > nanosecond resolution timestamp, and add a make sure that all > > > > drivers use the same base 1/1/1970 same as Linux/Unix. > > > > > > Yes, standardizing on nanosecond resolution and a common base might > > > have been a better choice than using driver-specific units for the > > > generic timestamp dynamic field. > > > If the driver can use the NIC's native clock system, the driver > > > doesn't need > > to > > > convert to nanoseconds, which has a performance cost. > > > However, I suppose any application using timestamps needs to do this > > > conversion in the application instead, so the total performance is > > > the same > > as > > > if the drivers did it. I.e. from a performance perspective, the > > > drivers > > might as > > > well do the conversion, and from a usability perspective, it would > > > be easier with a standard unit and base. > > > > > > We should define a roadmap towards dynamic mbuf field timestamps > > > using fixed unit and base (instead of driver-specific) and migrate > > > towards it. > > > > > > Perhaps start by adding an ethdev capability flag, > > > RTE_ETH_RX_OFFLOAD_TIMESTAMP_NS used together with > > > RTE_ETH_RX_OFFLOAD_TIMESTAMP to indicate that the timestamp unit > and > > > base follows a common standard, i.e. nanoseconds since UNIX epoch. > > > > > > There may be other considerations, though: The NIC's clock may drift > > > compared to the CPU's clock, and compared to the clock of other NICs > > > in the same system. So the "base" and "nanoseconds" will still be > > > using the NIC's clock as reference, and it might be way out of sync with > > > the > CPU's clock. > > > > > > > Also, having > > > > standard helpers in ethdev for the conversion from TSC to NS would > > > > help. > > > > > > Helpers to convert from CPU TSC to nanoseconds have broader scope > > > than ethdev and belong in the EAL, perhaps in > > > /lib/eal/include/generic/rte_cycles.h? > > > > Should I extend my patch to include the new > > RTE_ETH_RX_OFFLOAD_TIMESTAMP_NS capability? > > That would be nice, but not a requirement. :-) > > Please do it as a series of patches, maybe three: > 1. This patch. > 2. A patch to generally introduce TIMESTAMP_NS RX offload and capability > flags. > 3. A patch to implement TIMESTAMP_NS in af_packet.
I will give it a try. :-) > The new TIMESTAMP_NS feature might trigger some discussions, and you don't > want this patch caught up too much in that discussion. > > > What happens if the user only enables RTE_ETH_RX_OFFLOAD_TIMESTAMP in > > the AF_PACKET PMD? > > I would suggest that in this case the timestamp will have microsecond > > accuracy and only if RTE_ETH_RX_OFFLOAD_TIMESTAMP_NS is also enabled, > > then the timestamp will have nanosecond accuracy. > > There's no need for different timestamp accuracy if TIMESTAMP_NS is not > enabled. > RTE_ETH_RX_OFFLOAD_TIMESTAMP means that a timestamp is present, with > driver dependent clock and base. > The driver is allowed to use nanoseconds as clock and UNIX origo as base, > regardless. > Okay - so the purpose of RTE_ETH_RX_OFFLOAD_TIMESTAMP_NS is just to signal that the PMD delivers timestamps with nanoseconds resolution - I see. What about this patch: are the changes valid / good enough to become part of the mainline version?
