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 14D57A04AE; Tue, 5 May 2020 09:25:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6C5BC1D52D; Tue, 5 May 2020 09:25:01 +0200 (CEST) Received: from wnew3-smtp.messagingengine.com (wnew3-smtp.messagingengine.com [64.147.123.17]) by dpdk.org (Postfix) with ESMTP id BD6511D155 for ; Tue, 5 May 2020 09:24:59 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.west.internal (Postfix) with ESMTP id 3704348F; Tue, 5 May 2020 03:24:58 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute7.internal (MEProxy); Tue, 05 May 2020 03:24:59 -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=fm1; bh= VQ30HofaydmIdYMCBbnRHhR949fPb7Q0vxMror0VT2M=; b=R+1z+WOqMjNx3zNq iiHfqCs32bRcEuI3Ni8ncIReQM78/w4DjQEI4y5y4Md1IMnyqHkdcFCnXgwbrMvu tLm0sAe2ZaCyLRSHOj1P5CPDn2Q23PXug0RRab2UekV3jD8oHabOPpNgs4Jjbicv cxwxmV0KvUzXh48BamS029y+1CC5jBTzo4Mu4j3H0jAldw3+Ee/yUqzfnwFVI+IK GCn2iHTwe5ClMjPv+/TMVmhIBJ1sfPV8cHKi5/ilimh4hr1zLR1ZmEL+I20UFmJh gkNz2QUs5OyJdOvBsMjF5elXGmZeMoZ8G5zwV+8adM/5vtn8NDvlRUo7Fjokg8Xn lHk3FQ== 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=VQ30HofaydmIdYMCBbnRHhR949fPb7Q0vxMror0VT 2M=; b=4Yw08iGx0MZTwoT6yrWcFJi2r3TPm/+EPW5y6FDQd5UV7i+6+pvVqVtY7 Dzr0KisSTNYopzYL57lEmueIIKQ0n2bXE2y4MYUvugezIYJntKwjPoXg2kZycNth ucvQKApQ44PswQSLU/ApEjvzC0dNxVGdgjFJkHJ0CssiCrQOpfXu7EjS7VXLfsEz sn9OMeaLdGuaBurNVE+WlR5hugPRbdsMnp/nLvfYfAUrRnQIhHG9P8p3qRTA5pOM xB1N5WlWT7tRI6D7cgJ1GVeBTHwgD2lSAE1KFlf1l/9xxy8sbsg1GvDpEQ7cfJtC 27Y0R+/nf63c7dvJ0h+Vc5q30ScJg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrjeehgdduudeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecukfhppeejjedrudefgedrvddtfedrudekgeenucevlhhushhtvghruf hiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghl ohhnrdhnvght 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 1F0A93065FDF; Tue, 5 May 2020 03:24:56 -0400 (EDT) From: Thomas Monjalon To: Jerin Jacob Cc: David Marchand , dpdk-dev , Jerin Jacob , Sunil Kumar Kori , John McNamara , Marko Kovacevic , Declan Doherty , Ferruh Yigit , Andrew Rybchenko , Olivier Matz Date: Tue, 05 May 2020 09:24:55 +0200 Message-ID: <2479551.BddDVKsqQX@thomas> In-Reply-To: References: <20200503203135.6493-1-david.marchand@redhat.com> <2596990.BEx9A2HvPv@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH 2/8] trace: simplify trace point registration 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" 05/05/2020 09:17, Jerin Jacob: > On Tue, May 5, 2020 at 12:31 PM Thomas Monjalon wrote: > > 05/05/2020 05:43, Jerin Jacob: > > > On Tue, May 5, 2020 at 3:01 AM Thomas Monjalon wrote: > > > > 04/05/2020 19:54, Jerin Jacob: > > > > > On Mon, May 4, 2020 at 11:10 PM David Marchand > > > > > > On Mon, May 4, 2020 at 7:19 PM Jerin Jacob wrote: > > > > > > > On Mon, May 4, 2020 at 10:38 PM David Marchand > > > > > > > > On Mon, May 4, 2020 at 4:39 PM Jerin Jacob wrote: > > > > > > > > > On Mon, May 4, 2020 at 7:34 PM David Marchand wrote: > > > > > > > > > > On Mon, May 4, 2020 at 4:47 AM Jerin Jacob wrote: > > > > > > > > > > > On Mon, May 4, 2020 at 2:02 AM David Marchand wrote: > > > > > > > > > > > > > > > > > > > > > > > > RTE_TRACE_POINT_DEFINE and RTE_TRACE_POINT_REGISTER must come in pairs. > > > > > > > > > > > > Merge them and let RTE_TRACE_POINT_REGISTER handle the constructor part. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Initially, I thought of doing the same. But, later I realized that > > > > > > > > > > > this largely grows the number of constructors been called. > > > > > > > > > > > I had concerns about the boot time of the application and/or loading > > > > > > > > > > > the shared library, that the reason why spitting > > > > > > > > > > > as two so that constructor registers a burst of traces like rte_log. > > > > > > > > > > > > > > > > > > > > I am a bit skeptical. > > > > > > > > > > In terms of cycles and looking at __rte_trace_point_register() (which > > > > > > > > > > calls malloc), the cost of calling multiple constructors instead of > > > > > > > > > > one is negligible. > > > > > > > > > > > > > > > > > > We will have a lot tracepoints latter, each one translates to the > > > > > > > > > constructor may not be a good > > > > > > > > > improvement. The scope is limited only to register function so IMO it > > > > > > > > > is okay to have split > > > > > > > > > just like rte_log. I don't see any reason why it has to be a different > > > > > > > > > than rte_log. > > > > > > > > > > > > > > > > What is similar to rte_log? > > > > > > > > There is neither RTE_LOG_REGISTER macro, nor two-steps declaration of > > > > > > > > dynamic logtypes. > > > > > > > > > > > > > > > > > > > > > Here is an example of rte_log registration. Which has _one_ > > > > > > > constructor and N number of > > > > > > > rte_log_register() underneath. > > > > > > > > > > > > rte_log is one thing, rte_trace is already different. > > > > > > > > > > > > There is _no macro_ in rte_log for registration. > > > > > > The reason being in that a rte_log logtype is a simple integer without > > > > > > any special declaration requiring a macro. > > > > > > > > > > I just wrapped in macro for convincing, but it has the same semantics. > > > > > global variable and API/macro to register. > > > > > > > > > > > > > > > > > > > > > > For tracepoints, we have a special two steps thing: the tracepoint > > > > > > handle must be derived from the tracepoint name. > > > > > > Then this handle must be registered. > > > > > > What I proposed is to make life easier for developers that want to add > > > > > > tracepoints and I suppose you noticed patch 1 of this series. > > > > > > > > > > To reduce the constructors. I don't want trace libraries to add lot of > > > > > constructors. > > > > > I don't think it simplifies a lot as the scope of only for registration. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > One of the thought process is, we probably remove the constructor > > > > > > > > > scheme to all other with DPDK > > > > > > > > > and replace it with a more register scheme. In such a case, we can > > > > > > > > > skip calling the constructor all tother > > > > > > > > > when trace is disabled. > > > > > > > > > > > > > > > > Sorry, but I have a hard time understanding your point. > > > > > > > > Are you talking about application boot time? > > > > > > > > > > > > > > Yes. The optimization of application boottime time in case of static > > > > > > > binary and/or shared library(.so) load time. > > > > > > > > > > > > As Thomas mentioned, do you have numbers? > > > > > > > > > > No. But I know, it is obvious that current code is better in terms of > > > > > boot time than the proposed one. > > > > > I would like to not add a lot of constructor for trace as the FIRST > > > > > module in DPDK. > > > > > > > > No, it is not obvious. > > > > The version from David looks simpler to use and to understand. > > > > Without any number, I consider usability (and maintenance) wins. > > > > > > Thanks for the feedback. > > > > > > As the trace maintainer, I would like not to explode constructor usage > > > for trace library. > > > My reasoning, We could do trace registration without this constructor scheme. > > ??? > > We don't need this patch to make trace to work. > > > > > > > > If you think, it is better usability, lets add an option for rte_log > > > for the constructor scheme. > > > > It makes non-sense. > > rte_log requires only one function call per log type. > > Here is the example of the log registration: > > global variable: > int otx2_logtype_base; > int otx2_logtype_mbox; > int otx2_logtype_npa; > > RTE_INIT(otx2_log_init); > static void > otx2_log_init(void) > { > otx2_logtype_base = rte_log_register("pmd.octeontx2.base"); > otx2_logtype_mbox = rte_log_register("pmd.octeontx2.mbox"); > otx2_logtype_npa = rte_log_register("pmd.mempool.octeontx2"); > } > > What the proposed patch here. > # Making N constructors from one > # Grouping global variable and register function under a single Marco > and making it as N constructors. > Why can we do the same logic for rte_log? rte_log is simple, there is nothing to simplify. This comparison makes no sense. > > rte_trace requires 3 macros calls per trace type: > > RTE_TRACE_POINT_REGISTER, RTE_TRACE_POINT_DEFINE, RTE_TRACE_POINT_ARGS > > This patch is unifying the first 2 macro calls to make usage simpler, > > and ease rte_trace adoption. > > RTE_TRACE_POINT_ARGS is NOP and for the syntax. > It is similar to rte_log. rte_log don't have RTE_TRACE_POINT_REGISTER instead > it is creating global variable see, "int otx2_logtype_base; > > > > > Note: the other usability weirdness is mandating declaring each trace > > function with a magic double underscore prefix which appears nowhere else. > > > > > > > Analyze the impact wrt boot time and cross-platform pov as the log > > > has a lot of entries to test. If the usage makes sense then it should make sense > > > for rte_log too. I would like to NOT have trace to be the first > > > library to explode > > > with the constructor scheme. I suggest removing this specific patch from RC2 and > > > revisit later. > > > > You still did not give any argument against increasing the number > > of constructors. > > If you are proposing the new scheme, you have to prove the overhead > with a significant number of constructors > and why it has differed from existing scheme of things. That's is the > norm in opensource. I say there is no overhead. The target is to simplify the usage and I prove it: 1 call replacing 2 calls.