RE: [PATCH] net/af_packet: add timestamp offloading support

["Stefan Lässer" <stefan.laesser@omicronenergy.com>]

> > From: Stefan Lässer [mailto:stefan.laesser@omicronenergy.com]
> > Sent: Friday, 6 September 2024 08.23
> >
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, 3 September 2024 18.22
> > > >
> > > > On Tue, 3 Sep 2024 13:43:06 +0200
> > > > Stefan Laesser <stefan.laesser@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.laesser@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?