[v4] dmadev: add tracepoints

Message ID 20230526084213.30586-1-fengchengwen@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v4] dmadev: add tracepoints |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

fengchengwen May 26, 2023, 8:42 a.m. UTC
  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.

---
 lib/dmadev/meson.build               |   2 +-
 lib/dmadev/rte_dmadev.c              |  39 +++++--
 lib/dmadev/rte_dmadev.h              |  56 +++++++---
 lib/dmadev/rte_dmadev_trace.h        | 118 +++++++++++++++++++++
 lib/dmadev/rte_dmadev_trace_fp.h     | 150 +++++++++++++++++++++++++++
 lib/dmadev/rte_dmadev_trace_points.c |  59 +++++++++++
 lib/dmadev/version.map               |  10 ++
 7 files changed, 413 insertions(+), 21 deletions(-)
 create mode 100644 lib/dmadev/rte_dmadev_trace.h
 create mode 100644 lib/dmadev/rte_dmadev_trace_fp.h
 create mode 100644 lib/dmadev/rte_dmadev_trace_points.c
  

Comments

fengchengwen July 3, 2023, 3:54 a.m. UTC | #1
Hi Thomas,

  This version alaready fix asan smoke fail, could you help merge it ?

Thanks

On 2023/5/26 16:42, Chengwen Feng wrote:
> 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.
> 

...

>  
>
  
Thomas Monjalon July 7, 2023, 10:40 a.m. UTC | #2
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.
Note: you could have caught this if testing the example app for DMA.
Second, you must avoid structs and enum in this header file,
otherwise it cannot be included alone.
Look at what is done in other *_trace_fp.h files.
  
fengchengwen July 9, 2023, 3:23 a.m. UTC | #3
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
  
Thomas Monjalon July 10, 2023, 6:49 a.m. UTC | #4
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.

Also, the test chkincs must run fine.
  
fengchengwen July 10, 2023, 7:50 a.m. UTC | #5
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 ?

> 
> 
> .
>
  
Thomas Monjalon July 31, 2023, 12:48 p.m. UTC | #6
10/07/2023 09:50, fengchengwen:
> 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).

Can you try with enable_trace_fp=false (the default)?

> > Also, the test chkincs must run fine.
> 
> chkincs ?

If this a word you don't know, you can try "git grep" to better understand.
There is a Meson option "check_includes" to enable chkincs.

I recommend using devtools/test-meson-builds.sh to test patches,
it includes the above options.
  
fengchengwen Aug. 3, 2023, 7:52 a.m. UTC | #7
Hi Thomas,

On 2023/7/31 20:48, Thomas Monjalon wrote:
> 10/07/2023 09:50, fengchengwen:
>> 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).
> 
> Can you try with enable_trace_fp=false (the default)?

It works well too.

> 
>>> Also, the test chkincs must run fine.
>>
>> chkincs ?
> 
> If this a word you don't know, you can try "git grep" to better understand.
> There is a Meson option "check_includes" to enable chkincs.
> 
> I recommend using devtools/test-meson-builds.sh to test patches,
> it includes the above options.

According your suggest, I use test-meson-builds.sh, and pass.

TestEnv: x86_64
         gcc version: gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
         aarch64-linux-gnu-gcc version: gcc version 7.5.0 (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04)
Changes: 1) set 'DPDK_MESON_OPTIONS="max_numa_nodes=1"' in .develconfig (because don't have libnuma)
         2) disable compile event/* driver when build armv8 (fix compile drivers/event/cnxk/deq/cn10k/deq_*.c error: Error: reg pair must start from even reg at operand 1 -- `caspal x7,x8,x7,x8,[x4]')

> 
> 
> .
>
  
Thomas Monjalon Aug. 14, 2023, 2:16 p.m. UTC | #8
jeudi 3 août 2023, fengchengwen:
> Hi Thomas,
> 
> On 2023/7/31 20:48, Thomas Monjalon wrote:
> > 10/07/2023 09:50, fengchengwen:
> >> 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).
> > 
> > Can you try with enable_trace_fp=false (the default)?
> 
> It works well too.
> 
> > 
> >>> Also, the test chkincs must run fine.
> >>
> >> chkincs ?
> > 
> > If this a word you don't know, you can try "git grep" to better understand.
> > There is a Meson option "check_includes" to enable chkincs.
> > 
> > I recommend using devtools/test-meson-builds.sh to test patches,
> > it includes the above options.
> 
> According your suggest, I use test-meson-builds.sh, and pass.

It does not pass for me:

In file included from dmafwd.c:14:
build-x86-generic/install/usr/local/include/rte_dmadev.h:799:10:
fatal error: rte_dmadev_trace_fp.h: No such file or directory
  799 | #include "rte_dmadev_trace_fp.h"

Let me repeat again my recommendations:
First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
	YOU NEED TO ADD IT in meson.build FILE
Note: you could have caught this if testing the example app for DMA.
Second, you must avoid structs and enum in this header file,
otherwise it cannot be included alone.
Look at what is done in other *_trace_fp.h files.


> TestEnv: x86_64
>          gcc version: gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
>          aarch64-linux-gnu-gcc version: gcc version 7.5.0 (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04)
> Changes: 1) set 'DPDK_MESON_OPTIONS="max_numa_nodes=1"' in .develconfig (because don't have libnuma)
>          2) disable compile event/* driver when build armv8 (fix compile drivers/event/cnxk/deq/cn10k/deq_*.c error: Error: reg pair must start from even reg at operand 1 -- `caspal x7,x8,x7,x8,[x4]')

Is it an error related to your patch?
If not, please raise it in bugzilla to get it fixed.
  
fengchengwen Oct. 11, 2023, 9:55 a.m. UTC | #9
Hi Thomas,

  Sorry for the late reply.

On 2023/8/14 22:16, Thomas Monjalon wrote:
> jeudi 3 août 2023, fengchengwen:
>> Hi Thomas,
>>
>> On 2023/7/31 20:48, Thomas Monjalon wrote:
>>> 10/07/2023 09:50, fengchengwen:
>>>> 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).
>>>
>>> Can you try with enable_trace_fp=false (the default)?
>>
>> It works well too.
>>
>>>
>>>>> Also, the test chkincs must run fine.
>>>>
>>>> chkincs ?
>>>
>>> If this a word you don't know, you can try "git grep" to better understand.
>>> There is a Meson option "check_includes" to enable chkincs.
>>>
>>> I recommend using devtools/test-meson-builds.sh to test patches,
>>> it includes the above options.
>>
>> According your suggest, I use test-meson-builds.sh, and pass.
> 
> It does not pass for me:
> 
> In file included from dmafwd.c:14:
> build-x86-generic/install/usr/local/include/rte_dmadev.h:799:10:
> fatal error: rte_dmadev_trace_fp.h: No such file or directory
>   799 | #include "rte_dmadev_trace_fp.h"

I still can't reproduce the above error with .develconfig contain
"DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all"".

Could you provide the config options (which load by devtools/load-devel-config) ?

> 
> Let me repeat again my recommendations:
> First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
> 	YOU NEED TO ADD IT in meson.build FILE
> Note: you could have caught this if testing the example app for DMA.
> Second, you must avoid structs and enum in this header file,

Yes, I found compile error with .develconfig contain
"DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all enable_trace_fp=true""

The root cause is that the structs (e.g. struct rte_dma_sge) and enum (e.g. enum rte_dma_status_code)
usage in dmadev fastpath API.

I try to include "rte_dmadev.h" and it also not work (more compiler error).

So I think two options:
1. don't support fastpath tracepoints with dmadev library
2. exclude xxx_trace_fp.h in buildtools/chkincs

Would like to hear your opinion.

> otherwise it cannot be included alone.
> Look at what is done in other *_trace_fp.h files.
> 
> 
>> TestEnv: x86_64
>>          gcc version: gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
>>          aarch64-linux-gnu-gcc version: gcc version 7.5.0 (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04)
>> Changes: 1) set 'DPDK_MESON_OPTIONS="max_numa_nodes=1"' in .develconfig (because don't have libnuma)
>>          2) disable compile event/* driver when build armv8 (fix compile drivers/event/cnxk/deq/cn10k/deq_*.c error: Error: reg pair must start from even reg at operand 1 -- `caspal x7,x8,x7,x8,[x4]')
> 
> Is it an error related to your patch?
> If not, please raise it in bugzilla to get it fixed.
> 
> 
> 
> .
>
  
Thomas Monjalon Nov. 6, 2023, 8:59 p.m. UTC | #10
11/10/2023 11:55, fengchengwen:
> Hi Thomas,
> 
>   Sorry for the late reply.
> 
> On 2023/8/14 22:16, Thomas Monjalon wrote:
> > jeudi 3 août 2023, fengchengwen:
> >> Hi Thomas,
> >>
> >> On 2023/7/31 20:48, Thomas Monjalon wrote:
> >>> 10/07/2023 09:50, fengchengwen:
> >>>> 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).
> >>>
> >>> Can you try with enable_trace_fp=false (the default)?
> >>
> >> It works well too.
> >>
> >>>
> >>>>> Also, the test chkincs must run fine.
> >>>>
> >>>> chkincs ?
> >>>
> >>> If this a word you don't know, you can try "git grep" to better understand.
> >>> There is a Meson option "check_includes" to enable chkincs.
> >>>
> >>> I recommend using devtools/test-meson-builds.sh to test patches,
> >>> it includes the above options.
> >>
> >> According your suggest, I use test-meson-builds.sh, and pass.
> > 
> > It does not pass for me:
> > 
> > In file included from dmafwd.c:14:
> > build-x86-generic/install/usr/local/include/rte_dmadev.h:799:10:
> > fatal error: rte_dmadev_trace_fp.h: No such file or directory
> >   799 | #include "rte_dmadev_trace_fp.h"
> 
> I still can't reproduce the above error with .develconfig contain
> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all"".
> 
> Could you provide the config options (which load by devtools/load-devel-config) ?
> 
> > 
> > Let me repeat again my recommendations:
> > First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
> > 	YOU NEED TO ADD IT in meson.build FILE
> > Note: you could have caught this if testing the example app for DMA.
> > Second, you must avoid structs and enum in this header file,
> 
> Yes, I found compile error with .develconfig contain
> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all enable_trace_fp=true""
> 
> The root cause is that the structs (e.g. struct rte_dma_sge) and enum (e.g. enum rte_dma_status_code)
> usage in dmadev fastpath API.
> 
> I try to include "rte_dmadev.h" and it also not work (more compiler error).
> 
> So I think two options:
> 1. don't support fastpath tracepoints with dmadev library
> 2. exclude xxx_trace_fp.h in buildtools/chkincs
> 
> Would like to hear your opinion.

I've merged the patch for control path.
Please send a new patch for data path, I will test it,
and I will work with you to understand what happens.
Let's target it for 24.03 release.

Thanks
  
fengchengwen Nov. 7, 2023, 1:26 a.m. UTC | #11
Hi Thomas,

On 2023/11/7 4:59, Thomas Monjalon wrote:
> 11/10/2023 11:55, fengchengwen:
>> Hi Thomas,
>>
>>   Sorry for the late reply.
>>
>> On 2023/8/14 22:16, Thomas Monjalon wrote:
>>> jeudi 3 août 2023, fengchengwen:
>>>> Hi Thomas,
>>>>
>>>> On 2023/7/31 20:48, Thomas Monjalon wrote:
>>>>> 10/07/2023 09:50, fengchengwen:
>>>>>> 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).
>>>>>
>>>>> Can you try with enable_trace_fp=false (the default)?
>>>>
>>>> It works well too.
>>>>
>>>>>
>>>>>>> Also, the test chkincs must run fine.
>>>>>>
>>>>>> chkincs ?
>>>>>
>>>>> If this a word you don't know, you can try "git grep" to better understand.
>>>>> There is a Meson option "check_includes" to enable chkincs.
>>>>>
>>>>> I recommend using devtools/test-meson-builds.sh to test patches,
>>>>> it includes the above options.
>>>>
>>>> According your suggest, I use test-meson-builds.sh, and pass.
>>>
>>> It does not pass for me:
>>>
>>> In file included from dmafwd.c:14:
>>> build-x86-generic/install/usr/local/include/rte_dmadev.h:799:10:
>>> fatal error: rte_dmadev_trace_fp.h: No such file or directory
>>>   799 | #include "rte_dmadev_trace_fp.h"
>>
>> I still can't reproduce the above error with .develconfig contain
>> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all"".
>>
>> Could you provide the config options (which load by devtools/load-devel-config) ?
>>
>>>
>>> Let me repeat again my recommendations:
>>> First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
>>> 	YOU NEED TO ADD IT in meson.build FILE
>>> Note: you could have caught this if testing the example app for DMA.
>>> Second, you must avoid structs and enum in this header file,
>>
>> Yes, I found compile error with .develconfig contain
>> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all enable_trace_fp=true""
>>
>> The root cause is that the structs (e.g. struct rte_dma_sge) and enum (e.g. enum rte_dma_status_code)
>> usage in dmadev fastpath API.
>>
>> I try to include "rte_dmadev.h" and it also not work (more compiler error).
>>
>> So I think two options:
>> 1. don't support fastpath tracepoints with dmadev library
>> 2. exclude xxx_trace_fp.h in buildtools/chkincs
>>
>> Would like to hear your opinion.
> 
> I've merged the patch for control path.
> Please send a new patch for data path, I will test it,
> and I will work with you to understand what happens.
> Let's target it for 24.03 release.

Got it, thanks a lot.

> 
> Thanks
> 
> 
> .
>
  
fengchengwen Jan. 12, 2024, 10:38 a.m. UTC | #12
Hi Thomas,

On 2023/11/7 4:59, Thomas Monjalon wrote:
> 11/10/2023 11:55, fengchengwen:
>> Hi Thomas,
>>
>>   Sorry for the late reply.
>>
>> On 2023/8/14 22:16, Thomas Monjalon wrote:
>>> jeudi 3 août 2023, fengchengwen:
>>>> Hi Thomas,
>>>>
>>>> On 2023/7/31 20:48, Thomas Monjalon wrote:
>>>>> 10/07/2023 09:50, fengchengwen:
>>>>>> 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).
>>>>>
>>>>> Can you try with enable_trace_fp=false (the default)?
>>>>
>>>> It works well too.
>>>>
>>>>>
>>>>>>> Also, the test chkincs must run fine.
>>>>>>
>>>>>> chkincs ?
>>>>>
>>>>> If this a word you don't know, you can try "git grep" to better understand.
>>>>> There is a Meson option "check_includes" to enable chkincs.
>>>>>
>>>>> I recommend using devtools/test-meson-builds.sh to test patches,
>>>>> it includes the above options.
>>>>
>>>> According your suggest, I use test-meson-builds.sh, and pass.
>>>
>>> It does not pass for me:
>>>
>>> In file included from dmafwd.c:14:
>>> build-x86-generic/install/usr/local/include/rte_dmadev.h:799:10:
>>> fatal error: rte_dmadev_trace_fp.h: No such file or directory
>>>   799 | #include "rte_dmadev_trace_fp.h"
>>
>> I still can't reproduce the above error with .develconfig contain
>> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all"".
>>
>> Could you provide the config options (which load by devtools/load-devel-config) ?
>>
>>>
>>> Let me repeat again my recommendations:
>>> First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
>>> 	YOU NEED TO ADD IT in meson.build FILE
>>> Note: you could have caught this if testing the example app for DMA.
>>> Second, you must avoid structs and enum in this header file,
>>
>> Yes, I found compile error with .develconfig contain
>> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all enable_trace_fp=true""
>>
>> The root cause is that the structs (e.g. struct rte_dma_sge) and enum (e.g. enum rte_dma_status_code)
>> usage in dmadev fastpath API.
>>
>> I try to include "rte_dmadev.h" and it also not work (more compiler error).
>>
>> So I think two options:
>> 1. don't support fastpath tracepoints with dmadev library
>> 2. exclude xxx_trace_fp.h in buildtools/chkincs
>>
>> Would like to hear your opinion.
> 
> I've merged the patch for control path.
> Please send a new patch for data path, I will test it,
> and I will work with you to understand what happens.
> Let's target it for 24.03 release.

I send v1 to fix the chk_includes compile error by adding rte_dmadev_trace_fp.h in indirect_headers
(previous is headers). According the indirect_headers usage in [1], I think it's feasible.

BTW: If not solved by above, I think we need divide rte_dmadev.h into two part, first includes common
definition, and second includes data-path definition. and then rte_dmadev_trace_fp.h include the
common definition.

[1] doc/guides/contributing/coding_style.rst

Thanks

> 
> Thanks
> 
> 
> .
>
  

Patch

diff --git a/lib/dmadev/meson.build b/lib/dmadev/meson.build
index 2f17587b75..e0d90aea67 100644
--- a/lib/dmadev/meson.build
+++ b/lib/dmadev/meson.build
@@ -1,7 +1,7 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2021 HiSilicon Limited.
 
-sources = files('rte_dmadev.c')
+sources = files('rte_dmadev.c', 'rte_dmadev_trace_points.c')
 headers = files('rte_dmadev.h')
 indirect_headers += files('rte_dmadev_core.h')
 driver_sdk_headers += files('rte_dmadev_pmd.h')
diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 8c095e1f35..25fa78de8f 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -17,6 +17,7 @@ 
 
 #include "rte_dmadev.h"
 #include "rte_dmadev_pmd.h"
+#include "rte_dmadev_trace.h"
 
 static int16_t dma_devices_max;
 
@@ -434,6 +435,8 @@  rte_dma_info_get(int16_t dev_id, struct rte_dma_info *dev_info)
 	dev_info->numa_node = dev->device->numa_node;
 	dev_info->nb_vchans = dev->data->dev_conf.nb_vchans;
 
+	rte_dma_trace_info_get(dev_id, dev_info);
+
 	return 0;
 }
 
@@ -483,6 +486,8 @@  rte_dma_configure(int16_t dev_id, const struct rte_dma_conf *dev_conf)
 		memcpy(&dev->data->dev_conf, dev_conf,
 		       sizeof(struct rte_dma_conf));
 
+	rte_dma_trace_configure(dev_id, dev_conf, ret);
+
 	return ret;
 }
 
@@ -509,6 +514,7 @@  rte_dma_start(int16_t dev_id)
 		goto mark_started;
 
 	ret = (*dev->dev_ops->dev_start)(dev);
+	rte_dma_trace_start(dev_id, ret);
 	if (ret != 0)
 		return ret;
 
@@ -535,6 +541,7 @@  rte_dma_stop(int16_t dev_id)
 		goto mark_stopped;
 
 	ret = (*dev->dev_ops->dev_stop)(dev);
+	rte_dma_trace_stop(dev_id, ret);
 	if (ret != 0)
 		return ret;
 
@@ -565,6 +572,8 @@  rte_dma_close(int16_t dev_id)
 	if (ret == 0)
 		dma_release(dev);
 
+	rte_dma_trace_close(dev_id, ret);
+
 	return ret;
 }
 
@@ -655,14 +664,18 @@  rte_dma_vchan_setup(int16_t dev_id, uint16_t vchan,
 
 	if (*dev->dev_ops->vchan_setup == NULL)
 		return -ENOTSUP;
-	return (*dev->dev_ops->vchan_setup)(dev, vchan, conf,
+	ret = (*dev->dev_ops->vchan_setup)(dev, vchan, conf,
 					sizeof(struct rte_dma_vchan_conf));
+	rte_dma_trace_vchan_setup(dev_id, vchan, conf, ret);
+
+	return ret;
 }
 
 int
 rte_dma_stats_get(int16_t dev_id, uint16_t vchan, struct rte_dma_stats *stats)
 {
 	const struct rte_dma_dev *dev = &rte_dma_devices[dev_id];
+	int ret;
 
 	if (!rte_dma_is_valid(dev_id) || stats == NULL)
 		return -EINVAL;
@@ -677,14 +690,18 @@  rte_dma_stats_get(int16_t dev_id, uint16_t vchan, struct rte_dma_stats *stats)
 	if (*dev->dev_ops->stats_get == NULL)
 		return -ENOTSUP;
 	memset(stats, 0, sizeof(struct rte_dma_stats));
-	return (*dev->dev_ops->stats_get)(dev, vchan, stats,
-					  sizeof(struct rte_dma_stats));
+	ret = (*dev->dev_ops->stats_get)(dev, vchan, stats,
+					 sizeof(struct rte_dma_stats));
+	rte_dma_trace_stats_get(dev_id, vchan, stats, ret);
+
+	return ret;
 }
 
 int
 rte_dma_stats_reset(int16_t dev_id, uint16_t vchan)
 {
 	struct rte_dma_dev *dev = &rte_dma_devices[dev_id];
+	int ret;
 
 	if (!rte_dma_is_valid(dev_id))
 		return -EINVAL;
@@ -698,13 +715,17 @@  rte_dma_stats_reset(int16_t dev_id, uint16_t vchan)
 
 	if (*dev->dev_ops->stats_reset == NULL)
 		return -ENOTSUP;
-	return (*dev->dev_ops->stats_reset)(dev, vchan);
+	ret = (*dev->dev_ops->stats_reset)(dev, vchan);
+	rte_dma_trace_stats_reset(dev_id, vchan, ret);
+
+	return ret;
 }
 
 int
 rte_dma_vchan_status(int16_t dev_id, uint16_t vchan, enum rte_dma_vchan_status *status)
 {
 	struct rte_dma_dev *dev = &rte_dma_devices[dev_id];
+	int ret;
 
 	if (!rte_dma_is_valid(dev_id))
 		return -EINVAL;
@@ -716,7 +737,10 @@  rte_dma_vchan_status(int16_t dev_id, uint16_t vchan, enum rte_dma_vchan_status *
 
 	if (*dev->dev_ops->vchan_status == NULL)
 		return -ENOTSUP;
-	return (*dev->dev_ops->vchan_status)(dev, vchan, status);
+	ret = (*dev->dev_ops->vchan_status)(dev, vchan, status);
+	rte_dma_trace_vchan_status(dev_id, vchan, status, ret);
+
+	return ret;
 }
 
 static const char *
@@ -792,9 +816,10 @@  rte_dma_dump(int16_t dev_id, FILE *f)
 		dev->data->dev_conf.enable_silent ? "on" : "off");
 
 	if (dev->dev_ops->dev_dump != NULL)
-		return (*dev->dev_ops->dev_dump)(dev, f);
+		ret = (*dev->dev_ops->dev_dump)(dev, f);
+	rte_dma_trace_dump(dev_id, f, ret);
 
-	return 0;
+	return ret;
 }
 
 static int
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"
 
 /**@{@name DMA operation flag
  * @see rte_dma_copy()
@@ -856,6 +857,7 @@  rte_dma_copy(int16_t dev_id, uint16_t vchan, rte_iova_t src, rte_iova_t dst,
 	     uint32_t length, uint64_t flags)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
+	int ret;
 
 #ifdef RTE_DMADEV_DEBUG
 	if (!rte_dma_is_valid(dev_id) || length == 0)
@@ -864,7 +866,10 @@  rte_dma_copy(int16_t dev_id, uint16_t vchan, rte_iova_t src, rte_iova_t dst,
 		return -ENOTSUP;
 #endif
 
-	return (*obj->copy)(obj->dev_private, vchan, src, dst, length, flags);
+	ret = (*obj->copy)(obj->dev_private, vchan, src, dst, length, flags);
+	rte_dma_trace_copy(dev_id, vchan, src, dst, length, flags, ret);
+
+	return ret;
 }
 
 /**
@@ -907,6 +912,7 @@  rte_dma_copy_sg(int16_t dev_id, uint16_t vchan, struct rte_dma_sge *src,
 		uint64_t flags)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
+	int ret;
 
 #ifdef RTE_DMADEV_DEBUG
 	if (!rte_dma_is_valid(dev_id) || src == NULL || dst == NULL ||
@@ -916,8 +922,12 @@  rte_dma_copy_sg(int16_t dev_id, uint16_t vchan, struct rte_dma_sge *src,
 		return -ENOTSUP;
 #endif
 
-	return (*obj->copy_sg)(obj->dev_private, vchan, src, dst, nb_src,
-			       nb_dst, flags);
+	ret = (*obj->copy_sg)(obj->dev_private, vchan, src, dst, nb_src,
+			      nb_dst, flags);
+	rte_dma_trace_copy_sg(dev_id, vchan, src, dst, nb_src, nb_dst, flags,
+			      ret);
+
+	return ret;
 }
 
 /**
@@ -955,6 +965,7 @@  rte_dma_fill(int16_t dev_id, uint16_t vchan, uint64_t pattern,
 	     rte_iova_t dst, uint32_t length, uint64_t flags)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
+	int ret;
 
 #ifdef RTE_DMADEV_DEBUG
 	if (!rte_dma_is_valid(dev_id) || length == 0)
@@ -963,8 +974,11 @@  rte_dma_fill(int16_t dev_id, uint16_t vchan, uint64_t pattern,
 		return -ENOTSUP;
 #endif
 
-	return (*obj->fill)(obj->dev_private, vchan, pattern, dst, length,
-			    flags);
+	ret = (*obj->fill)(obj->dev_private, vchan, pattern, dst, length,
+			   flags);
+	rte_dma_trace_fill(dev_id, vchan, pattern, dst, length, flags, ret);
+
+	return ret;
 }
 
 /**
@@ -989,6 +1003,7 @@  static inline int
 rte_dma_submit(int16_t dev_id, uint16_t vchan)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
+	int ret;
 
 #ifdef RTE_DMADEV_DEBUG
 	if (!rte_dma_is_valid(dev_id))
@@ -997,7 +1012,10 @@  rte_dma_submit(int16_t dev_id, uint16_t vchan)
 		return -ENOTSUP;
 #endif
 
-	return (*obj->submit)(obj->dev_private, vchan);
+	ret = (*obj->submit)(obj->dev_private, vchan);
+	rte_dma_trace_submit(dev_id, vchan, ret);
+
+	return ret;
 }
 
 /**
@@ -1031,7 +1049,7 @@  rte_dma_completed(int16_t dev_id, uint16_t vchan, const uint16_t nb_cpls,
 		  uint16_t *last_idx, bool *has_error)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
-	uint16_t idx;
+	uint16_t idx, ret;
 	bool err;
 
 #ifdef RTE_DMADEV_DEBUG
@@ -1055,8 +1073,12 @@  rte_dma_completed(int16_t dev_id, uint16_t vchan, const uint16_t nb_cpls,
 		has_error = &err;
 
 	*has_error = false;
-	return (*obj->completed)(obj->dev_private, vchan, nb_cpls, last_idx,
-				 has_error);
+	ret = (*obj->completed)(obj->dev_private, vchan, nb_cpls, last_idx,
+				has_error);
+	rte_dma_trace_completed(dev_id, vchan, nb_cpls, last_idx, has_error,
+				ret);
+
+	return ret;
 }
 
 /**
@@ -1095,7 +1117,7 @@  rte_dma_completed_status(int16_t dev_id, uint16_t vchan,
 			 enum rte_dma_status_code *status)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
-	uint16_t idx;
+	uint16_t idx, ret;
 
 #ifdef RTE_DMADEV_DEBUG
 	if (!rte_dma_is_valid(dev_id) || nb_cpls == 0 || status == NULL)
@@ -1107,8 +1129,12 @@  rte_dma_completed_status(int16_t dev_id, uint16_t vchan,
 	if (last_idx == NULL)
 		last_idx = &idx;
 
-	return (*obj->completed_status)(obj->dev_private, vchan, nb_cpls,
-					last_idx, status);
+	ret = (*obj->completed_status)(obj->dev_private, vchan, nb_cpls,
+				       last_idx, status);
+	rte_dma_trace_completed_status(dev_id, vchan, nb_cpls, last_idx, status,
+				       ret);
+
+	return ret;
 }
 
 /**
@@ -1131,6 +1157,7 @@  static inline uint16_t
 rte_dma_burst_capacity(int16_t dev_id, uint16_t vchan)
 {
 	struct rte_dma_fp_object *obj = &rte_dma_fp_objs[dev_id];
+	uint16_t ret;
 
 #ifdef RTE_DMADEV_DEBUG
 	if (!rte_dma_is_valid(dev_id))
@@ -1138,7 +1165,10 @@  rte_dma_burst_capacity(int16_t dev_id, uint16_t vchan)
 	if (*obj->burst_capacity == NULL)
 		return 0;
 #endif
-	return (*obj->burst_capacity)(obj->dev_private, vchan);
+	ret = (*obj->burst_capacity)(obj->dev_private, vchan);
+	rte_dma_trace_burst_capacity(dev_id, vchan, ret);
+
+	return ret;
 }
 
 #ifdef __cplusplus
diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
new file mode 100644
index 0000000000..cbcddb3a70
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -0,0 +1,118 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 HiSilicon Limited
+ */
+
+#ifndef RTE_DMADEV_TRACE_H
+#define RTE_DMADEV_TRACE_H
+
+/**
+ * @file
+ *
+ * API for dmadev trace support.
+ */
+
+#include <rte_trace_point.h>
+
+#include "rte_dmadev.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+RTE_TRACE_POINT(
+	rte_dma_trace_info_get,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info *dev_info),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_string(dev_info->dev_name);
+	rte_trace_point_emit_u64(dev_info->dev_capa);
+	rte_trace_point_emit_u16(dev_info->max_vchans);
+	rte_trace_point_emit_u16(dev_info->max_desc);
+	rte_trace_point_emit_u16(dev_info->min_desc);
+	rte_trace_point_emit_u16(dev_info->max_sges);
+	rte_trace_point_emit_i16(dev_info->numa_node);
+	rte_trace_point_emit_u16(dev_info->nb_vchans);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_configure,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf *dev_conf,
+			     int ret),
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
+	const struct rte_dma_conf __dev_conf = {0};
+	dev_conf = &__dev_conf;
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+	int enable_silent = (int)dev_conf->enable_silent;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(dev_conf->nb_vchans);
+	rte_trace_point_emit_int(enable_silent);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_start,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_stop,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_close,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_vchan_setup,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     const struct rte_dma_vchan_conf *conf, int ret),
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
+	const struct rte_dma_vchan_conf __conf = {0};
+	conf = &__conf;
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+	int src_port_type = conf->src_port.port_type;
+	int dst_port_type = conf->dst_port.port_type;
+	int direction = conf->direction;
+	uint64_t src_pcie_cfg;
+	uint64_t dst_pcie_cfg;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_int(direction);
+	rte_trace_point_emit_u16(conf->nb_desc);
+	rte_trace_point_emit_int(src_port_type);
+	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_u64(src_pcie_cfg);
+	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_int(dst_port_type);
+	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_stats_reset,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT(
+	rte_dma_trace_dump,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, FILE *f, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_ptr(f);
+	rte_trace_point_emit_int(ret);
+)
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_DMADEV_TRACE_H */
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
new file mode 100644
index 0000000000..9bcdf7434e
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -0,0 +1,150 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 HiSilicon Limited
+ */
+
+#ifndef RTE_DMADEV_TRACE_FP_H
+#define RTE_DMADEV_TRACE_FP_H
+
+/**
+ * @file
+ *
+ * API for dmadev fastpath trace support
+ */
+
+#include <rte_trace_point.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_stats_get,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     struct rte_dma_stats *stats, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_u64(stats->submitted);
+	rte_trace_point_emit_u64(stats->completed);
+	rte_trace_point_emit_u64(stats->errors);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_vchan_status,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     enum rte_dma_vchan_status *status, int ret),
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
+	enum rte_dma_vchan_status __status = 0;
+	status = &__status;
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+	int vchan_status = *status;
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_int(vchan_status);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_copy,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan, rte_iova_t src,
+			     rte_iova_t dst, uint32_t length, uint64_t flags,
+			     int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_u64(src);
+	rte_trace_point_emit_u64(dst);
+	rte_trace_point_emit_u32(length);
+	rte_trace_point_emit_u64(flags);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_copy_sg,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     struct rte_dma_sge *src, struct rte_dma_sge *dst,
+			     uint16_t nb_src, uint16_t nb_dst, uint64_t flags,
+			     int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_ptr(src);
+	rte_trace_point_emit_ptr(dst);
+	rte_trace_point_emit_u16(nb_src);
+	rte_trace_point_emit_u16(nb_dst);
+	rte_trace_point_emit_u64(flags);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_fill,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan, uint64_t pattern,
+			     rte_iova_t dst, uint32_t length, uint64_t flags,
+			     int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_u64(pattern);
+	rte_trace_point_emit_u64(dst);
+	rte_trace_point_emit_u32(length);
+	rte_trace_point_emit_u64(flags);
+	rte_trace_point_emit_int(ret);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_submit,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan, int ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_int(ret);
+)
+
+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;
+	has_error = &__has_error;
+#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);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_completed_status,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
+			     const uint16_t nb_cpls, uint16_t *last_idx,
+			     enum rte_dma_status_code *status, uint16_t ret),
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
+	uint16_t __last_idx = 0;
+	last_idx = &__last_idx;
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+	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_ptr(status);
+	rte_trace_point_emit_u16(ret);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_dma_trace_burst_capacity,
+	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan, uint16_t ret),
+	rte_trace_point_emit_i16(dev_id);
+	rte_trace_point_emit_u16(vchan);
+	rte_trace_point_emit_u16(ret);
+)
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* RTE_DMADEV_TRACE_FP_H */
diff --git a/lib/dmadev/rte_dmadev_trace_points.c b/lib/dmadev/rte_dmadev_trace_points.c
new file mode 100644
index 0000000000..ddf60922bf
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace_points.c
@@ -0,0 +1,59 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2023 HiSilicon Limited
+ */
+
+#include <rte_trace_point_register.h>
+
+#include "rte_dmadev_trace.h"
+#include "rte_dmadev_trace_fp.h"
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_info_get,
+	lib.dmadev.info_get)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_configure,
+	lib.dmadev.configure)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_start,
+	lib.dmadev.start)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_stop,
+	lib.dmadev.stop)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_close,
+	lib.dmadev.close)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_vchan_setup,
+	lib.dmadev.vchan_setup)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_stats_get,
+	lib.dmadev.stats_get)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_stats_reset,
+	lib.dmadev.stats_reset)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_vchan_status,
+	lib.dmadev.vchan_status)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_dump,
+	lib.dmadev.dump)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_copy,
+	lib.dmadev.copy)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_copy_sg,
+	lib.dmadev.copy_sg)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_fill,
+	lib.dmadev.fill)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_submit,
+	lib.dmadev.submit)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_completed,
+	lib.dmadev.completed)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_completed_status,
+	lib.dmadev.completed_status)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_burst_capacity,
+	lib.dmadev.burst_capacity)
diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
index 7031d6b335..04db94bce5 100644
--- a/lib/dmadev/version.map
+++ b/lib/dmadev/version.map
@@ -1,6 +1,7 @@ 
 EXPERIMENTAL {
 	global:
 
+	# added in 21.11
 	rte_dma_close;
 	rte_dma_configure;
 	rte_dma_count_avail;
@@ -17,6 +18,15 @@  EXPERIMENTAL {
 	rte_dma_vchan_setup;
 	rte_dma_vchan_status;
 
+	# added in 23.07
+	__rte_dma_trace_burst_capacity;
+	__rte_dma_trace_completed;
+	__rte_dma_trace_completed_status;
+	__rte_dma_trace_copy;
+	__rte_dma_trace_copy_sg;
+	__rte_dma_trace_fill;
+	__rte_dma_trace_submit;
+
 	local: *;
 };