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.
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]')
>
>
> .
>
