Re: [PATCH v4] dmadev: add tracepoints

[Thomas Monjalon <thomas@monjalon.net>]

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.