Re: [PATCH v4] dmadev: add tracepoints

[fengchengwen <fengchengwen@huawei.com>]

Hi Thomas,

On 2023/7/10 14:49, Thomas Monjalon wrote:
> 09/07/2023 05:23, fengchengwen:
>> 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
> 
> This is the test application, not the example.
> Please make sure examples/dma/ is compiling.

Work well with examples/dma (compiled with enable_trace_fp=true).

dpdk-dma   -a 0000:7b:00.0 -a 0000:7d:00.0 --file-prefix=feng -l 10-11 --trace=lib.dmadev.*  -- -c hw

./dpdk-dma, Worker Threads = 1, Copy Mode = hw,
Updating MAC = enabled, Rx Queues = 1, Ring Size = 2048
Force Min Copy Size = 0 Packet Data Room Size = 2048

Statistics for port 0 ------------------------------
Packets sent:                           26588760
Packets received:                       26589288
Packets dropped on tx:                       528
Packets dropped on copy:                       0
DMA channel 0
	 Total submitted ops: 26589288
	 Total completed ops: 26589288
	 Total failed ops: 0

Aggregate statistics ===============================
Total packets Tx:                      0 [pkt/s]
Total packets Rx:                      0 [pkt/s]
Total packets dropped:                 0 [pkt/s]
Total submitted ops:                   0 [ops/s]
Total completed ops:                   0 [ops/s]
Total failed ops:                      0 [ops/s]
====================================================
Closing port 0
0000:7d:00.0 hns3_dev_close(): Close port 0 finished
Stopping dmadev 0
EAL: Trace dir: /root/dpdk-traces/feng-2023-07-10-PM-11-19-07
Bye...

[localhost fengchengwen]# babeltrace /root/dpdk-traces/feng-2023-07-10-PM-11-19-07 | grep submit | head -1
[23:19:01.442334710] (+1.000143090) lib.dmadev.stats_get: { cpu_id = 0xA, name = "dpdk-dma" }, { dev_id = 0, vchan = 0x0, stats_submitted = 0x0, stats_completed = 0x0, stats_errors = 0x0, ret = 0 }
[localhost fengchengwen]# babeltrace /root/dpdk-traces/feng-2023-07-10-PM-11-19-07 | grep dmadev.completed | head -1
[23:19:08.440327670] (+0.997219191) lib.dmadev.completed: { cpu_id = 0xB, name = "rte-worker-11" }, { dev_id = 0, vchan = 0x0, nb_cpls = 0x20, last_idx_val = 47207, has_error_val = 0, ret = 0x0 }

> 
> Also, the test chkincs must run fine.

chkincs ?

> 
> 
> .
>