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 1C9E9A04AE; Tue, 5 May 2020 05:43:41 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E42171C440; Tue, 5 May 2020 05:43:40 +0200 (CEST) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by dpdk.org (Postfix) with ESMTP id 743361C43F for ; Tue, 5 May 2020 05:43:39 +0200 (CEST) Received: by mail-io1-f66.google.com with SMTP id i19so496610ioh.12 for ; Mon, 04 May 2020 20:43:39 -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=WPtowjhVxgvkqBMf+5chpqg7m/bl7vTnsRnOZYZTEDQ=; b=JaiVV66nOnmAsvi1rFccIvra8/3g2YUUo8Bni2sG2tB7Plkfz+XlI5GeZhxSERzR9i OcrJsBTsbuicqVFQKwuCInrXvEDZfWwUUyVuznWzh51WIBArTO19ODwJOmYh2ghoaWiD xxfFS/rIGDzUGs0Pz675MrmBkhP5WbcOwQ8PPwrD7vV47gWeuQBge9QW9grNcyUj4DUn 4FOpcNzUFujfiIHuqIoIxDtp1gk/9ugcIHQVBeFoRIP7TYh55974wQDkuOddU9oZzlLl PAgsahH0gfAPQeMRG8INFNjHpLJsW/nQShdJYwk94eaEgVoHTsHq61hFU44YT1G5vltC W66Q== 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=WPtowjhVxgvkqBMf+5chpqg7m/bl7vTnsRnOZYZTEDQ=; b=AGlmVpQBOnQCHbEfD6+Zgg667Huz/ykLDwVtby6coh9ASqfKz3IXQuKrW0T5dKInF+ 3iuSEhqwYsopR2g9rHiXLD/fG5jSYbkb8X5vgLSnU4ZIgGUcOqlsigyHuzXrp2mDs96D d2H6RJTwhzceNcxOYVJOUOyjilMEUKY7B6pKD62+ifNohOLmHMiS+I+lM1MHHx2mGoOO fpjGuXFt3fLGezWcKXq+UOV0Uf7cEWqF3TRhXEocb8YiLbqhZAkHhh8mfbDPM7hpWaly DG1EIz/8VcNLnLhX9aPlXqBYJkwfVd5q2mf91bs+duHvHMzD9UH1qaRSJM1s7BME3O52 8Diw== X-Gm-Message-State: AGi0PubTjL8RrGX6AE0an7S6+kTeSwzQo7FXPNTyjFcaf4mJbDcnDIdW PiCdSIZNOsstMPj7k3bAU14yYxp9guxubzuJn1E= X-Google-Smtp-Source: APiQypJyKsEhP/PaiUcdRuyHsACr5uPjq9YNEw3yRRj5HDlnEy+c6puDXRxSGG5gw/MsZTq1zx8w64vxDi1uIuM/6pQ= X-Received: by 2002:a05:6638:2a2:: with SMTP id d2mr1677200jaq.104.1588650218652; Mon, 04 May 2020 20:43:38 -0700 (PDT) MIME-Version: 1.0 References: <20200503203135.6493-1-david.marchand@redhat.com> <7788528.NyiUUSuA9g@thomas> In-Reply-To: <7788528.NyiUUSuA9g@thomas> From: Jerin Jacob Date: Tue, 5 May 2020 09:13:22 +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 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. If you think, it is better usability, lets add an option for rte_log for the constructor scheme. 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. > >