diff mbox series

[04/15] latency: switch timestamp to dynamic mbuf field

Message ID 20201029092751.3837177-5-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series remove mbuf timestamp |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Oct. 29, 2020, 9:27 a.m. UTC 
  The mbuf timestamp is moved to a dynamic field
in order to allow removal of the deprecated static field.
The related mbuf flag is also replaced with the dynamic one.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_latencystats/rte_latencystats.c | 48 +++++++++++++++++++---
 1 file changed, 43 insertions(+), 5 deletions(-)

Comments

Andrew Rybchenko Oct. 29, 2020, 10:13 a.m. UTC | #1 
On 10/29/20 12:27 PM, Thomas Monjalon wrote:
> The mbuf timestamp is moved to a dynamic field
> in order to allow removal of the deprecated static field.
> The related mbuf flag is also replaced with the dynamic one.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

[snip]

> diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
> index ba2fff3bcb..a21f6239d9 100644
> --- a/lib/librte_latencystats/rte_latencystats.c
> +++ b/lib/librte_latencystats/rte_latencystats.c

[snip]

> @@ -204,6 +216,14 @@ int
>  rte_latencystats_init(uint64_t app_samp_intvl,
>  		rte_latency_stats_flow_type_fn user_cb)
>  {
> +	static const struct rte_mbuf_dynfield timestamp_dynfield_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 timestamp_dynflag_desc = {
> +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
> +	};

I dislike the duplication. If we can't just lookup by name
which is done after ethdev configure (I guess so), may be
ethdev should provide an API to register?
Thomas Monjalon Oct. 29, 2020, 10:40 a.m. UTC | #2 
29/10/2020 11:13, Andrew Rybchenko:
> On 10/29/20 12:27 PM, Thomas Monjalon wrote:
> > The mbuf timestamp is moved to a dynamic field
> > in order to allow removal of the deprecated static field.
> > The related mbuf flag is also replaced with the dynamic one.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> [snip]
> 
> > diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
> > index ba2fff3bcb..a21f6239d9 100644
> > --- a/lib/librte_latencystats/rte_latencystats.c
> > +++ b/lib/librte_latencystats/rte_latencystats.c
> 
> [snip]
> 
> > @@ -204,6 +216,14 @@ int
> >  rte_latencystats_init(uint64_t app_samp_intvl,
> >  		rte_latency_stats_flow_type_fn user_cb)
> >  {
> > +	static const struct rte_mbuf_dynfield timestamp_dynfield_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 timestamp_dynflag_desc = {
> > +		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
> > +	};
> 
> I dislike the duplication. If we can't just lookup by name
> which is done after ethdev configure (I guess so), may be
> ethdev should provide an API to register?

That's because it is a separate library.
We don't know whether the feature timestamp is already enabled.
We have the port_id, so we could do something.
But the current behaviour is to use timestamp even if it is disabled
at ethdev level. And I don't want to change the behaviour.

Maybe the right solution is to register a separate field for this lib.
Anyway the time base is not the same.
Pattan, Reshma Oct. 29, 2020, 2:20 p.m. UTC | #3 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>


<snip>

> 	rte_mbuf_dynflag_register(&timestamp_dynflag_desc);
> +	if (timestamp_dynflag_offset < 0) {
> +		RTE_LOG(ERR, LATENCY_STATS,
> +				"Cannot register mbuf field for timestamp\n");

Field->flag, i.e. field should be changed to flag?

<snip>
Thomas Monjalon Oct. 29, 2020, 4:15 p.m. UTC | #4 
29/10/2020 15:20, Pattan, Reshma:
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> 
> 
> <snip>
> 
> > 	rte_mbuf_dynflag_register(&timestamp_dynflag_desc);
> > +	if (timestamp_dynflag_offset < 0) {
> > +		RTE_LOG(ERR, LATENCY_STATS,
> > +				"Cannot register mbuf field for timestamp\n");
> 
> Field->flag, i.e. field should be changed to flag?

Yes good catch, thanks!
diff mbox series

Patch

diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index ba2fff3bcb..a21f6239d9 100644
--- a/lib/librte_latencystats/rte_latencystats.c
+++ b/lib/librte_latencystats/rte_latencystats.c
@@ -8,7 +8,9 @@ 
 #include <math.h>
 
 #include <rte_string_fns.h>
+#include <rte_bitops.h>
 #include <rte_mbuf.h>
+#include <rte_mbuf_dyn.h>
 #include <rte_log.h>
 #include <rte_cycles.h>
 #include <rte_ethdev.h>
@@ -31,6 +33,16 @@  latencystat_cycles_per_ns(void)
 /* Macros for printing using RTE_LOG */
 #define RTE_LOGTYPE_LATENCY_STATS RTE_LOGTYPE_USER1
 
+static uint64_t timestamp_dynflag;
+static int timestamp_dynfield_offset = -1;
+
+static inline rte_mbuf_timestamp_t *
+timestamp_dynfield(struct rte_mbuf *mbuf)
+{
+	return RTE_MBUF_DYNFIELD(mbuf,
+			timestamp_dynfield_offset, rte_mbuf_timestamp_t *);
+}
+
 static const char *MZ_RTE_LATENCY_STATS = "rte_latencystats";
 static int latency_stats_index;
 static uint64_t samp_intvl;
@@ -128,10 +140,10 @@  add_time_stamps(uint16_t pid __rte_unused,
 		diff_tsc = now - prev_tsc;
 		timer_tsc += diff_tsc;
 
-		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) == 0
+		if ((pkts[i]->ol_flags & timestamp_dynflag) == 0
 				&& (timer_tsc >= samp_intvl)) {
-			pkts[i]->timestamp = now;
-			pkts[i]->ol_flags |= PKT_RX_TIMESTAMP;
+			*timestamp_dynfield(pkts[i]) = now;
+			pkts[i]->ol_flags |= timestamp_dynflag;
 			timer_tsc = 0;
 		}
 		prev_tsc = now;
@@ -161,8 +173,8 @@  calc_latency(uint16_t pid __rte_unused,
 
 	now = rte_rdtsc();
 	for (i = 0; i < nb_pkts; i++) {
-		if (pkts[i]->ol_flags & PKT_RX_TIMESTAMP)
-			latency[cnt++] = now - pkts[i]->timestamp;
+		if (pkts[i]->ol_flags & timestamp_dynflag)
+			latency[cnt++] = now - *timestamp_dynfield(pkts[i]);
 	}
 
 	rte_spinlock_lock(&glob_stats->lock);
@@ -204,6 +216,14 @@  int
 rte_latencystats_init(uint64_t app_samp_intvl,
 		rte_latency_stats_flow_type_fn user_cb)
 {
+	static const struct rte_mbuf_dynfield timestamp_dynfield_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 timestamp_dynflag_desc = {
+		.name = RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME,
+	};
 	unsigned int i;
 	uint16_t pid;
 	uint16_t qid;
@@ -211,6 +231,7 @@  rte_latencystats_init(uint64_t app_samp_intvl,
 	const char *ptr_strings[NUM_LATENCY_STATS] = {0};
 	const struct rte_memzone *mz = NULL;
 	const unsigned int flags = 0;
+	int timestamp_dynflag_offset;
 	int ret;
 
 	if (rte_memzone_lookup(MZ_RTE_LATENCY_STATS))
@@ -241,6 +262,23 @@  rte_latencystats_init(uint64_t app_samp_intvl,
 		return -1;
 	}
 
+	/* Register mbuf field and flag for Rx timestamp */
+	timestamp_dynfield_offset =
+			rte_mbuf_dynfield_register(&timestamp_dynfield_desc);
+	if (timestamp_dynfield_offset < 0) {
+		RTE_LOG(ERR, LATENCY_STATS,
+				"Cannot register mbuf field for timestamp\n");
+		return -rte_errno;
+	}
+	timestamp_dynflag_offset =
+			rte_mbuf_dynflag_register(&timestamp_dynflag_desc);
+	if (timestamp_dynflag_offset < 0) {
+		RTE_LOG(ERR, LATENCY_STATS,
+				"Cannot register mbuf field for timestamp\n");
+		return -rte_errno;
+	}
+	timestamp_dynflag = RTE_BIT64(timestamp_dynflag_offset);
+
 	/** Register Rx/Tx callbacks */
 	RTE_ETH_FOREACH_DEV(pid) {
 		struct rte_eth_dev_info dev_info;