DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] net/af_packet: add timestamp offloading support
@ 2024-09-03 11:43 Stefan Laesser
  2024-09-03 16:21 ` Stephen Hemminger
  2024-09-11  6:59 ` Morten Brørup
  0 siblings, 2 replies; 7+ messages in thread
From: Stefan Laesser @ 2024-09-03 11:43 UTC (permalink / raw)
  To: Thomas Monjalon, John W. Linville; +Cc: dev, Stefan Laesser

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(-)

diff --git a/.mailmap b/.mailmap
index 4a508bafad..69314e34a0 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1414,6 +1414,7 @@ Steeven Lee <steeven@gmail.com>
 Steeven Li <steeven.li@broadcom.com>
 Stefan Baranoff <sbaranoff@gmail.com>
 Stefan Hajnoczi <stefanha@redhat.com>
+Stefan Laesser <stefan.laesser@omicronenergy.com>
 Stefan Puiu <stefan.puiu@gmail.com>
 Stefan Wegrzyn <stefan.wegrzyn@intel.com>
 Stepan Sojka <stepan.sojka@adaptivemobile.com>
diff --git a/doc/guides/nics/af_packet.rst b/doc/guides/nics/af_packet.rst
index 66b977e1a2..e5da625e09 100644
--- a/doc/guides/nics/af_packet.rst
+++ b/doc/guides/nics/af_packet.rst
@@ -69,6 +69,8 @@ framecnt=512):
 Features and Limitations
 ------------------------
 
-The PMD will re-insert the VLAN tag transparently to the packet if the kernel
-strips it, as long as the ``RTE_ETH_RX_OFFLOAD_VLAN_STRIP`` is not enabled by the
-application.
+*  The PMD will re-insert the VLAN tag transparently to the packet if the kernel
+   strips it, as long as the ``RTE_ETH_RX_OFFLOAD_VLAN_STRIP`` is not enabled by the
+   application.
+*  The PMD will add the kernel packet timestamp with nanoseconds resolution if
+   ``RTE_ETH_RX_OFFLOAD_TIMESTAMP`` is enabled.
diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 6b7b16f348..6a7d5d0ffb 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -40,6 +40,9 @@
 #define DFLT_FRAME_SIZE		(1 << 11)
 #define DFLT_FRAME_COUNT	(1 << 9)
 
+uint64_t af_packet_timestamp_dynflag;
+int af_packet_timestamp_dynfield_offset = -1;
+
 struct __rte_cache_aligned pkt_rx_queue {
 	int sockfd;
 
@@ -51,6 +54,7 @@ struct __rte_cache_aligned pkt_rx_queue {
 	struct rte_mempool *mb_pool;
 	uint16_t in_port;
 	uint8_t vlan_strip;
+	uint8_t timestamp_offloading;
 
 	volatile unsigned long rx_pkts;
 	volatile unsigned long rx_bytes;
@@ -82,6 +86,7 @@ struct pmd_internals {
 	struct pkt_rx_queue *rx_queue;
 	struct pkt_tx_queue *tx_queue;
 	uint8_t vlan_strip;
+	uint8_t timestamp_offloading;
 };
 
 static const char *valid_arguments[] = {
@@ -157,6 +162,16 @@ eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 				PMD_LOG(ERR, "Failed to reinsert VLAN tag");
 		}
 
+		/* add kernel provided timestamp when offloading is enabled */
+		if (pkt_q->timestamp_offloading) {
+			/* since TPACKET_V2 timestamps are provided in nanoseconds resolution */
+			*RTE_MBUF_DYNFIELD(mbuf, af_packet_timestamp_dynfield_offset,
+				rte_mbuf_timestamp_t *) =
+					(uint64_t)ppd->tp_sec * 1000000000 + ppd->tp_nsec;
+
+			mbuf->ol_flags |= af_packet_timestamp_dynflag;
+		}
+
 		/* release incoming frame and advance ring buffer */
 		ppd->tp_status = TP_STATUS_KERNEL;
 		if (++framenum >= framecount)
@@ -315,13 +330,25 @@ static int
 eth_dev_start(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = dev->data->dev_private;
-	uint16_t i;
+
+	if (internals->timestamp_offloading) {
+		/* Register mbuf field and flag for Rx timestamp */
+		int rc = rte_mbuf_dyn_rx_timestamp_register(&af_packet_timestamp_dynfield_offset,
+				&af_packet_timestamp_dynflag);
+		if (rc) {
+			PMD_LOG(ERR, "Cannot register mbuf field/flag for timestamp");
+			return rc;
+		}
+	}
 
 	dev->data->dev_link.link_status = RTE_ETH_LINK_UP;
+
+	uint16_t i;
 	for (i = 0; i < internals->nb_queues; i++) {
 		dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
 		dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
 	}
+
 	return 0;
 }
 
@@ -365,6 +392,7 @@ eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 	struct pmd_internals *internals = dev->data->dev_private;
 
 	internals->vlan_strip = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP);
+	internals->timestamp_offloading = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP);
 	return 0;
 }
 
@@ -381,7 +409,8 @@ eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->min_rx_bufsize = 0;
 	dev_info->tx_offload_capa = RTE_ETH_TX_OFFLOAD_MULTI_SEGS |
 		RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
-	dev_info->rx_offload_capa = RTE_ETH_RX_OFFLOAD_VLAN_STRIP;
+	dev_info->rx_offload_capa = RTE_ETH_RX_OFFLOAD_VLAN_STRIP |
+		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
 
 	return 0;
 }
@@ -508,6 +537,7 @@ eth_rx_queue_setup(struct rte_eth_dev *dev,
 	dev->data->rx_queues[rx_queue_id] = pkt_q;
 	pkt_q->in_port = dev->data->port_id;
 	pkt_q->vlan_strip = internals->vlan_strip;
+	pkt_q->timestamp_offloading = internals->timestamp_offloading;
 
 	return 0;
 }
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/af_packet: add timestamp offloading support
  2024-09-03 11:43 [PATCH] net/af_packet: add timestamp offloading support Stefan Laesser
@ 2024-09-03 16:21 ` Stephen Hemminger
  2024-09-03 21:11   ` Morten Brørup
  2024-09-11  6:59 ` Morten Brørup
  1 sibling, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2024-09-03 16:21 UTC (permalink / raw)
  To: Stefan Laesser; +Cc: Thomas Monjalon, John W. Linville, dev

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.

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.  Also, having
standard helpers in ethdev for the conversion from TSC to NS would
help.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_packet: add timestamp offloading support
  2024-09-03 16:21 ` Stephen Hemminger
@ 2024-09-03 21:11   ` Morten Brørup
  2024-09-06  6:22     ` Stefan Lässer
  0 siblings, 1 reply; 7+ messages in thread
From: Morten Brørup @ 2024-09-03 21:11 UTC (permalink / raw)
  To: Stephen Hemminger, Stefan Laesser; +Cc: Thomas Monjalon, John W. Linville, dev

> 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.

> 
> 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?


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_packet: add timestamp offloading support
  2024-09-03 21:11   ` Morten Brørup
@ 2024-09-06  6:22     ` Stefan Lässer
  2024-09-06  8:13       ` Morten Brørup
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Lässer @ 2024-09-06  6:22 UTC (permalink / raw)
  To: Morten Brørup, Stephen Hemminger
  Cc: Thomas Monjalon, John W. Linville, dev

> > 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?
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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_packet: add timestamp offloading support
  2024-09-06  6:22     ` Stefan Lässer
@ 2024-09-06  8:13       ` Morten Brørup
  2024-09-11  5:44         ` Stefan Lässer
  0 siblings, 1 reply; 7+ messages in thread
From: Morten Brørup @ 2024-09-06  8:13 UTC (permalink / raw)
  To: Stefan Lässer, Stephen Hemminger
  Cc: Thomas Monjalon, John W. Linville, dev

> 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.

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.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_packet: add timestamp offloading support
  2024-09-06  8:13       ` Morten Brørup
@ 2024-09-11  5:44         ` Stefan Lässer
  0 siblings, 0 replies; 7+ messages in thread
From: Stefan Lässer @ 2024-09-11  5:44 UTC (permalink / raw)
  To: Morten Brørup, Stephen Hemminger
  Cc: Thomas Monjalon, John W. Linville, dev

> > 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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_packet: add timestamp offloading support
  2024-09-03 11:43 [PATCH] net/af_packet: add timestamp offloading support Stefan Laesser
  2024-09-03 16:21 ` Stephen Hemminger
@ 2024-09-11  6:59 ` Morten Brørup
  1 sibling, 0 replies; 7+ messages in thread
From: Morten Brørup @ 2024-09-11  6:59 UTC (permalink / raw)
  To: Stefan Laesser; +Cc: dev, stephen, Thomas Monjalon, John W. Linville

> From: Stefan Laesser [mailto:stefan.laesser@omicronenergy.com]
> Sent: Tuesday, 3 September 2024 13.43
> 
> 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.

Suggest adding: and UNIX origo, i.e. time since 1-JAN-1970 UTC.

> 
> Signed-off-by: Stefan Laesser <stefan.laesser@omicronenergy.com>
> ---


> +uint64_t af_packet_timestamp_dynflag;
> +int af_packet_timestamp_dynfield_offset = -1;

No need to expose these publicly, they should be static.
This also means that you can remove the af_packet_ prefix.


>  eth_dev_start(struct rte_eth_dev *dev)
>  {
>  	struct pmd_internals *internals = dev->data->dev_private;
> -	uint16_t i;
> +
> +	if (internals->timestamp_offloading) {
> +		/* Register mbuf field and flag for Rx timestamp */
> +		int rc =
> rte_mbuf_dyn_rx_timestamp_register(&af_packet_timestamp_dynfield_offset,
> +				&af_packet_timestamp_dynflag);
> +		if (rc) {
> +			PMD_LOG(ERR, "Cannot register mbuf field/flag for
> timestamp");
> +			return rc;
> +		}
> +	}
> 
>  	dev->data->dev_link.link_status = RTE_ETH_LINK_UP;
> +
> +	uint16_t i;

No need to move this down here. Please leave it where it was.

Alternatively, minimize its scope by moving it inside the for loop:
-	for (i = 0; i < internals->nb_queues; i++) {
+	for (uint16_t i = 0; i < internals->nb_queues; i++) {


With the two variables made local,

Acked-by: Morten Brørup <mb@smartsharesystems.com>


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-09-11  6:59 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-03 11:43 [PATCH] net/af_packet: add timestamp offloading support Stefan Laesser
2024-09-03 16:21 ` Stephen Hemminger
2024-09-03 21:11   ` Morten Brørup
2024-09-06  6:22     ` Stefan Lässer
2024-09-06  8:13       ` Morten Brørup
2024-09-11  5:44         ` Stefan Lässer
2024-09-11  6:59 ` Morten Brørup

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).