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 <fengcheng...@huawei.com>
>>>>>>>> Acked-by: Morten Brørup <m...@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.
>
>
>
> .
>
