From: Thomas Monjalon <thomas@monjalon.net>
To: fengchengwen <fengchengwen@huawei.com>
Cc: dev@dpdk.org, mb@smartsharesystems.com,
Kevin Laatz <kevin.laatz@intel.com>,
Bruce Richardson <bruce.richardson@intel.com>,
david.marchand@redhat.com
Subject: Re: [PATCH v4] dmadev: add tracepoints
Date: Mon, 06 Nov 2023 21:59:18 +0100 [thread overview]
Message-ID: <2896474.SvYEEZNnvj@thomas> (raw)
In-Reply-To: <7286e948-5ada-52ff-3b7d-aaf6888238f4@huawei.com>
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
next prev parent reply other threads:[~2023-11-06 20:59 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-12 2:48 [PATCH] " Chengwen Feng
2023-04-12 9:52 ` Bruce Richardson
2023-04-13 3:44 ` fengchengwen
2023-04-13 8:45 ` Bruce Richardson
2023-04-12 11:00 ` Morten Brørup
2023-04-13 6:30 ` fengchengwen
2023-04-13 8:25 ` Morten Brørup
2023-04-15 0:33 ` [PATCH v3] " Chengwen Feng
2023-05-24 21:12 ` Thomas Monjalon
2023-05-27 0:17 ` fengchengwen
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
2023-07-10 6:49 ` Thomas Monjalon
2023-07-10 7:50 ` fengchengwen
2023-07-31 12:48 ` Thomas Monjalon
2023-08-03 7:52 ` fengchengwen
2023-08-14 14:16 ` Thomas Monjalon
2023-10-11 9:55 ` fengchengwen
2023-11-06 20:59 ` Thomas Monjalon [this message]
2023-11-07 1:26 ` fengchengwen
2024-01-12 10:38 ` fengchengwen
2023-10-20 2:21 ` [PATCH] dmadev: add tracepoints at control path APIs Chengwen Feng
2023-11-06 20:55 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2896474.SvYEEZNnvj@thomas \
--to=thomas@monjalon.net \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=kevin.laatz@intel.com \
--cc=mb@smartsharesystems.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).