RE: [PATCH v4 1/6] eal: trace: add trace point emit for array

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> Sent: Thursday, 22 December 2022 07.33
> 
> Adds a trace point emit function for array. The maximum array
> bytes which can be captured is set to 32.

This seems very useful.

Disclaimer: I am not familiar with the trace library or the CTF file format, so my feedback below may be completely wrong and impossible - please keep in mind when reading.

> 
> Also adds test case for emit array tracepoint function.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> ---
>  app/test/test_trace.c                      |  3 +++
>  lib/eal/common/eal_common_trace_points.c   |  2 ++
>  lib/eal/include/rte_eal_trace.h            |  6 ++++++
>  lib/eal/include/rte_trace_point.h          | 20 ++++++++++++++++++++
>  lib/eal/include/rte_trace_point_register.h |  8 ++++++++
>  5 files changed, 39 insertions(+)
> 
> diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> index 6bedf14024..99cd0762d1 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -177,6 +177,7 @@ test_fp_trace_points(void)
>  static int
>  test_generic_trace_points(void)
>  {
> +	uint8_t arr[32] = {0};
>  	int tmp;
> 
>  	rte_eal_trace_generic_void();
> @@ -195,6 +196,8 @@ test_generic_trace_points(void)
>  	rte_eal_trace_generic_ptr(&tmp);
>  	rte_eal_trace_generic_str("my string");
>  	rte_eal_trace_generic_size_t(sizeof(void *));

Please also test smaller, unaligned length, e.g.:
rte_eal_trace_generic_char_array(arr, 17);

Please also test variable len, unknown at build time, e.g.:
rte_eal_trace_generic_char_array(arr, rand() % 31);

> +	rte_eal_trace_generic_char_array(arr, 32);
> +	rte_eal_trace_generic_char_array(arr, 64);
>  	RTE_EAL_TRACE_GENERIC_FUNC;
> 
>  	return TEST_SUCCESS;
> diff --git a/lib/eal/common/eal_common_trace_points.c
> b/lib/eal/common/eal_common_trace_points.c
> index 0b0b254615..93fdaa634e 100644
> --- a/lib/eal/common/eal_common_trace_points.c
> +++ b/lib/eal/common/eal_common_trace_points.c
> @@ -40,6 +40,8 @@
> RTE_TRACE_POINT_REGISTER(rte_eal_trace_generic_size_t,
>  	lib.eal.generic.size_t)
>  RTE_TRACE_POINT_REGISTER(rte_eal_trace_generic_func,
>  	lib.eal.generic.func)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_generic_char_array,
> +	lib.eal.generic.char.array)
> 
>  RTE_TRACE_POINT_REGISTER(rte_eal_trace_alarm_set,
>  	lib.eal.alarm.set)
> diff --git a/lib/eal/include/rte_eal_trace.h
> b/lib/eal/include/rte_eal_trace.h
> index 5ef4398230..34fdd5331f 100644
> --- a/lib/eal/include/rte_eal_trace.h
> +++ b/lib/eal/include/rte_eal_trace.h
> @@ -143,6 +143,12 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_string(func);
>  )
> 
> +RTE_TRACE_POINT(
> +	rte_eal_trace_generic_char_array,
> +	RTE_TRACE_POINT_ARGS(void *in, uint8_t len),
> +	rte_trace_point_emit_char_array(in, len);
> +)
> +
>  #define RTE_EAL_TRACE_GENERIC_FUNC
> rte_eal_trace_generic_func(__func__)
> 
>  /* Interrupt */
> diff --git a/lib/eal/include/rte_trace_point.h
> b/lib/eal/include/rte_trace_point.h
> index 0f8700974f..9d9a9e0aaa 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -144,6 +144,8 @@ _tp _args \
>  #define rte_trace_point_emit_ptr(val)
>  /** Tracepoint function payload for string datatype */
>  #define rte_trace_point_emit_string(val)
> +/** Tracepoint function payload for char array */
> +#define rte_trace_point_emit_char_array(val, len)
> 
>  #endif /* __DOXYGEN__ */
> 
> @@ -151,6 +153,8 @@ _tp _args \
>  #define __RTE_TRACE_EMIT_STRING_LEN_MAX 32
>  /** @internal Macro to define event header size. */
>  #define __RTE_TRACE_EVENT_HEADER_SZ sizeof(uint64_t)
> +/** @internal Macro to define maximum emit length of array. */
> +#define __RTE_TRACE_EMIT_ARRAY_LEN_MAX 32
> 
>  /**
>   * Enable recording events of the given tracepoint in the trace
> buffer.
> @@ -374,12 +378,28 @@ do { \
>  	mem = RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_STRING_LEN_MAX); \
>  } while (0)
> 
> +#define rte_trace_point_emit_char_array(in, len) \
> +do { \
> +	if (unlikely(in == NULL)) \
> +		return; \
> +	if (len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX) \
> +		return; \
> +	memcpy(mem, in, len); \
> +	mem = RTE_PTR_ADD(mem, len); \

This does not work for len < 32, because the trace format is defined to emit an array of 32 byte. You must always increment mem by 32:

mem = RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_ARRAY_LEN_MAX); \

Also, instead of silently ignoring len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX, you should:

if (len < __RTE_TRACE_EMIT_ARRAY_LEN_MAX) { \
	memcpy(mem, in, len); \
	memset(RTE_PTR_ADD(mem, len), 0, __RTE_TRACE_EMIT_ARRAY_LEN_MAX - len); \
} else { \
	memcpy(mem, in, __RTE_TRACE_EMIT_ARRAY_LEN_MAX); \
} \

If not emitting the length value (see my comments at the end), you should clear the unused part of the array; notice the added memset().

> +} while (0)
> +
>  #else
> 
>  #define __rte_trace_point_emit_header_generic(t) RTE_SET_USED(t)
>  #define __rte_trace_point_emit_header_fp(t) RTE_SET_USED(t)
>  #define __rte_trace_point_emit(in, type) RTE_SET_USED(in)
>  #define rte_trace_point_emit_string(in) RTE_SET_USED(in)
> +#define rte_trace_point_emit_char_array(in, len) \
> +do { \
> +	RTE_SET_USED(in); \
> +	RTE_SET_USED(len); \
> +} while (0)
> +
> 
>  #endif /* ALLOW_EXPERIMENTAL_API */
>  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
> diff --git a/lib/eal/include/rte_trace_point_register.h
> b/lib/eal/include/rte_trace_point_register.h
> index a32f4d731b..c76fe4dd48 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -47,6 +47,14 @@ do { \
>  		RTE_STR(in)"[32]", "string_bounded_t"); \
>  } while (0)
> 
> +#define rte_trace_point_emit_char_array(in, len) \
> +do { \
> +	RTE_SET_USED(in); \
> +	if (len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX) \
> +		return; \
> +	__rte_trace_point_emit_field(len, RTE_STR(in)"[32]", "uint8_t");
> \
> +} while (0)
> +
>  #ifdef __cplusplus
>  }
>  #endif
> --
> 2.25.1
> 

This emits 0..32 byte of binary data into a 32 byte array to the trace. But there is no way to read the length from the trace file, i.e. how many of the 32 bytes contain data.

If length is required to be constant at build time, perhaps the "[32]" in rte_trace_point_register.h can be modified to emit the length as the array size. (If so, the requirement for the length to be constant must be documented.)

Suggestion (not a requirement):

Instead of simply emitting an array, consider emitting a structure where the first field is the length, and the second field is an array of 32 bytes of data:

struct {
    integer {
        size = 8;
    } len;
    integer {
        size = 8;
    } data[32];
}

Or even better, emit a sequence [1]:

struct {
    integer {
        size = 16;
    } length;
    integer {
        size = 8;
    } data[length];
}

[1]: https://diamon.org/ctf/#spec4.2.4

If emitting a sequence, the length does not have to be limited to 32 byte, but could be up to 65535 byte.


And a personal preference about the name: I would prefer _blob instead of _char_array.