DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5
@ 2020-07-24 20:23 Patrick Keroulas
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 1/4] net/mlx5: query device frequency Patrick Keroulas
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Patrick Keroulas @ 2020-07-24 20:23 UTC (permalink / raw)
  To: dev; +Cc: Patrick Keroulas

The intention is to produce a pcap with nanosecond precision when
Rx timestamp offloading is activated on mlx5 NIC.

The packets forwarded by testpmd hold the raw counter but a pcap
requires a time unit. Assuming that the NIC clock is already synced
with external master clock, this patchset simply integrates the
nanosecond converter that derives from device frequency and start time.

v2 -> v3:
    - replace ib_verbs nanosecond converter with more generic method
      based on device frequency and start time.

Patrick Keroulas (3):
  net/mlx5: query device frequency
  ethdev: add API to query device frequency
  pdump: convert timestamp to nanoseconds on Rx path

Vivien Didelot (1):
  net/pcap: support hardware Tx timestamps

 doc/guides/rel_notes/release_20_08.rst   |  1 +
 drivers/common/mlx5/mlx5_devx_cmds.c     |  2 ++
 drivers/common/mlx5/mlx5_devx_cmds.h     |  1 +
 drivers/net/mlx5/linux/mlx5_ethdev_os.c  | 22 ++++++++++++++++
 drivers/net/mlx5/linux/mlx5_os.c         |  1 +
 drivers/net/mlx5/mlx5.h                  |  1 +
 drivers/net/pcap/rte_eth_pcap.c          | 32 +++++++++++++-----------
 lib/librte_ethdev/rte_ethdev.c           | 12 +++++++++
 lib/librte_ethdev/rte_ethdev.h           | 17 +++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      |  5 ++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 ++
 lib/librte_pdump/rte_pdump.c             | 27 ++++++++++++++++++++
 12 files changed, 109 insertions(+), 14 deletions(-)

-- 
2.17.1


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

* [dpdk-dev] [[PATCH v3 1/4] net/mlx5: query device frequency
  2020-07-24 20:23 [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Patrick Keroulas
@ 2020-07-24 20:23 ` Patrick Keroulas
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 2/4] ethdev: add API to " Patrick Keroulas
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Patrick Keroulas @ 2020-07-24 20:23 UTC (permalink / raw)
  To: dev; +Cc: Patrick Keroulas

Get clock frequency (constant) from HCA attributes and add an accessor.

Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
---
 drivers/common/mlx5/mlx5_devx_cmds.c    |  2 ++
 drivers/common/mlx5/mlx5_devx_cmds.h    |  1 +
 drivers/net/mlx5/linux/mlx5_ethdev_os.c | 22 ++++++++++++++++++++++
 drivers/net/mlx5/linux/mlx5_os.c        |  1 +
 drivers/net/mlx5/mlx5.h                 |  1 +
 5 files changed, 27 insertions(+)

diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
index d1c674c7cf..e6671e3122 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.c
+++ b/drivers/common/mlx5/mlx5_devx_cmds.c
@@ -673,6 +673,8 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
 			MLX5_GET(cmd_hca_cap, hcattr, flow_counter_bulk_alloc);
 	attr->flow_counters_dump = MLX5_GET(cmd_hca_cap, hcattr,
 					    flow_counters_dump);
+	attr->device_frequency_khz = MLX5_GET(cmd_hca_cap, hcattr,
+					    device_frequency_khz);
 	attr->log_max_rqt_size = MLX5_GET(cmd_hca_cap, hcattr,
 					  log_max_rqt_size);
 	attr->eswitch_manager = MLX5_GET(cmd_hca_cap, hcattr, eswitch_manager);
diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
index 528cb7bdd1..cbe450c4b1 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.h
+++ b/drivers/common/mlx5/mlx5_devx_cmds.h
@@ -87,6 +87,7 @@ struct mlx5_hca_attr {
 	uint32_t lro_timer_supported_periods[MLX5_LRO_NUM_SUPP_PERIODS];
 	uint32_t flex_parser_protocols;
 	uint32_t hairpin:1;
+	uint32_t device_frequency_khz:20;
 	uint32_t log_max_hairpin_queues:5;
 	uint32_t log_max_hairpin_wq_data_sz:5;
 	uint32_t log_max_hairpin_num_packets:5;
diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
index 3d3dd2e862..628a909c8d 100644
--- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
+++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
@@ -301,6 +301,28 @@ mlx5_read_clock(struct rte_eth_dev *dev, uint64_t *clock)
 	return 0;
 }
 
+/**
+ * Get the clock frequency of ethernet device, in Hz
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param[out] freq
+ *   Pointer to the device clock frequency.
+ *
+ * @return
+ *   0 if the clock has correctly been read
+ *   The value of errno in case of error
+ */
+int
+mlx5_get_clock_freq(struct rte_eth_dev *dev, uint64_t *freq)
+{
+	struct mlx5_priv *priv = dev->data->dev_private;
+
+	*freq = priv->config.hca_attr.device_frequency_khz * 1000;
+
+	return 0;
+}
+
 /**
  * Retrieve the master device for representor in the same switch domain.
  *
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index fa3b02787e..234e95dbb1 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -2342,6 +2342,7 @@ const struct eth_dev_ops mlx5_os_dev_ops = {
 	.fw_version_get = mlx5_fw_version_get,
 	.dev_infos_get = mlx5_dev_infos_get,
 	.read_clock = mlx5_txpp_read_clock,
+	.get_clock_freq = mlx5_get_clock_freq,
 	.dev_supported_ptypes_get = mlx5_dev_supported_ptypes_get,
 	.vlan_filter_set = mlx5_vlan_filter_set,
 	.rx_queue_setup = mlx5_rx_queue_setup,
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index a92194d2dd..0ccfb6b8d9 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -820,6 +820,7 @@ int mlx5_get_mac(struct rte_eth_dev *dev, uint8_t (*mac)[RTE_ETHER_ADDR_LEN]);
 int mlx5_get_mtu(struct rte_eth_dev *dev, uint16_t *mtu);
 int mlx5_set_mtu(struct rte_eth_dev *dev, uint16_t mtu);
 int mlx5_read_clock(struct rte_eth_dev *dev, uint64_t *clock);
+int mlx5_get_clock_freq(struct rte_eth_dev *dev, uint64_t *freq);
 int mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete);
 int mlx5_dev_get_flow_ctrl(struct rte_eth_dev *dev,
 			   struct rte_eth_fc_conf *fc_conf);
-- 
2.17.1


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

* [dpdk-dev] [[PATCH v3 2/4] ethdev: add API to query device frequency
  2020-07-24 20:23 [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Patrick Keroulas
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 1/4] net/mlx5: query device frequency Patrick Keroulas
@ 2020-07-24 20:23 ` Patrick Keroulas
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 3/4] pdump: convert timestamp to nanoseconds on Rx path Patrick Keroulas
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Patrick Keroulas @ 2020-07-24 20:23 UTC (permalink / raw)
  To: dev; +Cc: Patrick Keroulas

Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
---
 lib/librte_ethdev/rte_ethdev.c           | 12 ++++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 17 +++++++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      |  5 +++++
 lib/librte_ethdev/rte_ethdev_version.map |  2 ++
 4 files changed, 36 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 7858ad5f11..8e8812782f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4882,6 +4882,18 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
 	return eth_err(port_id, (*dev->dev_ops->read_clock)(dev, clock));
 }
 
+int
+rte_eth_get_clock_freq(uint16_t port_id, uint64_t *freq)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_clock_freq, -ENOTSUP);
+	return eth_err(port_id, (*dev->dev_ops->get_clock_freq)(dev, freq));
+}
+
 int
 rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 57e4a6ca58..d17838e66f 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4264,6 +4264,23 @@ __rte_experimental
 int
 rte_eth_read_clock(uint16_t port_id, uint64_t *clock);
 
+/**
+ * Get the clock frequency of ethernet device, in Hz
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param[out] freq
+ *   Pointer to the device clock frequency.
+ *
+ * @return
+ *   - 0: Success.
+ *   - -ENODEV: The port ID is invalid.
+ *   - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+__rte_experimental
+int
+rte_eth_get_clock_freq(uint16_t port_id, uint64_t *freq);
+
 /**
  * Config l2 tunnel ether type of an Ethernet device for filtering specific
  * tunnel packets by ether type.
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 32407dd418..ac64961e6f 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -468,6 +468,10 @@ typedef int (*eth_read_clock)(struct rte_eth_dev *dev,
 				      uint64_t *timestamp);
 /**< @internal Function used to get the current value of the device clock. */
 
+typedef int (*eth_get_clock_freq)(struct rte_eth_dev *dev,
+				      uint64_t *freq);
+/**< @internal Function used to frequency from device clock */
+
 typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
 				struct rte_dev_reg_info *info);
 /**< @internal Retrieve registers  */
@@ -731,6 +735,7 @@ struct eth_dev_ops {
 	eth_timesync_write_time    timesync_write_time; /** Set the device clock time. */
 
 	eth_read_clock             read_clock;
+	eth_get_clock_freq         get_clock_freq;
 
 	eth_xstats_get_by_id_t     xstats_get_by_id;
 	/**< Get extended device statistic values by ID. */
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 1212a17d32..ffc00bf94f 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -241,6 +241,8 @@ EXPERIMENTAL {
 	__rte_ethdev_trace_rx_burst;
 	__rte_ethdev_trace_tx_burst;
 	rte_flow_get_aged_flows;
+
+	rte_eth_get_clock_freq;
 };
 
 INTERNAL {
-- 
2.17.1


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

* [dpdk-dev] [[PATCH v3 3/4] pdump: convert timestamp to nanoseconds on Rx path
  2020-07-24 20:23 [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Patrick Keroulas
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 1/4] net/mlx5: query device frequency Patrick Keroulas
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 2/4] ethdev: add API to " Patrick Keroulas
@ 2020-07-24 20:23 ` Patrick Keroulas
  2020-09-02 15:41   ` Pattan, Reshma
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps Patrick Keroulas
  2020-10-06 16:25 ` [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Ferruh Yigit
  4 siblings, 1 reply; 11+ messages in thread
From: Patrick Keroulas @ 2020-07-24 20:23 UTC (permalink / raw)
  To: dev; +Cc: Patrick Keroulas

Use start time and device frequency to translate raw Rx hardware
timestamps into nanoseconds.

Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
---
 lib/librte_pdump/rte_pdump.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index b3c8d5ce43..8fd7e13af5 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -2,6 +2,8 @@
  * Copyright(c) 2016-2018 Intel Corporation
  */
 
+#include <sys/time.h>
+
 #include <rte_memcpy.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
@@ -69,6 +71,21 @@ static struct pdump_rxtx_cbs {
 } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
 tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
 
+static uint64_t hz;
+static uint64_t start_time;
+static uint64_t start_cycles;
+
+static inline void
+pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
+{
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; i++) {
+		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
+			pkts[i]->timestamp = start_time +
+				 (pkts[i]->timestamp - start_cycles) * NS_PER_S / hz;
+	}
+}
 
 static inline void
 pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
@@ -107,6 +124,7 @@ pdump_rx(uint16_t port __rte_unused, uint16_t qidx __rte_unused,
 	uint16_t max_pkts __rte_unused,
 	void *user_params)
 {
+	pdump_ts_to_ns(pkts, nb_pkts);
 	pdump_copy(pkts, nb_pkts, user_params);
 	return nb_pkts;
 }
@@ -131,6 +149,13 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 	for (; qid < end_q; qid++) {
 		cbs = &rx_cbs[port][qid];
 		if (cbs && operation == ENABLE) {
+			struct timeval now;
+
+			rte_eth_read_clock(port, &start_cycles);
+			rte_eth_get_clock_freq(port, &hz);
+			gettimeofday(&now, NULL);
+			start_time = now.tv_sec * NS_PER_S + now.tv_usec * 1000;
+
 			if (cbs->cb) {
 				PDUMP_LOG(ERR,
 					"failed to add rx callback for port=%d "
-- 
2.17.1


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

* [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps
  2020-07-24 20:23 [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Patrick Keroulas
                   ` (2 preceding siblings ...)
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 3/4] pdump: convert timestamp to nanoseconds on Rx path Patrick Keroulas
@ 2020-07-24 20:23 ` Patrick Keroulas
  2020-07-25  8:35   ` Tom Barbette
  2020-07-26 22:17   ` Stephen Hemminger
  2020-10-06 16:25 ` [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Ferruh Yigit
  4 siblings, 2 replies; 11+ messages in thread
From: Patrick Keroulas @ 2020-07-24 20:23 UTC (permalink / raw)
  To: dev; +Cc: Vivien Didelot, Patrick Keroulas

From: Vivien Didelot <vivien.didelot@gmail.com>

When hardware timestamping is enabled on Rx path, system time should
no longer be used to calculate the timestamps when dumping packets.

Instead, use the value stored by the driver in mbuf->timestamp
and assume it is already converted to nanoseconds (otherwise the
application may edit the packet headers itself afterwards).

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
---
 doc/guides/rel_notes/release_20_08.rst |  1 +
 drivers/net/pcap/rte_eth_pcap.c        | 32 +++++++++++++-----------
 lib/librte_pdump/rte_pdump.c           | 34 ++++++++++++++------------
 3 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
index 89822bcb8d..bda2f7567a 100644
--- a/doc/guides/rel_notes/release_20_08.rst
+++ b/doc/guides/rel_notes/release_20_08.rst
@@ -105,6 +105,7 @@ New Features
   Updated PCAP driver with new features and improvements, including:
 
   * Support software Tx nanosecond timestamps precision.
+  * Support hardware Tx timestamps.
 
 * **Updated Broadcom bnxt driver.**
 
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 668cbd1fc7..45775951f7 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
 #define NSEC_PER_SEC	1000000000L
 
 static inline void
-calculate_timestamp(struct timeval *ts) {
-	uint64_t cycles;
-	struct timeval cur_time;
-
-	cycles = rte_get_timer_cycles() - start_cycles;
-	cur_time.tv_sec = cycles / hz;
-	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
-
-	ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
-	ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
-	if (ts->tv_usec >= NSEC_PER_SEC) {
-		ts->tv_usec -= NSEC_PER_SEC;
-		ts->tv_sec += 1;
+calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
+	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
+		ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
+		ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
+	} else {
+		uint64_t cycles = rte_get_timer_cycles() - start_cycles;
+		struct timeval cur_time = {
+			.tv_sec = cycles / hz,
+			.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz,
+		};
+
+		ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
+		ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
+		if (ts->tv_usec >= NSEC_PER_SEC) {
+			ts->tv_usec -= NSEC_PER_SEC;
+			ts->tv_sec += 1;
+		}
 	}
 }
 
@@ -339,7 +343,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			caplen = sizeof(temp_data);
 		}
 
-		calculate_timestamp(&header.ts);
+		calculate_timestamp(mbuf, &header.ts);
 		header.len = len;
 		header.caplen = caplen;
 		/* rte_pktmbuf_read() returns a pointer to the data directly
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index 8fd7e13af5..e8963e9dd8 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -71,21 +71,6 @@ static struct pdump_rxtx_cbs {
 } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
 tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
 
-static uint64_t hz;
-static uint64_t start_time;
-static uint64_t start_cycles;
-
-static inline void
-pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
-{
-	unsigned int i;
-
-	for (i = 0; i < nb_pkts; i++) {
-		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
-			pkts[i]->timestamp = start_time +
-				 (pkts[i]->timestamp - start_cycles) * NS_PER_S / hz;
-	}
-}
 
 static inline void
 pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
@@ -118,6 +103,23 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
 	}
 }
 
+#define NSEC_PER_SEC 1000000000L
+static uint64_t hz;
+static uint64_t start_time;
+static uint64_t start_cycles;
+
+static inline void
+pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
+{
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; i++) {
+		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
+			pkts[i]->timestamp = start_time +
+				 (pkts[i]->timestamp - start_cycles) * NSEC_PER_SEC / hz;
+	}
+}
+
 static uint16_t
 pdump_rx(uint16_t port __rte_unused, uint16_t qidx __rte_unused,
 	struct rte_mbuf **pkts, uint16_t nb_pkts,
@@ -154,7 +156,7 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 			rte_eth_read_clock(port, &start_cycles);
 			rte_eth_get_clock_freq(port, &hz);
 			gettimeofday(&now, NULL);
-			start_time = now.tv_sec * NS_PER_S + now.tv_usec * 1000;
+			start_time = now.tv_sec * NSEC_PER_SEC + now.tv_usec * 1000;
 
 			if (cbs->cb) {
 				PDUMP_LOG(ERR,
-- 
2.17.1


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

* Re: [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps Patrick Keroulas
@ 2020-07-25  8:35   ` Tom Barbette
  2020-07-25 11:51     ` Stephen Hemminger
  2020-07-26 22:17   ` Stephen Hemminger
  1 sibling, 1 reply; 11+ messages in thread
From: Tom Barbette @ 2020-07-25  8:35 UTC (permalink / raw)
  To: Patrick Keroulas, dev; +Cc: Vivien Didelot

Nice patch!

However I would use a multiply and shift approach instead of a division 
in the fast path. When we did that it was really hurting the performance.

Cheers,

Tom

Le 24/07/2020 à 22:23, Patrick Keroulas a écrit :
> From: Vivien Didelot <vivien.didelot@gmail.com>
> 
> When hardware timestamping is enabled on Rx path, system time should
> no longer be used to calculate the timestamps when dumping packets.
> 
> Instead, use the value stored by the driver in mbuf->timestamp
> and assume it is already converted to nanoseconds (otherwise the
> application may edit the packet headers itself afterwards).
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
> ---
>   doc/guides/rel_notes/release_20_08.rst |  1 +
>   drivers/net/pcap/rte_eth_pcap.c        | 32 +++++++++++++-----------
>   lib/librte_pdump/rte_pdump.c           | 34 ++++++++++++++------------
>   3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 89822bcb8d..bda2f7567a 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -105,6 +105,7 @@ New Features
>     Updated PCAP driver with new features and improvements, including:
>   
>     * Support software Tx nanosecond timestamps precision.
> +  * Support hardware Tx timestamps.
>   
>   * **Updated Broadcom bnxt driver.**
>   
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 668cbd1fc7..45775951f7 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
>   #define NSEC_PER_SEC	1000000000L
>   
>   static inline void
> -calculate_timestamp(struct timeval *ts) {
> -	uint64_t cycles;
> -	struct timeval cur_time;
> -
> -	cycles = rte_get_timer_cycles() - start_cycles;
> -	cur_time.tv_sec = cycles / hz;
> -	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
> -
> -	ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
> -	ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
> -	if (ts->tv_usec >= NSEC_PER_SEC) {
> -		ts->tv_usec -= NSEC_PER_SEC;
> -		ts->tv_sec += 1;
> +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
> +	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> +		ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
> +		ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
> +	} else {
> +		uint64_t cycles = rte_get_timer_cycles() - start_cycles;
> +		struct timeval cur_time = {
> +			.tv_sec = cycles / hz,
> +			.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz,
> +		};
> +
> +		ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
> +		ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
> +		if (ts->tv_usec >= NSEC_PER_SEC) {
> +			ts->tv_usec -= NSEC_PER_SEC;
> +			ts->tv_sec += 1;
> +		}
>   	}
>   }
>   
> @@ -339,7 +343,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   			caplen = sizeof(temp_data);
>   		}
>   
> -		calculate_timestamp(&header.ts);
> +		calculate_timestamp(mbuf, &header.ts);
>   		header.len = len;
>   		header.caplen = caplen;
>   		/* rte_pktmbuf_read() returns a pointer to the data directly
> diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
> index 8fd7e13af5..e8963e9dd8 100644
> --- a/lib/librte_pdump/rte_pdump.c
> +++ b/lib/librte_pdump/rte_pdump.c
> @@ -71,21 +71,6 @@ static struct pdump_rxtx_cbs {
>   } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
>   tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
>   
> -static uint64_t hz;
> -static uint64_t start_time;
> -static uint64_t start_cycles;
> -
> -static inline void
> -pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < nb_pkts; i++) {
> -		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
> -			pkts[i]->timestamp = start_time +
> -				 (pkts[i]->timestamp - start_cycles) * NS_PER_S / hz;
> -	}
> -}
>   
>   static inline void
>   pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
> @@ -118,6 +103,23 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
>   	}
>   }
>   
> +#define NSEC_PER_SEC 1000000000L
> +static uint64_t hz;
> +static uint64_t start_time;
> +static uint64_t start_cycles;
> +
> +static inline void
> +pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
> +			pkts[i]->timestamp = start_time +
> +				 (pkts[i]->timestamp - start_cycles) * NSEC_PER_SEC / hz;
> +	}
> +}
> +
>   static uint16_t
>   pdump_rx(uint16_t port __rte_unused, uint16_t qidx __rte_unused,
>   	struct rte_mbuf **pkts, uint16_t nb_pkts,
> @@ -154,7 +156,7 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
>   			rte_eth_read_clock(port, &start_cycles);
>   			rte_eth_get_clock_freq(port, &hz);
>   			gettimeofday(&now, NULL);
> -			start_time = now.tv_sec * NS_PER_S + now.tv_usec * 1000;
> +			start_time = now.tv_sec * NSEC_PER_SEC + now.tv_usec * 1000;
>   
>   			if (cbs->cb) {
>   				PDUMP_LOG(ERR,
> 


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

* Re: [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps
  2020-07-25  8:35   ` Tom Barbette
@ 2020-07-25 11:51     ` Stephen Hemminger
  0 siblings, 0 replies; 11+ messages in thread
From: Stephen Hemminger @ 2020-07-25 11:51 UTC (permalink / raw)
  To: Tom Barbette; +Cc: Patrick Keroulas, dev, Vivien Didelot

On Sat, 25 Jul 2020 10:35:48 +0200
Tom Barbette <barbette@kth.se> wrote:

> Nice patch!
> 
> However I would use a multiply and shift approach instead of a division 
> in the fast path. When we did that it was really hurting the performance.


See rte_reciprocal

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

* Re: [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps Patrick Keroulas
  2020-07-25  8:35   ` Tom Barbette
@ 2020-07-26 22:17   ` Stephen Hemminger
  1 sibling, 0 replies; 11+ messages in thread
From: Stephen Hemminger @ 2020-07-26 22:17 UTC (permalink / raw)
  To: Patrick Keroulas; +Cc: dev, Vivien Didelot

On Fri, 24 Jul 2020 16:23:15 -0400
Patrick Keroulas <patrick.keroulas@radio-canada.ca> wrote:

> From: Vivien Didelot <vivien.didelot@gmail.com>
> 
> When hardware timestamping is enabled on Rx path, system time should
> no longer be used to calculate the timestamps when dumping packets.
> 
> Instead, use the value stored by the driver in mbuf->timestamp
> and assume it is already converted to nanoseconds (otherwise the
> application may edit the packet headers itself afterwards).
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> Signed-off-by: Patrick Keroulas <patrick.keroulas@radio-canada.ca>
> ---
>  doc/guides/rel_notes/release_20_08.rst |  1 +
>  drivers/net/pcap/rte_eth_pcap.c        | 32 +++++++++++++-----------
>  lib/librte_pdump/rte_pdump.c           | 34 ++++++++++++++------------
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 89822bcb8d..bda2f7567a 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -105,6 +105,7 @@ New Features
>    Updated PCAP driver with new features and improvements, including:
>  
>    * Support software Tx nanosecond timestamps precision.
> +  * Support hardware Tx timestamps.
>  
>  * **Updated Broadcom bnxt driver.**
>  
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 668cbd1fc7..45775951f7 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
>  #define NSEC_PER_SEC	1000000000L
>  
>  static inline void
> -calculate_timestamp(struct timeval *ts) {
> -	uint64_t cycles;
> -	struct timeval cur_time;
> -
> -	cycles = rte_get_timer_cycles() - start_cycles;
> -	cur_time.tv_sec = cycles / hz;
> -	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
> -
> -	ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
> -	ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
> -	if (ts->tv_usec >= NSEC_PER_SEC) {
> -		ts->tv_usec -= NSEC_PER_SEC;
> -		ts->tv_sec += 1;
> +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {

DPDK style is to have { on seperate line

> +	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> +		ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
> +		ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
> +	} else {
> +		uint64_t cycles = rte_get_timer_cycles() - start_cycles;
> +		struct timeval cur_time = {
> +			.tv_sec = cycles / hz,
> +			.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz,
> +		};
> +
> +		ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
> +		ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
> +		if (ts->tv_usec >= NSEC_PER_SEC) {
> +			ts->tv_usec -= NSEC_PER_SEC;
> +			ts->tv_sec += 1;
> +		}
>  	}
>  }
>  
> @@ -339,7 +343,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			caplen = sizeof(temp_data);
>  		}
>  
> -		calculate_timestamp(&header.ts);
> +		calculate_timestamp(mbuf, &header.ts);

On Tx pcap you don't want the hardware RX timestamp.
You should update mbuf with software timestamp.

>  		header.len = len;
>  		header.caplen = caplen;
>  		/* rte_pktmbuf_read() returns a pointer to the data directly
> diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
> index 8fd7e13af5..e8963e9dd8 100644
> --- a/lib/librte_pdump/rte_pdump.c
> +++ b/lib/librte_pdump/rte_pdump.c
> @@ -71,21 +71,6 @@ static struct pdump_rxtx_cbs {
>  } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
>  tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
>  
> -static uint64_t hz;
> -static uint64_t start_time;
> -static uint64_t start_cycles;
> -
> -static inline void
> -pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < nb_pkts; i++) {
> -		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
> -			pkts[i]->timestamp = start_time +
> -				 (pkts[i]->timestamp - start_cycles) * NS_PER_S / hz;
> -	}
> -}
>  
>  static inline void
>  pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
> @@ -118,6 +103,23 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
>  	}
>  }
>  
> +#define NSEC_PER_SEC 1000000000L
> +static uint64_t hz;
> +static uint64_t start_time;
> +static uint64_t start_cycles;
> +
> +static inline void
> +pdump_ts_to_ns(struct rte_mbuf **pkts, uint16_t nb_pkts)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) && hz)
> +			pkts[i]->timestamp = start_time +
> +				 (pkts[i]->timestamp - start_cycles) * NSEC_PER_SEC / hz;
> +	}
> +}
> +
>  static uint16_t
>  pdump_rx(uint16_t port __rte_unused, uint16_t qidx __rte_unused,
>  	struct rte_mbuf **pkts, uint16_t nb_pkts,
> @@ -154,7 +156,7 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
>  			rte_eth_read_clock(port, &start_cycles);
>  			rte_eth_get_clock_freq(port, &hz);
>  			gettimeofday(&now, NULL);
> -			start_time = now.tv_sec * NS_PER_S + now.tv_usec * 1000;
> +			start_time = now.tv_sec * NSEC_PER_SEC + now.tv_usec * 1000;
>  
>  			if (cbs->cb) {
>  				PDUMP_LOG(ERR,


Actually using the current way through rte_eth_pcap adds a lot
of overhead. I dropped it when doing pcapng.

Perhaps we should work together on the pcapng version.


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

* Re: [dpdk-dev] [[PATCH v3 3/4] pdump: convert timestamp to nanoseconds on Rx path
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 3/4] pdump: convert timestamp to nanoseconds on Rx path Patrick Keroulas
@ 2020-09-02 15:41   ` Pattan, Reshma
  0 siblings, 0 replies; 11+ messages in thread
From: Pattan, Reshma @ 2020-09-02 15:41 UTC (permalink / raw)
  To: Patrick Keroulas, dev



> -----Original Message-----
> +			rte_eth_read_clock(port, &start_cycles);
> +			rte_eth_get_clock_freq(port, &hz);

Do you need to check return value of these calls to  handle -ENOTSUP? And avoid timestamping marking.
If -ENOTSUP, then hz value is 0 and using hz in function pdump_ts_to_ns() might be illegal.


> +			gettimeofday(&now, NULL);

Same here, need to handle return value carefully?

Thanks,
Reshma

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

* Re: [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5
  2020-07-24 20:23 [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Patrick Keroulas
                   ` (3 preceding siblings ...)
  2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps Patrick Keroulas
@ 2020-10-06 16:25 ` Ferruh Yigit
  2020-10-07  6:59   ` Morten Brørup
  4 siblings, 1 reply; 11+ messages in thread
From: Ferruh Yigit @ 2020-10-06 16:25 UTC (permalink / raw)
  To: Patrick Keroulas, dev, Vivien Didelot
  Cc: Stephen Hemminger, Morten Brørup, Viacheslav Ovsiienko,
	Tom Barbette, Reshma Pattan, Olivier Matz, Thomas Monjalon,
	Andrew Rybchenko

On 7/24/2020 9:23 PM, Patrick Keroulas wrote:
> The intention is to produce a pcap with nanosecond precision when
> Rx timestamp offloading is activated on mlx5 NIC.
> 
> The packets forwarded by testpmd hold the raw counter but a pcap
> requires a time unit. Assuming that the NIC clock is already synced
> with external master clock, this patchset simply integrates the
> nanosecond converter that derives from device frequency and start time.
> 
> v2 -> v3:
>      - replace ib_verbs nanosecond converter with more generic method
>        based on device frequency and start time.
> 
> Patrick Keroulas (3):
>    net/mlx5: query device frequency
>    ethdev: add API to query device frequency
>    pdump: convert timestamp to nanoseconds on Rx path
> 
> Vivien Didelot (1):
>    net/pcap: support hardware Tx timestamps
> 

We have three patch/patchset for same issue,

1) Current one, https://patches.dpdk.org/user/todo/dpdk/?series=11294
2) Vivien's series: https://patches.dpdk.org/user/todo/dpdk/?series=10678
3) Vivien's pcap patch: https://patches.dpdk.org/user/todo/dpdk/?series=10403

And one related one from Slava,
4) https://patchwork.dpdk.org/project/dpdk/list/?series=10948&state=%2A&archive=both

I am replying to this one since this is the latest, but first can we clarify if 
all are still valid and can we combine the effort?


Second, the problems to solve,
1) Device provided timestamp value has no unit, it needs to be converted to 
nanosecond.
There are different approaches in above patches,
- One adds '.convert_ts_to_ns' dev_ops to make PMD convert the timestamp
- Other adds '.eth_get_clock_freq' dev_ops, to get frequency from device clock
   so that conversion can be done within the app.
- I wonder why existing 'rte_eth_read_clock()' can't be enough for conversion,
   as described in its documentation:
   https://doc.dpdk.org/api/rte__ethdev_8h.html#a4346bf07a0d302c9ba4fe06baffd3196
     rte_eth_read_clock(port, start);
     rte_delay_ms(100);
     rte_eth_read_clock(port, end);
     double freq = (end - start) * 10;

2) Where to put the timestamps data, is it to the 'mbuf->timestamp' or dynamic
    filed 'RTE_MBUF_DYNFIELD_TIMESTAMP_NAME'? Using dynamic field of requires
    more work on registering and looking up the fields.

3) Calculation in the datapath should be done in a performance optimized way, 
instead of using division.

4) Should the timestamp value provided by the Rx device used, or should the time 
when the packet transmitted used. I can see current use case seems first one, 
but can there be cases we would like to record when packet exactly sent.

5) Should we create a 'PKT_TX_TIMESTAMP' flag, instead of re-using the Rx one, 
to let the application explicitly define which packets to record timestamp.

Please add if I missing more.

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

* Re: [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5
  2020-10-06 16:25 ` [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Ferruh Yigit
@ 2020-10-07  6:59   ` Morten Brørup
  0 siblings, 0 replies; 11+ messages in thread
From: Morten Brørup @ 2020-10-07  6:59 UTC (permalink / raw)
  To: Ferruh Yigit, Patrick Keroulas, dev, Vivien Didelot
  Cc: Stephen Hemminger, Viacheslav Ovsiienko, Tom Barbette,
	Reshma Pattan, Olivier Matz, Thomas Monjalon, Andrew Rybchenko

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, October 6, 2020 6:26 PM
> 
> On 7/24/2020 9:23 PM, Patrick Keroulas wrote:
> > The intention is to produce a pcap with nanosecond precision when
> > Rx timestamp offloading is activated on mlx5 NIC.
> >
> > The packets forwarded by testpmd hold the raw counter but a pcap
> > requires a time unit. Assuming that the NIC clock is already synced
> > with external master clock, this patchset simply integrates the
> > nanosecond converter that derives from device frequency and start
> time.
> >
> > v2 -> v3:
> >      - replace ib_verbs nanosecond converter with more generic method
> >        based on device frequency and start time.
> >
> > Patrick Keroulas (3):
> >    net/mlx5: query device frequency
> >    ethdev: add API to query device frequency
> >    pdump: convert timestamp to nanoseconds on Rx path
> >
> > Vivien Didelot (1):
> >    net/pcap: support hardware Tx timestamps
> >
> 
> We have three patch/patchset for same issue,
> 
> 1) Current one, https://patches.dpdk.org/user/todo/dpdk/?series=11294
> 2) Vivien's series:
> https://patches.dpdk.org/user/todo/dpdk/?series=10678
> 3) Vivien's pcap patch:
> https://patches.dpdk.org/user/todo/dpdk/?series=10403
> 
> And one related one from Slava,
> 4)
> https://patchwork.dpdk.org/project/dpdk/list/?series=10948&state=%2A&ar
> chive=both
> 
> I am replying to this one since this is the latest, but first can we
> clarify if
> all are still valid and can we combine the effort?
> 
> 
> Second, the problems to solve,
> 1) Device provided timestamp value has no unit, it needs to be
> converted to
> nanosecond.
> There are different approaches in above patches,
> - One adds '.convert_ts_to_ns' dev_ops to make PMD convert the
> timestamp
> - Other adds '.eth_get_clock_freq' dev_ops, to get frequency from
> device clock
>    so that conversion can be done within the app.
> - I wonder why existing 'rte_eth_read_clock()' can't be enough for
> conversion,
>    as described in its documentation:
> 
> https://doc.dpdk.org/api/rte__ethdev_8h.html#a4346bf07a0d302c9ba4fe06ba
> ffd3196
>      rte_eth_read_clock(port, start);
>      rte_delay_ms(100);
>      rte_eth_read_clock(port, end);
>      double freq = (end - start) * 10;
> 
> 2) Where to put the timestamps data, is it to the 'mbuf->timestamp' or
> dynamic
>     filed 'RTE_MBUF_DYNFIELD_TIMESTAMP_NAME'? Using dynamic field of
> requires
>     more work on registering and looking up the fields.
> 
> 3) Calculation in the datapath should be done in a performance
> optimized way,
> instead of using division.
> 
> 4) Should the timestamp value provided by the Rx device used, or should
> the time
> when the packet transmitted used. I can see current use case seems
> first one,
> but can there be cases we would like to record when packet exactly
> sent.
> 
> 5) Should we create a 'PKT_TX_TIMESTAMP' flag, instead of re-using the
> Rx one,
> to let the application explicitly define which packets to record
> timestamp.
> 
> Please add if I missing more.

Regarding RX timestamps:

I believe that it is on the roadmap to remove the timestamp field from the mbuf and make it a dynamic field instead.

Furthermore, I understand that some Mellanox NICs have accurate "wall clock" timestamping capability, possibly even PTP synchronized. So I propose to make two variants of the dynamic timestamp field, one with an accurate "wall clock" timestamp for NICs with that capability, and one with an NIC-specific unitless timestamp (like the current timestamp).

Regarding TX timestamps:

Do any NICs currently have a TX pipeline that puts a TX timestamp in the descriptor on transmission to the wire instead of just marking the descriptor as free? If not, then there is probably no need for TX timestamps at that level of accuracy for captured packets. The application can set the timestamp in the captured/mirrored packets while cloning the originals and passing them on to the NIC driver.

And if the dynamic mbuf field that instructs the NIC to transmit the packet at a specific time is present, the application might even use that timestamp, knowing that the NIC will transmit the packet at that time.


Med venlig hilsen / kind regards
- Morten Brørup


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

end of thread, other threads:[~2020-10-07  6:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 20:23 [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Patrick Keroulas
2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 1/4] net/mlx5: query device frequency Patrick Keroulas
2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 2/4] ethdev: add API to " Patrick Keroulas
2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 3/4] pdump: convert timestamp to nanoseconds on Rx path Patrick Keroulas
2020-09-02 15:41   ` Pattan, Reshma
2020-07-24 20:23 ` [dpdk-dev] [[PATCH v3 4/4] net/pcap: support hardware Tx timestamps Patrick Keroulas
2020-07-25  8:35   ` Tom Barbette
2020-07-25 11:51     ` Stephen Hemminger
2020-07-26 22:17   ` Stephen Hemminger
2020-10-06 16:25 ` [dpdk-dev] [[PATCH v3 0/4] pdump HW Rx timestamps for mlx5 Ferruh Yigit
2020-10-07  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).