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 E47B6A04AE; Tue, 5 May 2020 09:17:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 222281D52D; Tue, 5 May 2020 09:17:57 +0200 (CEST) Received: from mail-il1-f175.google.com (mail-il1-f175.google.com [209.85.166.175]) by dpdk.org (Postfix) with ESMTP id 1DC521D158 for ; Tue, 5 May 2020 09:17:56 +0200 (CEST) Received: by mail-il1-f175.google.com with SMTP id r2so250902ilo.6 for ; Tue, 05 May 2020 00:17:56 -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=H+2zP2GAV337udZNRw0nOvNtnEZl5feRYK+eDILwrjo=; b=jYgYSUMkZXwQh/bpqLyswok8FLq5yqyzTCdm+GzkH9q4VkCGtxHcqe+zibA1MaTB1V 1c2rTRcaAsOtjCwQ/mDdRYEQ3zzNozNxXiElMyDqKpA5GDICee+yjZwb09FD9/fzcT70 USwnw02uUOia08+G1/KfC8YPHtD3bgEWE3lEeQCAVKSGgFuIpTuAhckigA5f/B2U1Ddd g1mw1TrX8lSqTWGI72k3l3njocg34Jz9TYJkyaLltHJRpyiXEWcJ3Jq6UB8VukIwKnEb Znv4mxWKzLEzSwf2TXSZvGNi5Uo0SHW18zztk4gv/i3UYwOW/FKxKmYOMwI2Be6WJbKk 9c6w== 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=H+2zP2GAV337udZNRw0nOvNtnEZl5feRYK+eDILwrjo=; b=j3ffEwfzXnOFxooCRBzXr3nKXdbt3LoidSdA04mpTPADEAdcfAIDzbVWAy9Mr68bfx 0fJ5oMViol0a/k5eQvmBHV/Tdw4xCFQ6ClLC3bvVlPyXNcjsnM8A0O1AV4fjDNtbISQn K7K9i/axsSeRCPFbY+G0Blg/Mqa1/hwdrjaX2MsHDySEBVS5w/ItyPE/qXvGi+0SkvfR H02DRB58OcB+jcxLYlxy3zE/rC4sdE5vaU8PdN1bwKqOnSjeadYkArRboo9DslzKvQ4A sGRE2xx7l0tGVQ8WWnEUR7/F8EryxjOFFQNQvQOB0zPil7HbWKIumH/pTicG02ugbpgc ITqw== X-Gm-Message-State: AGi0PuZTwVrI2PYu5gHjfBpsutT4V9ozruKIslKg4o6uycsQOnIZeo1H 9Zs1UZgpwIIYmJtTurg6RJ1f3xjSEF71ynVXDAM= X-Google-Smtp-Source: APiQypLiZEKVuZoKiCXo8zKpzb8KZRCTaK6m/Jb/cy3xro+qKqdo5w31hLXXuhX5rQsZl/eVWBfW+FMFCqHUgKbqIxA= X-Received: by 2002:a92:9edb:: with SMTP id s88mr2310927ilk.294.1588663075246; Tue, 05 May 2020 00:17:55 -0700 (PDT) MIME-Version: 1.0 References: <20200503203135.6493-1-david.marchand@redhat.com> <7788528.NyiUUSuA9g@thomas> <2596990.BEx9A2HvPv@thomas> In-Reply-To: <2596990.BEx9A2HvPv@thomas> From: Jerin Jacob Date: Tue, 5 May 2020 12:47:39 +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: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_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. > >