Re: [PATCH v4] dmadev: add tracepoints

[fengchengwen <fengchengwen@huawei.com>]

Hi Thomas,

On 2023/7/7 18:40, Thomas Monjalon wrote:
> 26/05/2023 10:42, Chengwen Feng:
>> Add tracepoints at important APIs for tracing support.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>
>> ---
>> v4: Fix asan smoke fail.
>> v3: Address Morten's comment:
>>     Move stats_get and vchan_status and to trace_fp.h.
>> v2: Address Morten's comment:
>>     Make stats_get as fast-path trace-points.
>>     Place fast-path trace-point functions behind in version.map.
> 
> There are more things to fix.
> First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.

It was already included by rte_dmadev.h:
diff --git a/lib/dmadev/rte_dmadev.h b/lib/dmadev/rte_dmadev.h
index e61d71959e..e792b90ef8 100644
--- a/lib/dmadev/rte_dmadev.h
+++ b/lib/dmadev/rte_dmadev.h
@@ -796,6 +796,7 @@ struct rte_dma_sge {
 };

 #include "rte_dmadev_core.h"
+#include "rte_dmadev_trace_fp.h"


> Note: you could have caught this if testing the example app for DMA.
> Second, you must avoid structs and enum in this header file,

Let me explain the #if #endif logic:

For the function:
uint16_t
rte_dma_completed(int16_t dev_id, uint16_t vchan, const uint16_t nb_cpls,
		  uint16_t *last_idx, bool *has_error)

The common trace implementation:
RTE_TRACE_POINT_FP(
	rte_dma_trace_completed,
	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
			     const uint16_t nb_cpls, uint16_t *last_idx,
			     bool *has_error, uint16_t ret),
	rte_trace_point_emit_i16(dev_id);
	rte_trace_point_emit_u16(vchan);
	rte_trace_point_emit_u16(nb_cpls);
	rte_trace_point_emit_ptr(idx_val);
	rte_trace_point_emit_ptr(has_error);
	rte_trace_point_emit_u16(ret);
)

But it has a problem: for pointer parameter (e.g. last_idx and has_error), only record
the pointer value (i.e. address value).

I think the pointer value has no mean (in particular, many of there pointers are stack
variables), the value of the pointer point to is meaningful.

So I add the pointer reference like below (as V3 did):
RTE_TRACE_POINT_FP(
	rte_dma_trace_completed,
	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
			     const uint16_t nb_cpls, uint16_t *last_idx,
			     bool *has_error, uint16_t ret),
	int has_error_val = *has_error;            // pointer reference
	int last_idx_val = *last_idx;              // pointer reference
	rte_trace_point_emit_i16(dev_id);
	rte_trace_point_emit_u16(vchan);
	rte_trace_point_emit_u16(nb_cpls);
	rte_trace_point_emit_int(last_idx_val);    // record the value of pointer
	rte_trace_point_emit_int(has_error_val);   // record the value of pointer
	rte_trace_point_emit_u16(ret);
)

Unfortunately, the above lead to asan failed. because in:
RTE_TRACE_POINT_REGISTER(rte_dma_trace_completed,
	lib.dmadev.completed)
it will invoke rte_dma_trace_completed() with the parameter is undefined.


To solve this problem, consider the rte_dmadev_trace_points.c will include rte_trace_point_register.h,
and the rte_trace_point_register.h will defined macro: _RTE_TRACE_POINT_REGISTER_H_.

so we update trace points as (as V4 did):
RTE_TRACE_POINT_FP(
	rte_dma_trace_completed,
	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
			     const uint16_t nb_cpls, uint16_t *last_idx,
			     bool *has_error, uint16_t ret),
#ifdef _RTE_TRACE_POINT_REGISTER_H_
	uint16_t __last_idx = 0;
	bool __has_error = false;
	last_idx = &__last_idx;                  // make sure the pointer has meaningful value.
	has_error = &__has_error;                // so that the next pointer reference will work well.
#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
	int has_error_val = *has_error;
	int last_idx_val = *last_idx;
	rte_trace_point_emit_i16(dev_id);
	rte_trace_point_emit_u16(vchan);
	rte_trace_point_emit_u16(nb_cpls);
	rte_trace_point_emit_int(last_idx_val);
	rte_trace_point_emit_int(has_error_val);
	rte_trace_point_emit_u16(ret);
)

> otherwise it cannot be included alone.
> Look at what is done in other *_trace_fp.h files.
> 
> 

Whether enable_trace_fp is true or false, the v4 work well.
Below is that run examples with enable_trace_fp=true.

./dpdk-test --file-prefix=feng123 --trace=lib.dmadev.* -l 10-11

EAL: Detected CPU lcores: 96
EAL: Detected NUMA nodes: 4
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/feng123/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: VFIO support initialized
TELEMETRY: No legacy callbacks, legacy socket not created
APP: HPET is not enabled, using TSC as default timer
RTE>>dmadev_autotest
skeldma_probe(): Create dma_skeleton dmadev with lcore-id -1

### Test dmadev infrastructure using skeleton driver
test_dma_get_dev_id_by_name Passed
test_dma_is_valid_dev Passed
test_dma_count Passed
test_dma_info_get Passed
test_dma_configure Passed
test_dma_vchan_setup Passed
test_dma_start_stop Passed
test_dma_stats Passed
test_dma_dump Passed
test_dma_completed Passed
test_dma_completed_status Passed
Total tests   : 11
Passed        : 11
Failed        : 0

### Test dmadev instance 0 [dma_skeleton]
DMA Dev 0: Running copy Tests
Ops submitted: 85120    Ops completed: 85120    Errors: 0
DMA Dev 0: Running stop-start Tests
Ops submitted: 1	Ops completed: 1	Errors: 0
DMA Dev 0: Running burst capacity Tests
Ops submitted: 65536	Ops completed: 65536	Errors: 0
DMA Dev 0: device does not report errors, skipping error handling tests
DMA Dev 0: No device fill support, skipping fill tests
Test OK
RTE>>
RTE>>quit
skeldma_remove(): Remove dma_skeleton dmadev
EAL: Trace dir: /root/dpdk-traces/feng123-2023-07-09-PM-06-57-08

[localhost fengchengwen]# babeltrace /root/dpdk-traces/feng123-2023-07-09-PM-06-57-08 | grep dma | head -1
[18:54:27.066265250] (+?.?????????) lib.dmadev.copy: { cpu_id = 0xA, name = "dpdk-test" }, { dev_id = 0, vchan = 0x0, src = 0x17F266480, dst = 0x17F292280, length = 0x400, flags = 0x0, ret = 63340 }
[localhost fengchengwen]# babeltrace /root/dpdk-traces/feng123-2023-07-09-PM-06-57-08 | grep completed | head -1
[18:54:27.066315770] (+0.000001670) lib.dmadev.completed: { cpu_id = 0xA, name = "dpdk-test" }, { dev_id = 0, vchan = 0x0, nb_cpls = 0x40, last_idx_val = 63296, has_error_val = 0, ret = 0x40 }

> 
> .
> 

Thanks