From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E2537432BF; Tue, 7 Nov 2023 02:27:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ABF2D402B5; Tue, 7 Nov 2023 02:27:44 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id CC294402A1 for ; Tue, 7 Nov 2023 02:27:42 +0100 (CET) Received: from dggpeml100024.china.huawei.com (unknown [172.30.72.55]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4SPVpT4vt3zMmbW; Tue, 7 Nov 2023 09:23:13 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by dggpeml100024.china.huawei.com (7.185.36.115) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Tue, 7 Nov 2023 09:26:38 +0800 Subject: Re: [PATCH v4] dmadev: add tracepoints To: Thomas Monjalon CC: , , Kevin Laatz , Bruce Richardson , References: <20230412024808.41339-1-fengchengwen@huawei.com> <2075619.usQuhbGJ8B@thomas> <7286e948-5ada-52ff-3b7d-aaf6888238f4@huawei.com> <2896474.SvYEEZNnvj@thomas> From: fengchengwen Message-ID: <36c44f2f-fb5d-e23b-6289-c0a908aede43@huawei.com> Date: Tue, 7 Nov 2023 09:26:38 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <2896474.SvYEEZNnvj@thomas> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggpeml100024.china.huawei.com (7.185.36.115) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >>>>>>>>>> Acked-by: Morten Brørup >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> 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 > > > . >