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

[Arnaud Fiorini <arnaud.fiorini@polymtl.ca>]

On 2023-05-12 10:30 a.m., Arnaud Fiorini wrote:
> 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 +
>   doc/guides/prog_guide/service_cores.rst  | 14 ++++++
>   lib/eal/common/eal_common_thread.c       |  4 ++
>   lib/eal/common/eal_common_trace_points.c | 21 +++++++++
>   lib/eal/common/rte_service.c             | 17 ++++++-
>   lib/eal/include/eal_trace_internal.h     | 60 ++++++++++++++++++++++++
>   6 files changed, 116 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index 0859104404..6453cb2aa5 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/doc/guides/prog_guide/service_cores.rst b/doc/guides/prog_guide/service_cores.rst
> index 270b875783..3b9a947e4c 100644
> --- a/doc/guides/prog_guide/service_cores.rst
> +++ b/doc/guides/prog_guide/service_cores.rst
> @@ -52,3 +52,17 @@ The service core library is capable of collecting runtime statistics like number
>   of calls to a specific service, and number of cycles used by the service. The
>   cycle count collection is dynamically configurable, allowing any application to
>   profile the services running on the system at any time.
> +
> +Service Core Tracing
> +~~~~~~~~~~~~~~~~~~~~
> +
> +The service core library is instrumented with tracepoints using the DPDK Trace
> +Library. These tracepoints allow you to track the service and logical cores
> +state. To activate tracing when launching a DPDK program it is necessary to use the
> +``--trace`` option to specify a regular expression to select which tracepoints
> +to enable. Here is an example if you want to only specify service core tracing::
> +
> +    ./dpdk/examples/service_cores/build/service_cores --trace="lib.eal.thread*" --trace="lib.eal.service*"
> +
> +See the `DPDK Trace Library <https://doc.dpdk.org/guides/prog_guide/trace_lib.html>`_
> +for details.
> 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 7ab48f2be8..94e872a08a 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;
>   }
>   
> @@ -368,6 +373,7 @@ static inline void
>   service_runner_do_callback(struct rte_service_spec_impl *s,
>   			   struct core_state *cs, uint32_t service_idx)
>   {
> +	rte_eal_trace_service_run_begin(service_idx, rte_lcore_id());
>   	void *userdata = s->spec.callback_userdata;
>   
>   	if (service_stats_enabled(s)) {
> @@ -395,8 +401,10 @@ service_runner_do_callback(struct rte_service_spec_impl *s,
>   
>   		__atomic_store_n(&service_stats->calls,
>   			service_stats->calls + 1, __ATOMIC_RELAXED);
> -	} else
> +	} else {
>   		s->spec.callback(userdata);
> +	}
> +	rte_eal_trace_service_run_end(service_idx, rte_lcore_id());
>   }
>   
>   
> @@ -658,6 +666,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 +692,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 +791,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 +837,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..09c354717f 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(
> +	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(
> +	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(
> +	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
>   }

Hi,

I added documentation, removed tracepoint duplication and remove the 
unnecessary use of RTE_TRACE_POINT_FP.

Are there any other changes needed for this patch ?