[03/15] ethdev: register mbuf field and flags for timestamp
diff mbox series

Message ID 20201029092751.3837177-4-thomas@monjalon.net
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • remove mbuf timestamp
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Oct. 29, 2020, 9:27 a.m. UTC
During port configure or queue setup, the offload flags
DEV_RX_OFFLOAD_TIMESTAMP and DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP
trigger the registration of the related mbuf field and flags.

Previously, the Tx timestamp field and flag were registered in testpmd,
as described in mlx5 guide.
For the general usage of Rx and Tx timestamps,
managing registrations inside ethdev is simpler and properly documented.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/config.c          | 38 ------------------
 doc/guides/nics/mlx5.rst       |  5 +--
 lib/librte_ethdev/rte_ethdev.c | 70 ++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h |  9 ++++-
 lib/librte_mbuf/rte_mbuf_dyn.h |  1 +
 5 files changed, 81 insertions(+), 42 deletions(-)

Comments

Andrew Rybchenko Oct. 29, 2020, 10:08 a.m. UTC | #1
On 10/29/20 12:27 PM, Thomas Monjalon wrote:
> During port configure or queue setup, the offload flags
> DEV_RX_OFFLOAD_TIMESTAMP and DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP
> trigger the registration of the related mbuf field and flags.
> 
> Previously, the Tx timestamp field and flag were registered in testpmd,
> as described in mlx5 guide.
> For the general usage of Rx and Tx timestamps,
> managing registrations inside ethdev is simpler and properly documented.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

A small note below, other than that

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index b12bb3854d..7c9aadb461 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c

[snip]

> @@ -1232,6 +1233,59 @@ eth_dev_check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
>  	return ret;
>  }
>  
> +static inline int
> +eth_dev_timestamp_mbuf_register(uint64_t rx_offloads, uint64_t tx_offloads)
> +{
> +	static const struct rte_mbuf_dynfield field_desc = {
> +		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
> +		.size = sizeof(rte_mbuf_timestamp_t),
> +		.align = __alignof__(rte_mbuf_timestamp_t),
> +	};
> +	static const struct rte_mbuf_dynflag rx_flag_desc = {
> +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
> +	};
> +	static const struct rte_mbuf_dynflag tx_flag_desc = {
> +		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
> +	};
> +	static bool done_rx, done_tx;

I think we don't need these static flags. We can just repeat
registeration request and it will simply lookup and return
the same offset/flagbit as before.

[snip]
Thomas Monjalon Oct. 29, 2020, 10:12 a.m. UTC | #2
29/10/2020 11:08, Andrew Rybchenko:
> On 10/29/20 12:27 PM, Thomas Monjalon wrote:
> > During port configure or queue setup, the offload flags
> > DEV_RX_OFFLOAD_TIMESTAMP and DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP
> > trigger the registration of the related mbuf field and flags.
> > 
> > Previously, the Tx timestamp field and flag were registered in testpmd,
> > as described in mlx5 guide.
> > For the general usage of Rx and Tx timestamps,
> > managing registrations inside ethdev is simpler and properly documented.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> A small note below, other than that
> 
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
> > +static inline int
> > +eth_dev_timestamp_mbuf_register(uint64_t rx_offloads, uint64_t tx_offloads)
> > +{
> > +	static const struct rte_mbuf_dynfield field_desc = {
> > +		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
> > +		.size = sizeof(rte_mbuf_timestamp_t),
> > +		.align = __alignof__(rte_mbuf_timestamp_t),
> > +	};
> > +	static const struct rte_mbuf_dynflag rx_flag_desc = {
> > +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
> > +	};
> > +	static const struct rte_mbuf_dynflag tx_flag_desc = {
> > +		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
> > +	};
> > +	static bool done_rx, done_tx;
> 
> I think we don't need these static flags. We can just repeat
> registeration request and it will simply lookup and return
> the same offset/flagbit as before.

Absolutely.
I did it as a small optimization in control path.

I hesitated. Given it is only 2 booleans,
do you prefer with or without or no opinion?
Andrew Rybchenko Oct. 29, 2020, 10:33 a.m. UTC | #3
On 10/29/20 1:12 PM, Thomas Monjalon wrote:
> 29/10/2020 11:08, Andrew Rybchenko:
>> On 10/29/20 12:27 PM, Thomas Monjalon wrote:
>>> During port configure or queue setup, the offload flags
>>> DEV_RX_OFFLOAD_TIMESTAMP and DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP
>>> trigger the registration of the related mbuf field and flags.
>>>
>>> Previously, the Tx timestamp field and flag were registered in testpmd,
>>> as described in mlx5 guide.
>>> For the general usage of Rx and Tx timestamps,
>>> managing registrations inside ethdev is simpler and properly documented.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>
>> A small note below, other than that
>>
>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>
>>> +static inline int
>>> +eth_dev_timestamp_mbuf_register(uint64_t rx_offloads, uint64_t tx_offloads)
>>> +{
>>> +	static const struct rte_mbuf_dynfield field_desc = {
>>> +		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
>>> +		.size = sizeof(rte_mbuf_timestamp_t),
>>> +		.align = __alignof__(rte_mbuf_timestamp_t),
>>> +	};
>>> +	static const struct rte_mbuf_dynflag rx_flag_desc = {
>>> +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
>>> +	};
>>> +	static const struct rte_mbuf_dynflag tx_flag_desc = {
>>> +		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
>>> +	};
>>> +	static bool done_rx, done_tx;
>>
>> I think we don't need these static flags. We can just repeat
>> registeration request and it will simply lookup and return
>> the same offset/flagbit as before.
> 
> Absolutely.
> I did it as a small optimization in control path.
> 
> I hesitated. Given it is only 2 booleans,
> do you prefer with or without or no opinion?
> 
I'd prefer without it. It is always better without
static variables if possible.
Thomas Monjalon Oct. 29, 2020, 10:46 a.m. UTC | #4
29/10/2020 11:33, Andrew Rybchenko:
> On 10/29/20 1:12 PM, Thomas Monjalon wrote:
> > 29/10/2020 11:08, Andrew Rybchenko:
> >> On 10/29/20 12:27 PM, Thomas Monjalon wrote:
> >>> During port configure or queue setup, the offload flags
> >>> DEV_RX_OFFLOAD_TIMESTAMP and DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP
> >>> trigger the registration of the related mbuf field and flags.
> >>>
> >>> Previously, the Tx timestamp field and flag were registered in testpmd,
> >>> as described in mlx5 guide.
> >>> For the general usage of Rx and Tx timestamps,
> >>> managing registrations inside ethdev is simpler and properly documented.
> >>>
> >>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>
> >> A small note below, other than that
> >>
> >> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>
> >>> +static inline int
> >>> +eth_dev_timestamp_mbuf_register(uint64_t rx_offloads, uint64_t tx_offloads)
> >>> +{
> >>> +	static const struct rte_mbuf_dynfield field_desc = {
> >>> +		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
> >>> +		.size = sizeof(rte_mbuf_timestamp_t),
> >>> +		.align = __alignof__(rte_mbuf_timestamp_t),
> >>> +	};
> >>> +	static const struct rte_mbuf_dynflag rx_flag_desc = {
> >>> +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
> >>> +	};
> >>> +	static const struct rte_mbuf_dynflag tx_flag_desc = {
> >>> +		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
> >>> +	};
> >>> +	static bool done_rx, done_tx;
> >>
> >> I think we don't need these static flags. We can just repeat
> >> registeration request and it will simply lookup and return
> >> the same offset/flagbit as before.
> > 
> > Absolutely.
> > I did it as a small optimization in control path.
> > 
> > I hesitated. Given it is only 2 booleans,
> > do you prefer with or without or no opinion?
> > 
> I'd prefer without it. It is always better without
> static variables if possible.

I liked the naming of variables "todo" and "done"
but I will do what is preferred.
If nobody objects, I will remove this small (useless) optimization.

Patch
diff mbox series

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1668ae3238..9a2baf16fe 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3955,44 +3955,6 @@  show_tx_pkt_times(void)
 void
 set_tx_pkt_times(unsigned int *tx_times)
 {
-	uint16_t port_id;
-	int offload_found = 0;
-	int offset;
-	int flag;
-
-	static const struct rte_mbuf_dynfield desc_offs = {
-		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
-		.size = sizeof(uint64_t),
-		.align = __alignof__(uint64_t),
-	};
-	static const struct rte_mbuf_dynflag desc_flag = {
-		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
-	};
-
-	RTE_ETH_FOREACH_DEV(port_id) {
-		struct rte_eth_dev_info dev_info = { 0 };
-		int ret;
-
-		ret = rte_eth_dev_info_get(port_id, &dev_info);
-		if (ret == 0 && dev_info.tx_offload_capa &
-				DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP) {
-			offload_found = 1;
-			break;
-		}
-	}
-	if (!offload_found) {
-		printf("No device supporting Tx timestamp scheduling found, "
-		       "dynamic flag and field not registered\n");
-		return;
-	}
-	offset = rte_mbuf_dynfield_register(&desc_offs);
-	if (offset < 0 && rte_errno != EEXIST)
-		printf("Dynamic timestamp field registration error: %d",
-		       rte_errno);
-	flag = rte_mbuf_dynflag_register(&desc_flag);
-	if (flag < 0 && rte_errno != EEXIST)
-		printf("Dynamic timestamp flag registration error: %d",
-		       rte_errno);
 	tx_pkt_times_inter = tx_times[0];
 	tx_pkt_times_intra = tx_times[1];
 }
diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index afa65a1379..fa8b13dd1b 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -237,9 +237,8 @@  Limitations
   ``txq_inline_max`` and ``txq_inline_mpw`` devargs keys.
 
 - To provide the packet send scheduling on mbuf timestamps the ``tx_pp``
-  parameter should be specified, RTE_MBUF_DYNFIELD_TIMESTAMP_NAME and
-  RTE_MBUF_DYNFLAG_TIMESTAMP_NAME should be registered by application.
-  When PMD sees the RTE_MBUF_DYNFLAG_TIMESTAMP_NAME set on the packet
+  parameter should be specified.
+  When PMD sees the RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME set on the packet
   being sent it tries to synchronize the time of packet appearing on
   the wire with the specified packet timestamp. It the specified one
   is in the past it should be ignored, if one is in the distant future
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index b12bb3854d..7c9aadb461 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -31,6 +31,7 @@ 
 #include <rte_mempool.h>
 #include <rte_malloc.h>
 #include <rte_mbuf.h>
+#include <rte_mbuf_dyn.h>
 #include <rte_errno.h>
 #include <rte_spinlock.h>
 #include <rte_string_fns.h>
@@ -1232,6 +1233,59 @@  eth_dev_check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
 	return ret;
 }
 
+static inline int
+eth_dev_timestamp_mbuf_register(uint64_t rx_offloads, uint64_t tx_offloads)
+{
+	static const struct rte_mbuf_dynfield field_desc = {
+		.name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME,
+		.size = sizeof(rte_mbuf_timestamp_t),
+		.align = __alignof__(rte_mbuf_timestamp_t),
+	};
+	static const struct rte_mbuf_dynflag rx_flag_desc = {
+		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
+	};
+	static const struct rte_mbuf_dynflag tx_flag_desc = {
+		.name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME,
+	};
+	static bool done_rx, done_tx;
+	bool todo_rx, todo_tx;
+	int offset;
+
+	todo_rx = (rx_offloads & DEV_RX_OFFLOAD_TIMESTAMP) != 0
+		&& !done_rx;
+	todo_tx = (tx_offloads & DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP) != 0
+		&& !done_tx;
+
+	if (todo_rx || todo_tx) {
+		offset = rte_mbuf_dynfield_register(&field_desc);
+		if (offset < 0) {
+			RTE_ETHDEV_LOG(ERR,
+					"Failed to register mbuf field for timestamp\n");
+			return -rte_errno;
+		}
+	}
+	if (todo_rx) {
+		offset = rte_mbuf_dynflag_register(&rx_flag_desc);
+		if (offset < 0) {
+			RTE_ETHDEV_LOG(ERR,
+					"Failed to register mbuf flag for Rx timestamp\n");
+			return -rte_errno;
+		}
+		done_rx = true;
+	}
+	if (todo_tx) {
+		offset = rte_mbuf_dynflag_register(&tx_flag_desc);
+		if (offset < 0) {
+			RTE_ETHDEV_LOG(ERR,
+					"Failed to register mbuf flag for Tx timestamp\n");
+			return -rte_errno;
+		}
+		done_tx = true;
+	}
+
+	return 0;
+}
+
 /*
  * Validate offloads that are requested through rte_eth_dev_configure against
  * the offloads successfully set by the ethernet device.
@@ -1481,6 +1535,12 @@  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		goto rollback;
 	}
 
+	/* Register mbuf field and flags for timestamp offloads if enabled. */
+	ret = eth_dev_timestamp_mbuf_register(dev_conf->rxmode.offloads,
+			dev_conf->txmode.offloads);
+	if (ret != 0)
+		goto rollback;
+
 	/*
 	 * Setup new number of RX/TX queues and reconfigure device.
 	 */
@@ -2088,6 +2148,11 @@  rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 			return ret;
 	}
 
+	/* Register mbuf field and flag for Rx timestamp offload if enabled. */
+	ret = eth_dev_timestamp_mbuf_register(local_conf.offloads, 0);
+	if (ret != 0)
+		return ret;
+
 	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
 					      socket_id, &local_conf, mp);
 	if (!ret) {
@@ -2268,6 +2333,11 @@  rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Register mbuf field and flag for Tx timestamp offload if enabled. */
+	ret = eth_dev_timestamp_mbuf_register(0, local_conf.offloads);
+	if (ret != 0)
+		return ret;
+
 	rte_ethdev_trace_txq_setup(port_id, tx_queue_id, nb_tx_desc, tx_conf);
 	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
 		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index ba997f16ce..3be0050592 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1344,6 +1344,9 @@  struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_VLAN_EXTEND	0x00000400
 #define DEV_RX_OFFLOAD_JUMBO_FRAME	0x00000800
 #define DEV_RX_OFFLOAD_SCATTER		0x00002000
+/**
+ * The mbuf field and flag are registered when the offload is configured.
+ */
 #define DEV_RX_OFFLOAD_TIMESTAMP	0x00004000
 #define DEV_RX_OFFLOAD_SECURITY         0x00008000
 #define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000
@@ -1408,7 +1411,11 @@  struct rte_eth_conf {
 #define DEV_TX_OFFLOAD_IP_TNL_TSO       0x00080000
 /** Device supports outer UDP checksum */
 #define DEV_TX_OFFLOAD_OUTER_UDP_CKSUM  0x00100000
-/** Device supports send on timestamp */
+/**
+ * Device sends on time read from RTE_MBUF_DYNFIELD_TIMESTAMP_NAME
+ * if RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME is set in ol_flags.
+ * The mbuf field and flag are registered when the offload is configured.
+ */
 #define DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP 0x00200000
 /*
  * If new Tx offload capabilities are defined, they also must be
diff --git a/lib/librte_mbuf/rte_mbuf_dyn.h b/lib/librte_mbuf/rte_mbuf_dyn.h
index 5fb85c0610..d4d8f66f77 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.h
+++ b/lib/librte_mbuf/rte_mbuf_dyn.h
@@ -260,6 +260,7 @@  void rte_mbuf_dyn_dump(FILE *out);
  * used by PMD to schedule packet sending.
  */
 #define RTE_MBUF_DYNFIELD_TIMESTAMP_NAME "rte_dynfield_timestamp"
+typedef uint64_t rte_mbuf_timestamp_t;
 
 /**
  * Indicate that the timestamp field in the mbuf was filled by the driver.