* Re: [PATCH] dmadev: add tracepoints
2023-04-12 2:48 [PATCH] dmadev: add tracepoints Chengwen Feng
@ 2023-04-12 9:52 ` Bruce Richardson
2023-04-13 3:44 ` fengchengwen
2023-04-12 11:00 ` Morten Brørup
` (3 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2023-04-12 9:52 UTC (permalink / raw)
To: Chengwen Feng; +Cc: thomas, Kevin Laatz, dev
On Wed, Apr 12, 2023 at 02:48:08AM +0000, Chengwen Feng wrote:
> Add tracepoints at important APIs for tracing support.
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
> lib/dmadev/meson.build | 2 +-
> lib/dmadev/rte_dmadev.c | 39 ++++++--
> lib/dmadev/rte_dmadev.h | 56 ++++++++---
> lib/dmadev/rte_dmadev_trace.h | 133 +++++++++++++++++++++++++++
> lib/dmadev/rte_dmadev_trace_fp.h | 113 +++++++++++++++++++++++
> lib/dmadev/rte_dmadev_trace_points.c | 59 ++++++++++++
> lib/dmadev/version.map | 10 ++
> 7 files changed, 391 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
>
For completeness, do you have any numbers for the performance impact (if
any) to the DMA dataplane APIs with this tracing added?
/Bruce
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] dmadev: add tracepoints
2023-04-12 9:52 ` Bruce Richardson
@ 2023-04-13 3:44 ` fengchengwen
2023-04-13 8:45 ` Bruce Richardson
0 siblings, 1 reply; 25+ messages in thread
From: fengchengwen @ 2023-04-13 3:44 UTC (permalink / raw)
To: Bruce Richardson; +Cc: thomas, Kevin Laatz, dev
On 2023/4/12 17:52, Bruce Richardson wrote:
> On Wed, Apr 12, 2023 at 02:48:08AM +0000, Chengwen Feng wrote:
>> Add tracepoints at important APIs for tracing support.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>> lib/dmadev/meson.build | 2 +-
>> lib/dmadev/rte_dmadev.c | 39 ++++++--
>> lib/dmadev/rte_dmadev.h | 56 ++++++++---
>> lib/dmadev/rte_dmadev_trace.h | 133 +++++++++++++++++++++++++++
>> lib/dmadev/rte_dmadev_trace_fp.h | 113 +++++++++++++++++++++++
>> lib/dmadev/rte_dmadev_trace_points.c | 59 ++++++++++++
>> lib/dmadev/version.map | 10 ++
>> 7 files changed, 391 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
>>
> For completeness, do you have any numbers for the performance impact (if
> any) to the DMA dataplane APIs with this tracing added?
No, because:
The dataplane trace points was disable default (unless the RTE_ENABLE_TRACE_FP is set),
so there will no trace-points code default.
So I think it will not impact performance default.
>
> /Bruce
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] dmadev: add tracepoints
2023-04-13 3:44 ` fengchengwen
@ 2023-04-13 8:45 ` Bruce Richardson
0 siblings, 0 replies; 25+ messages in thread
From: Bruce Richardson @ 2023-04-13 8:45 UTC (permalink / raw)
To: fengchengwen; +Cc: thomas, Kevin Laatz, dev
On Thu, Apr 13, 2023 at 11:44:38AM +0800, fengchengwen wrote:
> On 2023/4/12 17:52, Bruce Richardson wrote:
> > On Wed, Apr 12, 2023 at 02:48:08AM +0000, Chengwen Feng wrote:
> >> Add tracepoints at important APIs for tracing support.
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> ---
> >> lib/dmadev/meson.build | 2 +-
> >> lib/dmadev/rte_dmadev.c | 39 ++++++--
> >> lib/dmadev/rte_dmadev.h | 56 ++++++++---
> >> lib/dmadev/rte_dmadev_trace.h | 133 +++++++++++++++++++++++++++
> >> lib/dmadev/rte_dmadev_trace_fp.h | 113 +++++++++++++++++++++++
> >> lib/dmadev/rte_dmadev_trace_points.c | 59 ++++++++++++
> >> lib/dmadev/version.map | 10 ++
> >> 7 files changed, 391 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
> >>
> > For completeness, do you have any numbers for the performance impact (if
> > any) to the DMA dataplane APIs with this tracing added?
>
> No, because:
>
> The dataplane trace points was disable default (unless the RTE_ENABLE_TRACE_FP is set),
> so there will no trace-points code default.
>
> So I think it will not impact performance default.
>
Right, I'd missed that bit. Thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH] dmadev: add tracepoints
2023-04-12 2:48 [PATCH] dmadev: add tracepoints Chengwen Feng
2023-04-12 9:52 ` Bruce Richardson
@ 2023-04-12 11:00 ` Morten Brørup
2023-04-13 6:30 ` fengchengwen
2023-04-15 0:33 ` [PATCH v3] " Chengwen Feng
` (2 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2023-04-12 11:00 UTC (permalink / raw)
To: Chengwen Feng, Kevin Laatz, Bruce Richardson; +Cc: dev, thomas
> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Wednesday, 12 April 2023 04.48
>
> Add tracepoints at important APIs for tracing support.
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
[...]
> diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
> new file mode 100644
> index 0000000000..0dae78ca15
> --- /dev/null
> +++ b/lib/dmadev/rte_dmadev_trace.h
> @@ -0,0 +1,133 @@
> +/* 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),
> + 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),
> + 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_get,
This should be a fast path trace point.
For reference, ethdev considers rte_eth_stats_get() a fast path function.
> + 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(
> + 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_vchan_status,
This should be a fast path trace point.
For reference, ethdev considers rte_eth_link_get() a fast path function.
> + RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
> + enum rte_dma_vchan_status *status, int ret),
> + 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(
> + 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..f5ebad1bc4
> --- /dev/null
> +++ b/lib/dmadev/rte_dmadev_trace_fp.h
> @@ -0,0 +1,113 @@
> +/* 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_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),
> + 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),
> + 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..4ee1b3f74a 100644
> --- a/lib/dmadev/version.map
> +++ b/lib/dmadev/version.map
> @@ -1,6 +1,16 @@
> EXPERIMENTAL {
> global:
>
> + # 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;
> +
Intuitively, I would suppose that the 23.07 functions should be listed after the 21.11 functions, not before.
> + # added in 21.11
Good catch.
> rte_dma_close;
> rte_dma_configure;
> rte_dma_count_avail;
> --
> 2.17.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] dmadev: add tracepoints
2023-04-12 11:00 ` Morten Brørup
@ 2023-04-13 6:30 ` fengchengwen
2023-04-13 8:25 ` Morten Brørup
0 siblings, 1 reply; 25+ messages in thread
From: fengchengwen @ 2023-04-13 6:30 UTC (permalink / raw)
To: Morten Brørup, Kevin Laatz, Bruce Richardson; +Cc: dev, thomas
On 2023/4/12 19:00, Morten Brørup wrote:
>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
>> Sent: Wednesday, 12 April 2023 04.48
>>
>> Add tracepoints at important APIs for tracing support.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>
...
>> +)
>> +
>> +RTE_TRACE_POINT(
>> + rte_dma_trace_stats_get,
>
> This should be a fast path trace point.
> For reference, ethdev considers rte_eth_stats_get() a fast path function.
Emm, I think it should discuss more, and make it clear.
The cryptodev and dmadev trace-points both make 'rte_xxx_trace_stats_get' as a slow path function.
And it mainly refer to the fast path API (means if a API is fast path then make it as a fast-path trace-points).
But the ethdev trace-points treats 'calls in loop function(such as rte_eth_trace_stats_get/rte_eth_trace_link_get/...)'
as fast path trace-points.
>
>> + 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);
>> +)
>> +
...
>> diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
>> index 7031d6b335..4ee1b3f74a 100644
>> --- a/lib/dmadev/version.map
>> +++ b/lib/dmadev/version.map
>> @@ -1,6 +1,16 @@
>> EXPERIMENTAL {
>> global:
>>
>> + # 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;
>> +
>
> Intuitively, I would suppose that the 23.07 functions should be listed after the 21.11 functions, not before.
+1, will fix in v2
>
>> + # added in 21.11
>
> Good catch.
>
>> rte_dma_close;
>> rte_dma_configure;
>> rte_dma_count_avail;
>> --
>> 2.17.1
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH] dmadev: add tracepoints
2023-04-13 6:30 ` fengchengwen
@ 2023-04-13 8:25 ` Morten Brørup
0 siblings, 0 replies; 25+ messages in thread
From: Morten Brørup @ 2023-04-13 8:25 UTC (permalink / raw)
To: fengchengwen, Kevin Laatz, Bruce Richardson; +Cc: dev, thomas
> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Thursday, 13 April 2023 08.30
>
> On 2023/4/12 19:00, Morten Brørup wrote:
> >> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> >> Sent: Wednesday, 12 April 2023 04.48
> >>
> >> Add tracepoints at important APIs for tracing support.
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> ---
> >
>
> ...
>
> >> +)
> >> +
> >> +RTE_TRACE_POINT(
> >> + rte_dma_trace_stats_get,
> >
> > This should be a fast path trace point.
> > For reference, ethdev considers rte_eth_stats_get() a fast path function.
>
> Emm, I think it should discuss more, and make it clear.
>
> The cryptodev and dmadev trace-points both make 'rte_xxx_trace_stats_get' as a
> slow path function.
Then those stats tracepoints should be fixed, so they behave like ethdev.
> And it mainly refer to the fast path API (means if a API is fast path then
> make it as a fast-path trace-points).
Dataplane functions must obviously be RTE_TRACE_POINT_FP. But some non-dataplane functions must also be RTE_TRACE_POINT_FP.
>
> But the ethdev trace-points treats 'calls in loop function(such as
> rte_eth_trace_stats_get/rte_eth_trace_link_get/...)'
> as fast path trace-points.
Yes. Such "grey area" functions must use FP trace. Please refer to the Techboard decision to treat them as FP trace [1].
[1]: http://inbox.dpdk.org/dev/2325762.trqCLbgVIZ@thomas/
The techboard made this decision when trace was introduced to ethdev, and it seems that it was not caught in review when trace was added to cryptodev.
PS: The trace point macro names are unfortunate... Since RTE_TRACE_POINT_FP is not only for Fast Path functions, they should have been named RTE_TRACE_POINT and RTE_TRACE_POINT_SLOW instead of RTE_TRACE_POINT_FP and RTE_TRACE_POINT. But it is too late to change now.
>
> >
> >> + 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);
> >> +)
> >> +
>
> ...
>
> >> diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
> >> index 7031d6b335..4ee1b3f74a 100644
> >> --- a/lib/dmadev/version.map
> >> +++ b/lib/dmadev/version.map
> >> @@ -1,6 +1,16 @@
> >> EXPERIMENTAL {
> >> global:
> >>
> >> + # 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;
> >> +
> >
> > Intuitively, I would suppose that the 23.07 functions should be listed after
> the 21.11 functions, not before.
>
> +1, will fix in v2
>
> >
> >> + # added in 21.11
> >
> > Good catch.
> >
> >> rte_dma_close;
> >> rte_dma_configure;
> >> rte_dma_count_avail;
> >> --
> >> 2.17.1
> >
> >
> > .
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3] dmadev: add tracepoints
2023-04-12 2:48 [PATCH] dmadev: add tracepoints Chengwen Feng
2023-04-12 9:52 ` Bruce Richardson
2023-04-12 11:00 ` Morten Brørup
@ 2023-04-15 0:33 ` Chengwen Feng
2023-05-24 21:12 ` Thomas Monjalon
2023-05-26 8:42 ` [PATCH v4] " Chengwen Feng
2023-10-20 2:21 ` [PATCH] dmadev: add tracepoints at control path APIs Chengwen Feng
4 siblings, 1 reply; 25+ messages in thread
From: Chengwen Feng @ 2023-04-15 0:33 UTC (permalink / raw)
To: thomas, mb, Kevin Laatz, Bruce Richardson; +Cc: dev
Add tracepoints at important APIs for tracing support.
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
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 | 110 ++++++++++++++++++++++
lib/dmadev/rte_dmadev_trace_fp.h | 136 +++++++++++++++++++++++++++
lib/dmadev/rte_dmadev_trace_points.c | 59 ++++++++++++
lib/dmadev/version.map | 10 ++
7 files changed, 391 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
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..c1d2f6f893
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -0,0 +1,110 @@
+/* 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),
+ 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),
+ 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..03eb9bd6dd
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -0,0 +1,136 @@
+/* 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),
+ 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),
+ 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),
+ 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: *;
};
--
2.17.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3] dmadev: add tracepoints
2023-04-15 0:33 ` [PATCH v3] " Chengwen Feng
@ 2023-05-24 21:12 ` Thomas Monjalon
2023-05-27 0:17 ` fengchengwen
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2023-05-24 21:12 UTC (permalink / raw)
To: Chengwen Feng; +Cc: mb, Kevin Laatz, Bruce Richardson, dev
15/04/2023 02:33, 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>
This patch triggers a failure in Intel CI:
asan_smoke | test_rxtx_with_ASan_enable| FAILED
If you think it is a false positive,
please could you submit it again as v4?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3] dmadev: add tracepoints
2023-05-24 21:12 ` Thomas Monjalon
@ 2023-05-27 0:17 ` fengchengwen
0 siblings, 0 replies; 25+ messages in thread
From: fengchengwen @ 2023-05-27 0:17 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: mb, Kevin Laatz, Bruce Richardson, dev
Hi Thomas,
On 2023/5/25 5:12, Thomas Monjalon wrote:
> 15/04/2023 02:33, 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>
>
> This patch triggers a failure in Intel CI:
> asan_smoke | test_rxtx_with_ASan_enable| FAILED
>
> If you think it is a false positive,
> please could you submit it again as v4?
V4 already sent, and I observed the V4 fix it in patchwork.
Thanks
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4] dmadev: add tracepoints
2023-04-12 2:48 [PATCH] dmadev: add tracepoints Chengwen Feng
` (2 preceding siblings ...)
2023-04-15 0:33 ` [PATCH v3] " Chengwen Feng
@ 2023-05-26 8:42 ` Chengwen Feng
2023-07-03 3:54 ` fengchengwen
2023-07-07 10:40 ` Thomas Monjalon
2023-10-20 2:21 ` [PATCH] dmadev: add tracepoints at control path APIs Chengwen Feng
4 siblings, 2 replies; 25+ messages in thread
From: Chengwen Feng @ 2023-05-26 8:42 UTC (permalink / raw)
To: thomas, mb, Kevin Laatz, Bruce Richardson; +Cc: dev
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
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: *;
};
--
2.17.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-05-26 8:42 ` [PATCH v4] " Chengwen Feng
@ 2023-07-03 3:54 ` fengchengwen
2023-07-07 10:40 ` Thomas Monjalon
1 sibling, 0 replies; 25+ messages in thread
From: fengchengwen @ 2023-07-03 3:54 UTC (permalink / raw)
To: thomas, mb, Kevin Laatz, Bruce Richardson; +Cc: dev
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.
>
...
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4] dmadev: add tracepoints
2023-05-26 8:42 ` [PATCH v4] " Chengwen Feng
2023-07-03 3:54 ` fengchengwen
@ 2023-07-07 10:40 ` Thomas Monjalon
2023-07-09 3:23 ` fengchengwen
1 sibling, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2023-07-07 10:40 UTC (permalink / raw)
To: Chengwen Feng; +Cc: mb, Kevin Laatz, Bruce Richardson, dev, david.marchand
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4] dmadev: add tracepoints
2023-07-07 10:40 ` Thomas Monjalon
@ 2023-07-09 3:23 ` fengchengwen
2023-07-10 6:49 ` Thomas Monjalon
0 siblings, 1 reply; 25+ messages in thread
From: fengchengwen @ 2023-07-09 3:23 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: mb, Kevin Laatz, Bruce Richardson, dev, david.marchand
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-07-09 3:23 ` fengchengwen
@ 2023-07-10 6:49 ` Thomas Monjalon
2023-07-10 7:50 ` fengchengwen
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2023-07-10 6:49 UTC (permalink / raw)
To: fengchengwen; +Cc: mb, Kevin Laatz, Bruce Richardson, dev, david.marchand
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.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-07-10 6:49 ` Thomas Monjalon
@ 2023-07-10 7:50 ` fengchengwen
2023-07-31 12:48 ` Thomas Monjalon
0 siblings, 1 reply; 25+ messages in thread
From: fengchengwen @ 2023-07-10 7:50 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: mb, Kevin Laatz, Bruce Richardson, dev, david.marchand
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 ?
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-07-10 7:50 ` fengchengwen
@ 2023-07-31 12:48 ` Thomas Monjalon
2023-08-03 7:52 ` fengchengwen
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2023-07-31 12:48 UTC (permalink / raw)
To: fengchengwen; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, dev, david.marchand
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.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-07-31 12:48 ` Thomas Monjalon
@ 2023-08-03 7:52 ` fengchengwen
2023-08-14 14:16 ` Thomas Monjalon
0 siblings, 1 reply; 25+ messages in thread
From: fengchengwen @ 2023-08-03 7:52 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, david.marchand
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]')
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-08-03 7:52 ` fengchengwen
@ 2023-08-14 14:16 ` Thomas Monjalon
2023-10-11 9:55 ` fengchengwen
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2023-08-14 14:16 UTC (permalink / raw)
To: fengchengwen; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, david.marchand
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.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-08-14 14:16 ` Thomas Monjalon
@ 2023-10-11 9:55 ` fengchengwen
2023-11-06 20:59 ` Thomas Monjalon
0 siblings, 1 reply; 25+ messages in thread
From: fengchengwen @ 2023-10-11 9:55 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, david.marchand
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.
>
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-10-11 9:55 ` fengchengwen
@ 2023-11-06 20:59 ` Thomas Monjalon
2023-11-07 1:26 ` fengchengwen
2024-01-12 10:38 ` fengchengwen
0 siblings, 2 replies; 25+ messages in thread
From: Thomas Monjalon @ 2023-11-06 20:59 UTC (permalink / raw)
To: fengchengwen; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, david.marchand
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-11-06 20:59 ` Thomas Monjalon
@ 2023-11-07 1:26 ` fengchengwen
2024-01-12 10:38 ` fengchengwen
1 sibling, 0 replies; 25+ messages in thread
From: fengchengwen @ 2023-11-07 1:26 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, david.marchand
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
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v4] dmadev: add tracepoints
2023-11-06 20:59 ` Thomas Monjalon
2023-11-07 1:26 ` fengchengwen
@ 2024-01-12 10:38 ` fengchengwen
1 sibling, 0 replies; 25+ messages in thread
From: fengchengwen @ 2024-01-12 10:38 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, mb, Kevin Laatz, Bruce Richardson, david.marchand
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
>
>
> .
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] dmadev: add tracepoints at control path APIs
2023-04-12 2:48 [PATCH] dmadev: add tracepoints Chengwen Feng
` (3 preceding siblings ...)
2023-05-26 8:42 ` [PATCH v4] " Chengwen Feng
@ 2023-10-20 2:21 ` Chengwen Feng
2023-11-06 20:55 ` Thomas Monjalon
4 siblings, 1 reply; 25+ messages in thread
From: Chengwen Feng @ 2023-10-20 2:21 UTC (permalink / raw)
To: thomas, mb, david.marchand, Kevin Laatz, Bruce Richardson; +Cc: dev
Add tracepoints at control path APIs for tracing support.
Note: Fast path APIs don't support tracepoints because the APIs contains
struct and enum, if adding tracepints will lead to chkincs failure.
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
v5: Only support control path APIs tracepoints.
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 | 25 +++++-
lib/dmadev/rte_dmadev_trace.h | 123 +++++++++++++++++++++++++++
lib/dmadev/rte_dmadev_trace_points.c | 31 +++++++
4 files changed, 176 insertions(+), 5 deletions(-)
create mode 100644 lib/dmadev/rte_dmadev_trace.h
create mode 100644 lib/dmadev/rte_dmadev_trace_points.c
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 bf7d5ec519..4e5e420c82 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,8 +664,11 @@ 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
@@ -685,6 +697,7 @@ 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,7 +711,10 @@ 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
@@ -792,9 +808,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_trace.h b/lib/dmadev/rte_dmadev_trace.h
new file mode 100644
index 0000000000..e55c4c6091
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -0,0 +1,123 @@
+/* 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),
+#ifdef _RTE_TRACE_POINT_REGISTER_H_
+ struct rte_dma_info __dev_info = {0};
+ dev_info = &__dev_info;
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+ 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_ptr(conf->auto_free.m2d.pool);
+ 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_points.c b/lib/dmadev/rte_dmadev_trace_points.c
new file mode 100644
index 0000000000..2a83b90cb3
--- /dev/null
+++ b/lib/dmadev/rte_dmadev_trace_points.c
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2023 HiSilicon Limited
+ */
+
+#include <rte_trace_point_register.h>
+
+#include "rte_dmadev_trace.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_reset,
+ lib.dmadev.stats_reset)
+
+RTE_TRACE_POINT_REGISTER(rte_dma_trace_dump,
+ lib.dmadev.dump)
--
2.17.1
^ permalink raw reply [flat|nested] 25+ messages in thread