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)?
> > 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.
