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 D5B3F4235E; Wed, 11 Oct 2023 11:55:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C340A40279; Wed, 11 Oct 2023 11:55:31 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id C887E40266 for ; Wed, 11 Oct 2023 11:55:29 +0200 (CEST) Received: from dggpeml100024.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4S57MQ720kzLpf2; Wed, 11 Oct 2023 17:51:30 +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; Wed, 11 Oct 2023 17:55:26 +0800 Subject: Re: [PATCH v4] dmadev: add tracepoints To: Thomas Monjalon CC: , , Kevin Laatz , Bruce Richardson , References: <20230412024808.41339-1-fengchengwen@huawei.com> <1737546.vCJZsxu672@thomas> <2075619.usQuhbGJ8B@thomas> From: fengchengwen Message-ID: <7286e948-5ada-52ff-3b7d-aaf6888238f4@huawei.com> Date: Wed, 11 Oct 2023 17:55:26 +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: <2075619.usQuhbGJ8B@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: dggems706-chm.china.huawei.com (10.3.19.183) 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, 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. > 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. > > > > . >