From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 36FE5A0588; Thu, 16 Apr 2020 18:23:11 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 56E071DD2C; Thu, 16 Apr 2020 18:23:10 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 32A371DD27 for ; Thu, 16 Apr 2020 18:23:08 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailout.nyi.internal (Postfix) with ESMTP id A51C45C004A; Thu, 16 Apr 2020 12:23:07 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute7.internal (MEProxy); Thu, 16 Apr 2020 12:23:07 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=faRdzXISnzrVM5tpIqFvQ8YN8BWP5aNw7Ze49Ta/+dg=; b=bmwuyV+1dQjq SGJOyOUPFCpd9nHTA8JYziEAYN1vSvc7dTIgEXMkltD495DFhOOeMy4k+cR1oe9b nU5PEqIfPkmsP2Q384+Be6VOX+fUQrJxeMyioClnkcFSrvExCpP8respovoCYpqV 10SnmN8kQti1J09rtHbEAjhkomVaymI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=faRdzXISnzrVM5tpIqFvQ8YN8BWP5aNw7Ze49Ta/+ dg=; b=jPTa6ELbhxS0Fg+X2B0ZDjrMV2Md1IBkLNVctLNmhv8Qnb8BJOdH6vPP1 bOOq+Tvp/pJqoArKf8GvuxEYfrMy5LWkg1JnmZd1+Ul5Sm+tJhfa4m8DJZiQwqFG IQKtoxoHppsZxZkYQTzisYzYengyTcphGk0WsVCjYMWPrNPetKiVtKXIhQDVw6yN t6j+rLNJVpZtcw3PSFBcT09TjB2Cfn1fAuhkrnaq/ZDZgMKTeKslY9rJEML+ZGAO uGpdDjTsiJMHiUn4JS4yhAS97Wf8zmKokXdkyS/ojVJn+c1+DxT5R+mNhZ61TpSP joZLv2/D6eRBP1iiFJjGvPulZ0VMw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrfeehgddutdduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecukf hppeejjedrudefgedrvddtfedrudekgeenucevlhhushhtvghrufhiiigvpedtnecurfgr rhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id DC28B306006C; Thu, 16 Apr 2020 12:23:05 -0400 (EDT) From: Thomas Monjalon To: David Marchand , Jerin Jacob Cc: Jerin Jacob Kollanukkaran , dev , Bruce Richardson , Mattias =?ISO-8859-1?Q?R=F6nnblom?= , Sunil Kumar Kori Date: Thu, 16 Apr 2020 18:23:04 +0200 Message-ID: <3598012.22IY78Rhhi@thomas> In-Reply-To: References: <20200403153709.3703448-1-jerinj@marvell.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v5 00/33] DPDK Trace support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 16/04/2020 18:08, Jerin Jacob: > On Thu, Apr 16, 2020 at 7:09 PM David Marchand > wrote: > > > > On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob wrote: > > > > - What do you think of splitting the API in two headers, thinking > > > > about who will use them? > > > > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > > > > users of the trace framework that want to > > > > * get the status of the whole trace subsystem, > > > > * enable/disable tracepoints by pattern/regexp, > > > > * dump the current events, > > > > * rte_tracepoint.h (rte_tracepoint_ prefix for all > > > > functions/macros/types) for developers that want to add tracepoints to > > > > their code > > > > > > # Initially, I thought of doing the same. > > > Later realized that some of the definitions such as following > > > > > > 1) > > > /** The trace object. The trace APIs are based on this opaque object. */ > > > typedef uint64_t rte_trace_t; > > > > As a user, I would ask the trace framework to enable tracepoints by > > calling rte_trace_pattern()/rte_trace_regexp(). > > This does not require the tracepoint descriptor to be exposed in rte_trace.h. > > > > > > If some application wants to store/manipulate the descriptors, then it > > will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object > > and API are: > > - rte_tracepoint_lookup (currently named rte_trace_by_name) > > - rte_tracepoint_enable > > - rte_tracepoint_disable > > - rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid, > > should be private, as discussed) > > - rte_tracepoint_is_enabled > > - RTE_TRACEPOINT/_FP macros > > - rte_tracepoint_register etc... > > From the prototype onwards, Myself shuffled abound multiple times on > the API name to satisfying > names. > > If you would like to classify based on the tracepoint object > dependency to a new header file, it is fine. > Let's go the last round for API naming details. > > I think, trace being the domain, IMO, it better to call the trace > point API with rte_trace_point_* > and trace point object to rte_trace_point_t (vs rte_tracepoint_t) > > I will summarise the public API and file name details. Let's finalize. > > # rte_trace.h will have > > rte_trace_global_is_enabled > rte_trace_mode_set > rte_trace_mode_get > rte_trace_pattern > rte_trace_regexp > rte_trace_save > rte_trace_metadata_dump > rte_trace_dump > > # rte_trace_point.h will have all operation related to rte_trace_point_t object > > # rte_trace_provider.h renamed rte_trace_point_provider.h > # rte_trace_register.h renamed to rte_trace_point_register.h > # rte_trace_eal.h renamed to rte_trace_point_eal.h > > > > > > > > > > > > 2) rte_trace_fp_is_enabled() > > > > As a user, what information would this give me? > > "Some fastpath tracepoints are not available" > > > > Moving to rte_tracepoint.h is enough to me. > > IMO, semantically not correct as we are splitting based on some definition. Semantically, rte_trace.h must be the API for simple users enabling traces, while rte_trace_point.h would be used by those adding traces. > How about, > 1) Not expose this API > OR > 2) rte_trace_point.h includes the rte_trace.h > > > > > > > > > # Regarding the API change the following to rte_tracepoint_* > > > > > > #define rte_trace_ctf_u64(val) > > > #define rte_trace_ctf_i64(val) > > > #define rte_trace_ctf_u32(val) > > > #define rte_trace_ctf_i32(val) > > > #define rte_trace_ctf_u16(val) > > > #define rte_trace_ctf_i16(val) > > > #define rte_trace_ctf_u8(val) > > > #define rte_trace_ctf_i8(val) > > > #define rte_trace_ctf_int(val) > > > #define rte_trace_ctf_long(val) > > > #define rte_trace_ctf_float(val) > > > #define rte_trace_ctf_double(val) > > > #define rte_trace_ctf_ptr(val) > > > #define rte_trace_ctf_string(val) > > > It could be done. Just concerned the length of API will be more. like > > > rte_trace_point_ctf_u64 > > > If you have a strong opinion on this then I can change it. > > > > I don't like mentioning ctf here. > > > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > > If we keep one rte_tracepoint_emit_ per line in tracepoint > > declarations, the length is not an issue by looking at how they are > > used. > > OK to remove ctf to make it as rte_trace_point_emit_*. OK? > > > > > Example: > > RTE_TRACEPOINT( > > rte_trace_lib_eal_intr_disable, > > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > > rte_tracepoint_emit_int(rc); > > rte_tracepoint_emit_int(handle->vfio_dev_fd); > > rte_tracepoint_emit_int(handle->fd); > > rte_tracepoint_emit_int(handle->type); > > rte_tracepoint_emit_u32(handle->max_intr); > > rte_tracepoint_emit_u32(handle->nb_efd); > > ) > > > > > > Besides, we don't need to define all those > > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > > rte_tracepoint_provider.h and rte_tracepoint_register.h. > > If we define a helper rte_tracepoint_emit_data(type, in) in > > rte_tracepoint.h, then the "provider" and "register" headers must only > > define how to emit a header (generic and fp cases), then > > rte_tracepoint_emit_data and rte_tracepoint_emit_string. > > The reason for rte_tracepoint_emit_(u|i)(8|16|32|64) to get compile > to check to correct time type used. > See: > rte_trace_point_emit_u32 defintion has > RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(uint32_t))); Is it possible to implement it with a common helper as David suggests?