[PATCH 0/1] eal: add tracepoints to track lcores and services

[PATCH 0/1] eal: add tracepoints to track lcores and services

[Arnaud Fiorini]

The events generated by these tracepoints are then used in Trace
Compass[1] to show the lcore state and service state throughout the
execution of a program. A trace has been generated using the service
cores application and can be used to test the analysis.

[1] https://git.eclipse.org/r/c/tracecompass.incubator/org.eclipse.tracecompass.incubator/+/200457

Arnaud Fiorini (1):
  eal: add tracepoints to track lcores and services

 .mailmap                                 |  1 +
 lib/eal/common/eal_common_thread.c       |  4 ++
 lib/eal/common/eal_common_trace_points.c | 21 +++++++++
 lib/eal/common/rte_service.c             | 18 ++++++-
 lib/eal/include/eal_trace_internal.h     | 60 ++++++++++++++++++++++++
 5 files changed, 103 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/1] eal: add tracepoints to track lcores and services

[Arnaud Fiorini]

The tracepoints added are used to track lcore role and status,
as well as service mapping and service runstates. These
tracepoints are then used in analyses in Trace Compass.

Signed-off-by: Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
---
 .mailmap                                 |  1 +
 lib/eal/common/eal_common_thread.c       |  4 ++
 lib/eal/common/eal_common_trace_points.c | 21 +++++++++
 lib/eal/common/rte_service.c             | 18 ++++++-
 lib/eal/include/eal_trace_internal.h     | 60 ++++++++++++++++++++++++
 5 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index dc30369117..2a0b132572 100644
--- a/.mailmap
+++ b/.mailmap
@@ -120,6 +120,7 @@ Archana Muniganti <marchana@marvell.com> <muniganti.archana@caviumnetworks.com>
 Archit Pandey <architpandeynitk@gmail.com>
 Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
 Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
+Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
 Arnon Warshavsky <arnon@qwilt.com>
 Arshdeep Kaur <arshdeep.kaur@intel.com>
 Artem V. Andreev <artem.andreev@oktetlabs.ru>
diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 079a385630..25dbdd68e3 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -205,6 +205,8 @@ eal_thread_loop(void *arg)
 				__ATOMIC_ACQUIRE)) == NULL)
 			rte_pause();
 
+		rte_eal_trace_thread_lcore_running(lcore_id, f);
+
 		/* call the function and store the return value */
 		fct_arg = lcore_config[lcore_id].arg;
 		ret = f(fct_arg);
@@ -219,6 +221,8 @@ eal_thread_loop(void *arg)
 		 */
 		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
 			__ATOMIC_RELEASE);
+
+		rte_eal_trace_thread_lcore_stopped(lcore_id);
 	}
 
 	/* never reached */
diff --git a/lib/eal/common/eal_common_trace_points.c b/lib/eal/common/eal_common_trace_points.c
index 3f5bf5c55c..0f1240ea3a 100644
--- a/lib/eal/common/eal_common_trace_points.c
+++ b/lib/eal/common/eal_common_trace_points.c
@@ -70,6 +70,27 @@ RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_remote_launch,
 	lib.eal.thread.remote.launch)
 RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_ready,
 	lib.eal.thread.lcore.ready)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_running,
+	lib.eal.thread.lcore.running)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_stopped,
+	lib.eal.thread.lcore.stopped)
+
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_map_lcore,
+	lib.eal.service.map.lcore)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_state_change,
+	lib.eal.service.lcore.state.change)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_start,
+	lib.eal.service.lcore.start)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_stop,
+	lib.eal.service.lcore.stop)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_run_begin,
+	lib.eal.service.run.begin)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_runstate_set,
+	lib.eal.service.run.state.set)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_run_end,
+	lib.eal.service.run.end)
+RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_component_register,
+	lib.eal.service.component.register)
 
 RTE_TRACE_POINT_REGISTER(rte_eal_trace_intr_callback_register,
 	lib.eal.intr.register)
diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
index 42ca1d001d..5daec007aa 100644
--- a/lib/eal/common/rte_service.c
+++ b/lib/eal/common/rte_service.c
@@ -9,6 +9,7 @@
 #include <rte_service.h>
 #include <rte_service_component.h>
 
+#include <eal_trace_internal.h>
 #include <rte_lcore.h>
 #include <rte_branch_prediction.h>
 #include <rte_common.h>
@@ -16,6 +17,7 @@
 #include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_spinlock.h>
+#include <rte_trace_point.h>
 
 #include "eal_private.h"
 
@@ -276,6 +278,8 @@ rte_service_component_register(const struct rte_service_spec *spec,
 	if (id_ptr)
 		*id_ptr = free_slot;
 
+	rte_eal_trace_service_component_register(free_slot, spec->name);
+
 	return 0;
 }
 
@@ -336,6 +340,7 @@ rte_service_runstate_set(uint32_t id, uint32_t runstate)
 		__atomic_store_n(&s->app_runstate, RUNSTATE_STOPPED,
 			__ATOMIC_RELEASE);
 
+	rte_eal_trace_service_runstate_set(id, runstate);
 	return 0;
 }
 
@@ -427,11 +432,15 @@ service_run(uint32_t i, struct core_state *cs, uint64_t service_mask,
 		if (!rte_spinlock_trylock(&s->execute_lock))
 			return -EBUSY;
 
+		rte_eal_trace_service_run_begin(i, rte_lcore_id());
 		service_runner_do_callback(s, cs, i);
 		rte_spinlock_unlock(&s->execute_lock);
-	} else
+	} else {
+		rte_eal_trace_service_run_begin(i, rte_lcore_id());
 		service_runner_do_callback(s, cs, i);
+	}
 
+	rte_eal_trace_service_run_end(i, rte_lcore_id());
 	return 0;
 }
 
@@ -658,6 +667,7 @@ int32_t
 rte_service_map_lcore_set(uint32_t id, uint32_t lcore, uint32_t enabled)
 {
 	uint32_t on = enabled > 0;
+	rte_eal_trace_service_map_lcore(id, lcore, enabled);
 	return service_update(id, lcore, &on, 0);
 }
 
@@ -683,6 +693,8 @@ set_lcore_state(uint32_t lcore, int32_t state)
 
 	/* update per-lcore optimized state tracking */
 	lcore_states[lcore].is_service_core = (state == ROLE_SERVICE);
+
+	rte_eal_trace_service_lcore_state_change(lcore, state);
 }
 
 int32_t
@@ -780,6 +792,8 @@ rte_service_lcore_start(uint32_t lcore)
 	 */
 	__atomic_store_n(&cs->runstate, RUNSTATE_RUNNING, __ATOMIC_RELEASE);
 
+	rte_eal_trace_service_lcore_start(lcore);
+
 	int ret = rte_eal_remote_launch(service_runner_func, 0, lcore);
 	/* returns -EBUSY if the core is already launched, 0 on success */
 	return ret;
@@ -824,6 +838,8 @@ rte_service_lcore_stop(uint32_t lcore)
 	__atomic_store_n(&lcore_states[lcore].runstate, RUNSTATE_STOPPED,
 		__ATOMIC_RELEASE);
 
+	rte_eal_trace_service_lcore_stop(lcore);
+
 	return 0;
 }
 
diff --git a/lib/eal/include/eal_trace_internal.h b/lib/eal/include/eal_trace_internal.h
index 57d6de2535..9a37a08567 100644
--- a/lib/eal/include/eal_trace_internal.h
+++ b/lib/eal/include/eal_trace_internal.h
@@ -174,6 +174,66 @@ RTE_TRACE_POINT(
 	rte_trace_point_emit_u32(lcore_id);
 	rte_trace_point_emit_string(cpuset);
 )
+RTE_TRACE_POINT_FP(
+	rte_eal_trace_thread_lcore_running,
+	RTE_TRACE_POINT_ARGS(unsigned int lcore_id, void *f),
+	rte_trace_point_emit_u32(lcore_id);
+	rte_trace_point_emit_ptr(f);
+)
+RTE_TRACE_POINT_FP(
+	rte_eal_trace_thread_lcore_stopped,
+	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
+	rte_trace_point_emit_u32(lcore_id);
+)
+
+/* Service */
+RTE_TRACE_POINT(
+	rte_eal_trace_service_map_lcore,
+	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id, unsigned int enabled),
+	rte_trace_point_emit_u32(id);
+	rte_trace_point_emit_u32(lcore_id);
+	rte_trace_point_emit_u32(enabled);
+)
+RTE_TRACE_POINT(
+	rte_eal_trace_service_lcore_state_change,
+	RTE_TRACE_POINT_ARGS(unsigned int lcore_id, int lcore_state),
+	rte_trace_point_emit_u32(lcore_id);
+	rte_trace_point_emit_i32(lcore_state);
+)
+RTE_TRACE_POINT(
+	rte_eal_trace_service_lcore_start,
+	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
+	rte_trace_point_emit_u32(lcore_id);
+)
+RTE_TRACE_POINT(
+	rte_eal_trace_service_lcore_stop,
+	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
+	rte_trace_point_emit_u32(lcore_id);
+)
+RTE_TRACE_POINT_FP(
+	rte_eal_trace_service_run_begin,
+	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id),
+	rte_trace_point_emit_u32(id);
+	rte_trace_point_emit_u32(lcore_id);
+)
+RTE_TRACE_POINT(
+	rte_eal_trace_service_runstate_set,
+	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int run_state),
+	rte_trace_point_emit_u32(id);
+	rte_trace_point_emit_u32(run_state);
+)
+RTE_TRACE_POINT(
+	rte_eal_trace_service_run_end,
+	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id),
+	rte_trace_point_emit_u32(id);
+	rte_trace_point_emit_u32(lcore_id);
+)
+RTE_TRACE_POINT(
+	rte_eal_trace_service_component_register,
+	RTE_TRACE_POINT_ARGS(int id, const char *service_name),
+	rte_trace_point_emit_i32(id);
+	rte_trace_point_emit_string(service_name);
+)
 
 #ifdef __cplusplus
 }
-- 
2.25.1

RE: [PATCH 1/1] eal: add tracepoints to track lcores and services

[Van Haaren, Harry]

> -----Original Message-----
> From: Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
> Sent: Monday, April 24, 2023 4:24 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob <jerinj@marvell.com>;
> Sunil Kumar Kori <skori@marvell.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
> Subject: [PATCH 1/1] eal: add tracepoints to track lcores and services
> 
> The tracepoints added are used to track lcore role and status,
> as well as service mapping and service runstates. These
> tracepoints are then used in analyses in Trace Compass.
> 
> Signed-off-by: Arnaud Fiorini <arnaud.fiorini@polymtl.ca>

I've had a look at these changes, and don't have any big objections;
When disabled, the tracepoints add no measurable overhead here, but
When enabled they cause a ~+50% cycle-cost (eg from ~100 to 150 cycles).
Running the "service_perf_autotest" prints cycle-costs, so this is easy to check.

Please add documentation on enabling the traces; today it is hard to identify/find
an example of enabling tracing on service-cores. Suggest to add it to:
1) the commit message, how to enable service cores traces only
2) add a section in the service-cores documentation, to enable service-cores only, and link to tracing documentation page for more info.

+CC Mattias and Honnappa who have expressed specific interest in service-cores
Performance in the past, any concerns/input Mattias/Honnappa?

@Arnaud, assuming no concerns from wider DPDK community, I'm happy to Ack a v2 with docs added.


> ---
>  .mailmap                                 |  1 +
>  lib/eal/common/eal_common_thread.c       |  4 ++
>  lib/eal/common/eal_common_trace_points.c | 21 +++++++++
>  lib/eal/common/rte_service.c             | 18 ++++++-
>  lib/eal/include/eal_trace_internal.h     | 60 ++++++++++++++++++++++++
>  5 files changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/.mailmap b/.mailmap
> index dc30369117..2a0b132572 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -120,6 +120,7 @@ Archana Muniganti <marchana@marvell.com>
> <muniganti.archana@caviumnetworks.com>
>  Archit Pandey <architpandeynitk@gmail.com>
>  Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>  Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
> +Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
>  Arnon Warshavsky <arnon@qwilt.com>
>  Arshdeep Kaur <arshdeep.kaur@intel.com>
>  Artem V. Andreev <artem.andreev@oktetlabs.ru>
> diff --git a/lib/eal/common/eal_common_thread.c
> b/lib/eal/common/eal_common_thread.c
> index 079a385630..25dbdd68e3 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -205,6 +205,8 @@ eal_thread_loop(void *arg)
>  				__ATOMIC_ACQUIRE)) == NULL)
>  			rte_pause();
> 
> +		rte_eal_trace_thread_lcore_running(lcore_id, f);
> +
>  		/* call the function and store the return value */
>  		fct_arg = lcore_config[lcore_id].arg;
>  		ret = f(fct_arg);
> @@ -219,6 +221,8 @@ eal_thread_loop(void *arg)
>  		 */
>  		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
>  			__ATOMIC_RELEASE);
> +
> +		rte_eal_trace_thread_lcore_stopped(lcore_id);
>  	}
> 
>  	/* never reached */
> diff --git a/lib/eal/common/eal_common_trace_points.c
> b/lib/eal/common/eal_common_trace_points.c
> index 3f5bf5c55c..0f1240ea3a 100644
> --- a/lib/eal/common/eal_common_trace_points.c
> +++ b/lib/eal/common/eal_common_trace_points.c
> @@ -70,6 +70,27 @@
> RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_remote_launch,
>  	lib.eal.thread.remote.launch)
>  RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_ready,
>  	lib.eal.thread.lcore.ready)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_running,
> +	lib.eal.thread.lcore.running)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_stopped,
> +	lib.eal.thread.lcore.stopped)
> +
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_map_lcore,
> +	lib.eal.service.map.lcore)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_state_change,
> +	lib.eal.service.lcore.state.change)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_start,
> +	lib.eal.service.lcore.start)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_stop,
> +	lib.eal.service.lcore.stop)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_run_begin,
> +	lib.eal.service.run.begin)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_runstate_set,
> +	lib.eal.service.run.state.set)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_run_end,
> +	lib.eal.service.run.end)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_component_register,
> +	lib.eal.service.component.register)
> 
>  RTE_TRACE_POINT_REGISTER(rte_eal_trace_intr_callback_register,
>  	lib.eal.intr.register)
> diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
> index 42ca1d001d..5daec007aa 100644
> --- a/lib/eal/common/rte_service.c
> +++ b/lib/eal/common/rte_service.c
> @@ -9,6 +9,7 @@
>  #include <rte_service.h>
>  #include <rte_service_component.h>
> 
> +#include <eal_trace_internal.h>
>  #include <rte_lcore.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_common.h>
> @@ -16,6 +17,7 @@
>  #include <rte_atomic.h>
>  #include <rte_malloc.h>
>  #include <rte_spinlock.h>
> +#include <rte_trace_point.h>
> 
>  #include "eal_private.h"
> 
> @@ -276,6 +278,8 @@ rte_service_component_register(const struct
> rte_service_spec *spec,
>  	if (id_ptr)
>  		*id_ptr = free_slot;
> 
> +	rte_eal_trace_service_component_register(free_slot, spec->name);
> +
>  	return 0;
>  }
> 
> @@ -336,6 +340,7 @@ rte_service_runstate_set(uint32_t id, uint32_t runstate)
>  		__atomic_store_n(&s->app_runstate, RUNSTATE_STOPPED,
>  			__ATOMIC_RELEASE);
> 
> +	rte_eal_trace_service_runstate_set(id, runstate);
>  	return 0;
>  }
> 
> @@ -427,11 +432,15 @@ service_run(uint32_t i, struct core_state *cs, uint64_t
> service_mask,
>  		if (!rte_spinlock_trylock(&s->execute_lock))
>  			return -EBUSY;
> 
> +		rte_eal_trace_service_run_begin(i, rte_lcore_id());
>  		service_runner_do_callback(s, cs, i);
>  		rte_spinlock_unlock(&s->execute_lock);
> -	} else
> +	} else {
> +		rte_eal_trace_service_run_begin(i, rte_lcore_id());
>  		service_runner_do_callback(s, cs, i);
> +	}
> 
> +	rte_eal_trace_service_run_end(i, rte_lcore_id());
>  	return 0;
>  }
> 
> @@ -658,6 +667,7 @@ int32_t
>  rte_service_map_lcore_set(uint32_t id, uint32_t lcore, uint32_t enabled)
>  {
>  	uint32_t on = enabled > 0;
> +	rte_eal_trace_service_map_lcore(id, lcore, enabled);
>  	return service_update(id, lcore, &on, 0);
>  }
> 
> @@ -683,6 +693,8 @@ set_lcore_state(uint32_t lcore, int32_t state)
> 
>  	/* update per-lcore optimized state tracking */
>  	lcore_states[lcore].is_service_core = (state == ROLE_SERVICE);
> +
> +	rte_eal_trace_service_lcore_state_change(lcore, state);
>  }
> 
>  int32_t
> @@ -780,6 +792,8 @@ rte_service_lcore_start(uint32_t lcore)
>  	 */
>  	__atomic_store_n(&cs->runstate, RUNSTATE_RUNNING,
> __ATOMIC_RELEASE);
> 
> +	rte_eal_trace_service_lcore_start(lcore);
> +
>  	int ret = rte_eal_remote_launch(service_runner_func, 0, lcore);
>  	/* returns -EBUSY if the core is already launched, 0 on success */
>  	return ret;
> @@ -824,6 +838,8 @@ rte_service_lcore_stop(uint32_t lcore)
>  	__atomic_store_n(&lcore_states[lcore].runstate, RUNSTATE_STOPPED,
>  		__ATOMIC_RELEASE);
> 
> +	rte_eal_trace_service_lcore_stop(lcore);
> +
>  	return 0;
>  }
> 
> diff --git a/lib/eal/include/eal_trace_internal.h
> b/lib/eal/include/eal_trace_internal.h
> index 57d6de2535..9a37a08567 100644
> --- a/lib/eal/include/eal_trace_internal.h
> +++ b/lib/eal/include/eal_trace_internal.h
> @@ -174,6 +174,66 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_u32(lcore_id);
>  	rte_trace_point_emit_string(cpuset);
>  )
> +RTE_TRACE_POINT_FP(
> +	rte_eal_trace_thread_lcore_running,
> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id, void *f),
> +	rte_trace_point_emit_u32(lcore_id);
> +	rte_trace_point_emit_ptr(f);
> +)
> +RTE_TRACE_POINT_FP(
> +	rte_eal_trace_thread_lcore_stopped,
> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
> +	rte_trace_point_emit_u32(lcore_id);
> +)
> +
> +/* Service */
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_map_lcore,
> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id, unsigned int
> enabled),
> +	rte_trace_point_emit_u32(id);
> +	rte_trace_point_emit_u32(lcore_id);
> +	rte_trace_point_emit_u32(enabled);
> +)
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_lcore_state_change,
> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id, int lcore_state),
> +	rte_trace_point_emit_u32(lcore_id);
> +	rte_trace_point_emit_i32(lcore_state);
> +)
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_lcore_start,
> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
> +	rte_trace_point_emit_u32(lcore_id);
> +)
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_lcore_stop,
> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
> +	rte_trace_point_emit_u32(lcore_id);
> +)
> +RTE_TRACE_POINT_FP(
> +	rte_eal_trace_service_run_begin,
> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id),
> +	rte_trace_point_emit_u32(id);
> +	rte_trace_point_emit_u32(lcore_id);
> +)
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_runstate_set,
> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int run_state),
> +	rte_trace_point_emit_u32(id);
> +	rte_trace_point_emit_u32(run_state);
> +)
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_run_end,
> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id),
> +	rte_trace_point_emit_u32(id);
> +	rte_trace_point_emit_u32(lcore_id);
> +)
> +RTE_TRACE_POINT(
> +	rte_eal_trace_service_component_register,
> +	RTE_TRACE_POINT_ARGS(int id, const char *service_name),
> +	rte_trace_point_emit_i32(id);
> +	rte_trace_point_emit_string(service_name);
> +)
> 
>  #ifdef __cplusplus
>  }
> --
> 2.25.1

Re: [PATCH 1/1] eal: add tracepoints to track lcores and services

[Mattias Rönnblom]

On 2023-05-03 12:14, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
>> Sent: Monday, April 24, 2023 4:24 PM
>> To: Thomas Monjalon <thomas@monjalon.net>; Jerin Jacob <jerinj@marvell.com>;
>> Sunil Kumar Kori <skori@marvell.com>; Van Haaren, Harry
>> <harry.van.haaren@intel.com>
>> Cc: dev@dpdk.org; Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
>> Subject: [PATCH 1/1] eal: add tracepoints to track lcores and services
>>
>> The tracepoints added are used to track lcore role and status,
>> as well as service mapping and service runstates. These
>> tracepoints are then used in analyses in Trace Compass.
>>
>> Signed-off-by: Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
> 
> I've had a look at these changes, and don't have any big objections;
> When disabled, the tracepoints add no measurable overhead here, but
> When enabled they cause a ~+50% cycle-cost (eg from ~100 to 150 cycles).
> Running the "service_perf_autotest" prints cycle-costs, so this is easy to check.
> 
> Please add documentation on enabling the traces; today it is hard to identify/find
> an example of enabling tracing on service-cores. Suggest to add it to:
> 1) the commit message, how to enable service cores traces only
> 2) add a section in the service-cores documentation, to enable service-cores only, and link to tracing documentation page for more info.
> 
> +CC Mattias and Honnappa who have expressed specific interest in service-cores
> Performance in the past, any concerns/input Mattias/Honnappa?
> 

Adding tracepoints to DPDK services seems like a great idea to me.

> @Arnaud, assuming no concerns from wider DPDK community, I'm happy to Ack a v2 with docs added.
> 
> 
>> ---
>>   .mailmap                                 |  1 +
>>   lib/eal/common/eal_common_thread.c       |  4 ++
>>   lib/eal/common/eal_common_trace_points.c | 21 +++++++++
>>   lib/eal/common/rte_service.c             | 18 ++++++-
>>   lib/eal/include/eal_trace_internal.h     | 60 ++++++++++++++++++++++++
>>   5 files changed, 103 insertions(+), 1 deletion(-)
>>
>> diff --git a/.mailmap b/.mailmap
>> index dc30369117..2a0b132572 100644
>> --- a/.mailmap
>> +++ b/.mailmap
>> @@ -120,6 +120,7 @@ Archana Muniganti <marchana@marvell.com>
>> <muniganti.archana@caviumnetworks.com>
>>   Archit Pandey <architpandeynitk@gmail.com>
>>   Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>   Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
>> +Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
>>   Arnon Warshavsky <arnon@qwilt.com>
>>   Arshdeep Kaur <arshdeep.kaur@intel.com>
>>   Artem V. Andreev <artem.andreev@oktetlabs.ru>
>> diff --git a/lib/eal/common/eal_common_thread.c
>> b/lib/eal/common/eal_common_thread.c
>> index 079a385630..25dbdd68e3 100644
>> --- a/lib/eal/common/eal_common_thread.c
>> +++ b/lib/eal/common/eal_common_thread.c
>> @@ -205,6 +205,8 @@ eal_thread_loop(void *arg)
>>   				__ATOMIC_ACQUIRE)) == NULL)
>>   			rte_pause();
>>
>> +		rte_eal_trace_thread_lcore_running(lcore_id, f);
>> +
>>   		/* call the function and store the return value */
>>   		fct_arg = lcore_config[lcore_id].arg;
>>   		ret = f(fct_arg);
>> @@ -219,6 +221,8 @@ eal_thread_loop(void *arg)
>>   		 */
>>   		__atomic_store_n(&lcore_config[lcore_id].state, WAIT,
>>   			__ATOMIC_RELEASE);
>> +
>> +		rte_eal_trace_thread_lcore_stopped(lcore_id);
>>   	}
>>
>>   	/* never reached */
>> diff --git a/lib/eal/common/eal_common_trace_points.c
>> b/lib/eal/common/eal_common_trace_points.c
>> index 3f5bf5c55c..0f1240ea3a 100644
>> --- a/lib/eal/common/eal_common_trace_points.c
>> +++ b/lib/eal/common/eal_common_trace_points.c
>> @@ -70,6 +70,27 @@
>> RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_remote_launch,
>>   	lib.eal.thread.remote.launch)
>>   RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_ready,
>>   	lib.eal.thread.lcore.ready)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_running,
>> +	lib.eal.thread.lcore.running)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_thread_lcore_stopped,
>> +	lib.eal.thread.lcore.stopped)
>> +
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_map_lcore,
>> +	lib.eal.service.map.lcore)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_state_change,
>> +	lib.eal.service.lcore.state.change)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_start,
>> +	lib.eal.service.lcore.start)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_lcore_stop,
>> +	lib.eal.service.lcore.stop)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_run_begin,
>> +	lib.eal.service.run.begin)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_runstate_set,
>> +	lib.eal.service.run.state.set)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_run_end,
>> +	lib.eal.service.run.end)
>> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_service_component_register,
>> +	lib.eal.service.component.register)
>>
>>   RTE_TRACE_POINT_REGISTER(rte_eal_trace_intr_callback_register,
>>   	lib.eal.intr.register)
>> diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
>> index 42ca1d001d..5daec007aa 100644
>> --- a/lib/eal/common/rte_service.c
>> +++ b/lib/eal/common/rte_service.c
>> @@ -9,6 +9,7 @@
>>   #include <rte_service.h>
>>   #include <rte_service_component.h>
>>
>> +#include <eal_trace_internal.h>
>>   #include <rte_lcore.h>
>>   #include <rte_branch_prediction.h>
>>   #include <rte_common.h>
>> @@ -16,6 +17,7 @@
>>   #include <rte_atomic.h>
>>   #include <rte_malloc.h>
>>   #include <rte_spinlock.h>
>> +#include <rte_trace_point.h>
>>
>>   #include "eal_private.h"
>>
>> @@ -276,6 +278,8 @@ rte_service_component_register(const struct
>> rte_service_spec *spec,
>>   	if (id_ptr)
>>   		*id_ptr = free_slot;
>>
>> +	rte_eal_trace_service_component_register(free_slot, spec->name);
>> +
>>   	return 0;
>>   }
>>
>> @@ -336,6 +340,7 @@ rte_service_runstate_set(uint32_t id, uint32_t runstate)
>>   		__atomic_store_n(&s->app_runstate, RUNSTATE_STOPPED,
>>   			__ATOMIC_RELEASE);
>>
>> +	rte_eal_trace_service_runstate_set(id, runstate);
>>   	return 0;
>>   }
>>
>> @@ -427,11 +432,15 @@ service_run(uint32_t i, struct core_state *cs, uint64_t
>> service_mask,
>>   		if (!rte_spinlock_trylock(&s->execute_lock))
>>   			return -EBUSY;
>>
>> +		rte_eal_trace_service_run_begin(i, rte_lcore_id());

Should you move the trace point to the runner function, to avoid 
duplication?

>>   		service_runner_do_callback(s, cs, i);
>>   		rte_spinlock_unlock(&s->execute_lock);
>> -	} else
>> +	} else {
>> +		rte_eal_trace_service_run_begin(i, rte_lcore_id());
>>   		service_runner_do_callback(s, cs, i);
>> +	}
>>
>> +	rte_eal_trace_service_run_end(i, rte_lcore_id());
>>   	return 0;
>>   }
>>
>> @@ -658,6 +667,7 @@ int32_t
>>   rte_service_map_lcore_set(uint32_t id, uint32_t lcore, uint32_t enabled)
>>   {
>>   	uint32_t on = enabled > 0;
>> +	rte_eal_trace_service_map_lcore(id, lcore, enabled);
>>   	return service_update(id, lcore, &on, 0);
>>   }
>>
>> @@ -683,6 +693,8 @@ set_lcore_state(uint32_t lcore, int32_t state)
>>
>>   	/* update per-lcore optimized state tracking */
>>   	lcore_states[lcore].is_service_core = (state == ROLE_SERVICE);
>> +
>> +	rte_eal_trace_service_lcore_state_change(lcore, state);
>>   }
>>
>>   int32_t
>> @@ -780,6 +792,8 @@ rte_service_lcore_start(uint32_t lcore)
>>   	 */
>>   	__atomic_store_n(&cs->runstate, RUNSTATE_RUNNING,
>> __ATOMIC_RELEASE);
>>
>> +	rte_eal_trace_service_lcore_start(lcore);
>> +
>>   	int ret = rte_eal_remote_launch(service_runner_func, 0, lcore);
>>   	/* returns -EBUSY if the core is already launched, 0 on success */
>>   	return ret;
>> @@ -824,6 +838,8 @@ rte_service_lcore_stop(uint32_t lcore)
>>   	__atomic_store_n(&lcore_states[lcore].runstate, RUNSTATE_STOPPED,
>>   		__ATOMIC_RELEASE);
>>
>> +	rte_eal_trace_service_lcore_stop(lcore);
>> +
>>   	return 0;
>>   }
>>
>> diff --git a/lib/eal/include/eal_trace_internal.h
>> b/lib/eal/include/eal_trace_internal.h
>> index 57d6de2535..9a37a08567 100644
>> --- a/lib/eal/include/eal_trace_internal.h
>> +++ b/lib/eal/include/eal_trace_internal.h
>> @@ -174,6 +174,66 @@ RTE_TRACE_POINT(
>>   	rte_trace_point_emit_u32(lcore_id);
>>   	rte_trace_point_emit_string(cpuset);
>>   )
>> +RTE_TRACE_POINT_FP(
>> +	rte_eal_trace_thread_lcore_running,
>> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id, void *f),
>> +	rte_trace_point_emit_u32(lcore_id);

There's a certain asymmetry here, with lcore_id being called both a 
uint32_t and unsigned int.

I see now there is no rte_tracepoint_emit_uint(), so maybe this is the 
best you can do.

Those two types are the same on anything DPDK supports anyway, I think, 
so it's a cosmetic issue only.

>> +	rte_trace_point_emit_ptr(f);
>> +)
>> +RTE_TRACE_POINT_FP(
>> +	rte_eal_trace_thread_lcore_stopped,
>> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
>> +	rte_trace_point_emit_u32(lcore_id);
>> +)
>> +
>> +/* Service */
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_map_lcore,
>> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id, unsigned int
>> enabled),
>> +	rte_trace_point_emit_u32(id);
>> +	rte_trace_point_emit_u32(lcore_id);
>> +	rte_trace_point_emit_u32(enabled);
>> +)
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_lcore_state_change,
>> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id, int lcore_state),
>> +	rte_trace_point_emit_u32(lcore_id);
>> +	rte_trace_point_emit_i32(lcore_state);
>> +)
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_lcore_start,
>> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
>> +	rte_trace_point_emit_u32(lcore_id);
>> +)
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_lcore_stop,
>> +	RTE_TRACE_POINT_ARGS(unsigned int lcore_id),
>> +	rte_trace_point_emit_u32(lcore_id);
>> +)
>> +RTE_TRACE_POINT_FP(
>> +	rte_eal_trace_service_run_begin,
>> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id),
>> +	rte_trace_point_emit_u32(id);
>> +	rte_trace_point_emit_u32(lcore_id);
>> +)
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_runstate_set,
>> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int run_state),
>> +	rte_trace_point_emit_u32(id);
>> +	rte_trace_point_emit_u32(run_state);
>> +)
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_run_end,
>> +	RTE_TRACE_POINT_ARGS(unsigned int id, unsigned int lcore_id),
>> +	rte_trace_point_emit_u32(id);
>> +	rte_trace_point_emit_u32(lcore_id);
>> +)
>> +RTE_TRACE_POINT(
>> +	rte_eal_trace_service_component_register,
>> +	RTE_TRACE_POINT_ARGS(int id, const char *service_name),
>> +	rte_trace_point_emit_i32(id);
>> +	rte_trace_point_emit_string(service_name);
>> +)
>>
>>   #ifdef __cplusplus
>>   }
>> --
>> 2.25.1