From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0C1964591C; Fri, 6 Sep 2024 10:13:29 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9382442EBB; Fri, 6 Sep 2024 10:13:28 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 2951A4029E for ; Fri, 6 Sep 2024 10:13:27 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id E763520CA1; Fri, 6 Sep 2024 10:13:26 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH] net/af_packet: add timestamp offloading support Date: Fri, 6 Sep 2024 10:13:22 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F6AE@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] net/af_packet: add timestamp offloading support Thread-Index: AQHa/faIgXyAWopV9kGrv4mtbbKJvrJGPscAgABQ2QCAA7yKkIAAHJmQ References: <20240903114306.2336633-1-stefan.laesser@omicronenergy.com> <20240903092138.0e071924@hermes.local> <98CBD80474FA8B44BF855DF32C47DC35E9F698@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: =?iso-8859-1?Q?Stefan_L=E4sser?= , "Stephen Hemminger" Cc: "Thomas Monjalon" , "John W. Linville" , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Stefan L=E4sser [mailto:stefan.laesser@omicronenergy.com] > Sent: Friday, 6 September 2024 08.23 >=20 > > > 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 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 > > > > --- > > > > .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. >=20 > Yes, I use the generic timestamp dynamic field as used in all the = other PMDs I > have looked at. >=20 > > > > > > > > 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? >=20 > 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. 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.