On 2023-05-03 12:14, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Arnaud Fiorini <arnaud.fior...@polymtl.ca>
>> Sent: Monday, April 24, 2023 4:24 PM
>> To: Thomas Monjalon <tho...@monjalon.net>; Jerin Jacob <jer...@marvell.com>;
>> Sunil Kumar Kori <sk...@marvell.com>; Van Haaren, Harry
>> <harry.van.haa...@intel.com>
>> Cc: dev@dpdk.org; Arnaud Fiorini <arnaud.fior...@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.fior...@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 <march...@marvell.com>
>> <muniganti.arch...@caviumnetworks.com>
>> Archit Pandey <architpandeyn...@gmail.com>
>> Arkadiusz Kubalewski <arkadiusz.kubalew...@intel.com>
>> Arkadiusz Kusztal <arkadiuszx.kusz...@intel.com>
>> +Arnaud Fiorini <arnaud.fior...@polymtl.ca>
>> Arnon Warshavsky <ar...@qwilt.com>
>> Arshdeep Kaur <arshdeep.k...@intel.com>
>> Artem V. Andreev <artem.andr...@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
