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 10355A04AE; Tue, 5 May 2020 09:33:25 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 75DB71D158; Tue, 5 May 2020 09:33:25 +0200 (CEST) Received: from mail-il1-f193.google.com (mail-il1-f193.google.com [209.85.166.193]) by dpdk.org (Postfix) with ESMTP id 3050E1D151 for ; Tue, 5 May 2020 09:33:24 +0200 (CEST) Received: by mail-il1-f193.google.com with SMTP id q10so1384495ile.0 for ; Tue, 05 May 2020 00:33:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XdNVvJKkMi3dSp34hVfRu2nlbzbZ8H35GBrnd5f8HBg=; b=QWRWPpcmCDXPCpYhhvf6R6pIYWHzZTF+WG1J1TrGXDeXs8E1ggIMPGR0BGe9eoyqgH xy9iIeZ4FT/xmHjv7+fbdix6h3jOISEMZ0ddt2r3L1ijKn9dvzN3plLrb6yo2lP6yAgE k6bptv6VwCOqOadL2/DO2ZqHalMZ2S+gJXPnx0Uw2xtuo3Sld30JBp7N4/EPawvHoYZV 8Nu+kfGDXRv4oel/hYLF0wucpBKA9fvhPe0rF0NnCfGxKHS1JmjHS32T/7pbSjwS7W2z WF+aqFQv94IlBlcUCRUFcSbQAME+dHrJSHyUXu9qQkkte2EGE0vmCID4jWBCPMy1UZo0 k4hg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XdNVvJKkMi3dSp34hVfRu2nlbzbZ8H35GBrnd5f8HBg=; b=HPBHYaQjHS2GrUK0+5aQDRqqAIzie/aMQ18Csl8DQfTFKNEdD2xZ92CVdIj5eZsMwY eRTm95gpH0/xYsXm2SvpwV9ixwPxKPMkoad60rM3Pz4sbSChsJVYTq82h6ynf2R03vT4 em5PAqHrAWCbiorz9/pNNcLlnnyR3JmvCV2ZmtnlnlgWN7Sa+AVRc+ms5OC9fxWdroLy Jdu3OVaH/0qT8YIpnGPVJ4BFJ9btbI13jT8WHOMlFQbvo9E9wdJOiU+qSCfGTU9jBjEf cIgR+0DbaXIFyk/iYAd0NEOhbIHXulQjUBXncN0UILcPfEJGFRDpqeAPL3ri6ZGKTglc EkYg== X-Gm-Message-State: AGi0PubzdAuQ2UiL2CZGYbv6r6C2XNTyS/biI7MsGfcm8dEze8N+ZFJZ UULxtT80FqPOPq3FNZieX+abTwToYE2+xKjrSZY= X-Google-Smtp-Source: APiQypJIvITg/hMrZwcjR4tPzWca/Ze6tW8I3ynZfXY/o5fWdSNop/ZaFd+izrT3AlIC6zd1ieBnWChMgM+n03DwcVM= X-Received: by 2002:a92:485b:: with SMTP id v88mr2288708ila.271.1588664003226; Tue, 05 May 2020 00:33:23 -0700 (PDT) MIME-Version: 1.0 References: <20200503203135.6493-1-david.marchand@redhat.com> <2596990.BEx9A2HvPv@thomas> <2479551.BddDVKsqQX@thomas> In-Reply-To: <2479551.BddDVKsqQX@thomas> From: Jerin Jacob Date: Tue, 5 May 2020 13:03:07 +0530 Message-ID: To: Thomas Monjalon Cc: David Marchand , dpdk-dev , Jerin Jacob , Sunil Kumar Kori , John McNamara , Marko Kovacevic , Declan Doherty , Ferruh Yigit , Andrew Rybchenko , Olivier Matz Content-Type: text/plain; charset="UTF-8" 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" On Tue, May 5, 2020 at 12:55 PM Thomas Monjalon wrote: > > 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. Why not make, rte_log_register() and the global variable under a macro? That's something done by the proposed patch. > > 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. Please share the data. > The target is to simplify the usage and I prove it: > 1 call replacing 2 calls. That we can the same scheme with rte_log as well. > > >