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 3D9DB432BC; Mon, 6 Nov 2023 21:59:24 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C1E10402BA; Mon, 6 Nov 2023 21:59:23 +0100 (CET) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id 2F750402B6 for ; Mon, 6 Nov 2023 21:59:22 +0100 (CET) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 848A35C00D1; Mon, 6 Nov 2023 15:59:21 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Mon, 06 Nov 2023 15:59:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to; s=fm3; t= 1699304361; x=1699390761; bh=9UdL80Y1nmHpDul4anSlBZ4xLgGybBJf2yD YguoK7wk=; b=qipFg9GWE1hbfnPBF4GopB0NiB0BHK4puOet7gK2z3jkqP7uyRg 9jo/RtRPLaI5ftKw0s9eUGdPVzVUsRZzD/0Bvnj+YTmpqGKjFkoZNrbdocCWpTX7 5M4fDpVJGdzb4ydRlXjHyCH5tanNx/tROTBgJ+T38vDqDFkSn4JKqPHP07gNzSos H4+/X7opXrVMbiW3clzHOwOV+4NVdHoRB+dJ11AZQOYA8YkGMuwVwZVOWKaEh3LU V8YN3CzyfXzxza7EM0qXeyPSoY4cl4DR2RrpvOXpHQD7051MrcgnjTOfwRfqHhcm 7F9rtqUntdepKRj7nYnkF+OGRvyZqFQSKXQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t= 1699304361; x=1699390761; bh=9UdL80Y1nmHpDul4anSlBZ4xLgGybBJf2yD YguoK7wk=; b=PeRinpkng/7FzR6QsaTZjNUEu1FYSdX88UtKtPX2bGejNHxeu8f pYZtpoHLGVAS55WI95wcHCM1Egf0uPO2MwgybDSUTHzEvaPgQREhpQU+i+jlnuIh WekCfJJ6G1V68KFkeSZA+xxxE+q8EDK8NnQm3zKOsXyfaDVeJ9Orgt7tjLuQ0BjW 4nLyYuak+leLUDimtPJsWBsD3rODKqGrnl66PaAPUKbuB15NBDSlZUFDrpjpjKMR FtW5falz4LAqmCn6o/cD/EhzN/kJjjR+QY07HS5xg7pvQkSCB2IjKY4pAqQxPZsN PHRTzkA7/sGk7BlX6oWzpokVS8yXq85sK+Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedruddugedgudegudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvfevufffkfgjfhgggfgtsehtqhertddttddunecuhfhrohhmpefvhhho mhgrshcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqne cuggftrfgrthhtvghrnhepfefhjeeluedvvedtuddtuedtvefhieejtefhffeujefhtedu udevtdektdeikeffnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilh hfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 6 Nov 2023 15:59:20 -0500 (EST) From: Thomas Monjalon To: fengchengwen Cc: dev@dpdk.org, mb@smartsharesystems.com, Kevin Laatz , Bruce Richardson , david.marchand@redhat.com Subject: Re: [PATCH v4] dmadev: add tracepoints Date: Mon, 06 Nov 2023 21:59:18 +0100 Message-ID: <2896474.SvYEEZNnvj@thomas> In-Reply-To: <7286e948-5ada-52ff-3b7d-aaf6888238f4@huawei.com> References: <20230412024808.41339-1-fengchengwen@huawei.com> <2075619.usQuhbGJ8B@thomas> <7286e948-5ada-52ff-3b7d-aaf6888238f4@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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 11/10/2023 11:55, fengchengwen: > Hi Thomas, >=20 > Sorry for the late reply. >=20 > On 2023/8/14 22:16, Thomas Monjalon wrote: > > jeudi 3 ao=FBt 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=F8rup > >>>>>>>> > >>>>>>>> --- > >>>>>>>> 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 D= MA. > >>>>>>> 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 n= b_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 ther= e 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 =3D *has_error; // pointer reference > >>>>>> int last_idx_val =3D *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 und= efined. > >>>>>> > >>>>>> > >>>>>> 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 =3D 0; > >>>>>> bool __has_error =3D false; > >>>>>> last_idx =3D &__last_idx; // make sure the point= er has meaningful value. > >>>>>> has_error =3D &__has_error; // so that the next po= inter reference will work well. > >>>>>> #endif /* _RTE_TRACE_POINT_REGISTER_H_ */ > >>>>>> int has_error_val =3D *has_error; > >>>>>> int last_idx_val =3D *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=3Dtrue. > >>>>>> > >>>>>> ./dpdk-test --file-prefix=3Dfeng123 --trace=3Dlib.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=3Dtrue). > >>> > >>> Can you try with enable_trace_fp=3Dfalse (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 under= stand. > >>> 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. > >=20 > > It does not pass for me: > >=20 > > 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" >=20 > I still can't reproduce the above error with .develconfig contain > "DPDK_MESON_OPTIONS=3D"max_numa_nodes=3D1 disable_drivers=3Devent/cnxk ex= amples=3Dall"". >=20 > Could you provide the config options (which load by devtools/load-devel-c= onfig) ? >=20 > >=20 > > Let me repeat again my recommendations: > > First you must export rte_dmadev_trace_fp.h as it is included by rte_dm= adev.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, >=20 > Yes, I found compile error with .develconfig contain > "DPDK_MESON_OPTIONS=3D"max_numa_nodes=3D1 disable_drivers=3Devent/cnxk ex= amples=3Dall enable_trace_fp=3Dtrue"" >=20 > 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. >=20 > I try to include "rte_dmadev.h" and it also not work (more compiler error= ). >=20 > So I think two options: > 1. don't support fastpath tracepoints with dmadev library > 2. exclude xxx_trace_fp.h in buildtools/chkincs >=20 > 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