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 2A226A057B; Wed, 1 Apr 2020 12:05:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9169D1BEAF; Wed, 1 Apr 2020 12:05:16 +0200 (CEST) Received: from mail-il1-f177.google.com (mail-il1-f177.google.com [209.85.166.177]) by dpdk.org (Postfix) with ESMTP id 53F261BEAF for ; Wed, 1 Apr 2020 12:05:15 +0200 (CEST) Received: by mail-il1-f177.google.com with SMTP id r5so22443338ilq.6 for ; Wed, 01 Apr 2020 03:05:15 -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=FmfxBfeuJD43x3PN5XWsQDJ0q5lI8rfkEk6uvqQ5wKU=; b=km1gCXWT3nZww4GX/7CTMsrzUOt2aIfHRuOp2NOdrHFDu3sIpETgxthHqygKWotMuj j03HPeXeGkArrVVp4qEto45BeXm2N/j+HvvPjU9enUYLSjlKFOoCKjf7WLgFYdKpeAbC JYkOAWSoXU+5Q2e0H6Ov3HUnX239oZ1GuO83EuERey3QvUO7UxwKbCgF1dWhcXSs7mF5 ZLvov4Qp0RXDoBjynIFsdaEBZn410zvd0lv7eE5dVE6FIBDuiPMkHOlE3NpcdCevRQqe cmlYOe/m99RSYcGC0NqXpX9hAoPOpiS0z3UNedy6jw5N823XbsCXNay20IUd0PepW8IY tr9w== 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=FmfxBfeuJD43x3PN5XWsQDJ0q5lI8rfkEk6uvqQ5wKU=; b=THpvW1zZJ+oPlsqrUy+yW97i2k3KewVQeuQ7G0USih/C1QEuek7PC27VhwLBT2XwgN ujOt2XhrFgz2EcuaAutroCL9om2ffgukW6ROa4yJU5FFZscSUezzBu8KkqGHvWpLu6ZO FXYa1YOXocBAtLARy776mVRGWHPg6U3JX+jcsgpR6BoUP+zwTZY+0/PC3nz4NTkQyOF+ I/ggqn9aSAJpzKM0zsNXBJi/cGlhy7FCZOpVim1Dgl4NceozqZZd3+ExrvJx8SyVLS7p 15U53rXKBT66R/elvpaiKGORxpwGl00FDm0r82/CbncVOWiG4Kz5jJfODV9lz25zdhD4 DRDQ== X-Gm-Message-State: ANhLgQ0rxIcNG4ZZldT4AV67XwpyUTh/pBNhgKbCAP8mdh7SfilY+NxC ZF/HXHXrq0TVF9tcneapWNIBzftSoBQiwO+ldsU= X-Google-Smtp-Source: ADFU+vtwyYUWavsZ/ecaMvxAz+RUn19iugpR+0PB4kHIhpi83JF0MxOphZaj85Lbqfy9U+tjQEumn1Xm3IlDI6mnfH4= X-Received: by 2002:a92:cbc6:: with SMTP id s6mr20794734ilq.271.1585735514419; Wed, 01 Apr 2020 03:05:14 -0700 (PDT) MIME-Version: 1.0 References: <20200325211603.240288-1-jerinj@marvell.com> <20200329144342.1543749-1-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Wed, 1 Apr 2020 15:34:57 +0530 Message-ID: To: David Marchand Cc: Jerin Jacob Kollanukkaran , dev , Thomas Monjalon , Bruce Richardson , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Sunil Kumar Kori , "Yigit, Ferruh" , Andrew Rybchenko , Declan Doherty , Olivier Matz , Neil Horman , Ray Kinsella Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3 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" On Wed, Apr 1, 2020 at 1:49 PM David Marchand wrote: > > Hello Jerin, Hello David, Thanks for the review. > > On Sun, Mar 29, 2020 at 4:43 PM wrote: > > Items that needs to be sort it out > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > # Makefile and meson.build are updated to allow experimental APIs. > > > > As multiple EXPERIMENTAL symbols, exported by trace library, are > > used in various drivers, lib, app and examples. So to fix compilation > > warning/error, Makefile and meson.build are updated for all required > > components to support EXPERIMENTAL APIs. > > It results same code changes at multiple components as well as > > increases source code line changes in patchset too. > > Suggestions are welcome to resolve this issue with lesser code changes. > > - Regardless of the trace framework, the ALLOW_EXPERIMENTAL_API flag > gates access to APIs so that external users are aware of their status. > I have been considering setting this flag unconditionally for internal > users in the top Makefile/meson for app/ lib/ and drivers/. > I could look at this and prepare a patch about this, but this is not > enough here. It makes sense to me. Let me know when you are planning to send that patch, I will rebase this series on top of that. If you don't have time then I can send the patch too. I assume the patch content will be: 1) Removing experimental API from app, lib, drivers, examples with make and meson 2) Have it enabled at the global level. > With the trace framework, we add experimental symbols in mempool or > ethdev inlines, which then inherit the experimental check. > This would break compilation of external code. Users are then forced > to enable experimental API. I agree with the analysis. > > How about: > * we introduce a global config switch that enables/disables the trace > framework (off by default): the people who want to test it and help > stabilize it will have to deal with the experimental flag for now, > * in 20.11, the trace points are put into the stable ABI, and the > option is removed, IMO, the better alternative would be: Since the trace changes in the "inline" functions of the specific library already disabled under _compile time_ RTE_ENABLE_TRACE_DP flag and it is using RTE_TRACE_POINT_DP() to define the trace unlike slow path trace like RTE_TRACE_POINT(). So I can improve RTE_TRACE_POINT_DP() to make absolute NOP if ALLOW_EXPERIMENTAL_API not defined. On the upside, The tracing code will be enabled by default(runtime it is disabled by default anyway). If some need to fastpath API tracing then ALLOW_EXPERIMENTAL_API need to enable. So this won't break applications. > > > - With the patchset rebased on the current master (could be my fault, > so take it with a grain of salt), the ALLOW_EXPERIMENTAL_API flag is > not passed when compiling the l3fwd example against an installed dpdk. I will check. We have added ALLOW_EXPERIMENTAL_API flag where we got compilation issues. > > > - On the MAINTAINERS update: > > Trace > M: Jerin Jacob > M: Sunil Kumar Kori > F: lib/librte_eal/include/rte_trace*.h > F: lib/librte_eal/common/eal_common_trace*.c > F: lib/librte_eal/common/eal_trace.h > F: lib/librte_ethdev/ethdev_trace_points.c > F: lib/librte_ethdev/rte_trace_ethdev*.h > F: lib/librte_eventdev/eventdev_trace_points.c > F: lib/librte_eventdev/rte_trace_eventdev*.h > F: lib/librte_cryptodev/cryptodev_trace_points.c > F: lib/librte_cryptodev/rte_trace_cryptodev*.h > F: lib/librte_mempool/mempool_trace_points.c > F: lib/librte_mempool/rte_trace_mempool*.h > > Once we exclude the trace framework, the trace points themselves > should be the responsibility of the component (ethdev, eventdev...) > maintainers (copied them). Yes. I will remove the following from the MAINTAINERS update in the next version. F: lib/librte_ethdev/ethdev_trace_points.c F: lib/librte_ethdev/rte_trace_ethdev*.h F: lib/librte_eventdev/eventdev_trace_points.c F: lib/librte_eventdev/rte_trace_eventdev*.h F: lib/librte_cryptodev/cryptodev_trace_points.c F: lib/librte_cryptodev/rte_trace_cryptodev*.h F: lib/librte_mempool/mempool_trace_points.c F: lib/librte_mempool/rte_trace_mempool*.h > > > - I had something more dynamic in mind for gathering traces: like > enabling/disabling trace points in a running process and getting the > traces on the fly. rte_trace_enable() and rte_trace_disable() already avilable for this. > But I suppose the current framework is enough and we can work on this later. Yes. for all new features. > > > > > > > More details: > > ~~~~~~~~~~~~~ > > > > # The Native DPDK CTF trace support does not have any dependency on > > third-party library. > > The generated output file is compatible with LTTng as both are using > > CTF trace format. > > > > The performance gain comes from: > > 1) exploit dpdk worker thread usage model to avoid atomics and use per > > core variables > > 2) use hugepage, > > 3) avoid a lot function pointers in fast-path etc > > 4) avoid unaligned store for arm64 etc > > > > Features: > > ~~~~~~~~~ > > - APIs and Features are similar to rte_log dynamic framework > > API(expect log prints on stdout vs it dumps on trace file) > > I am not sure about the global and per tracepoint levels. > We must enable each tracepoint anyway, the level is just a commodity. > > I don't have strong arguments against it, but it feels odd to > copy/paste the rte_log framework. I have intentionally kept public API similar to rte_log wherever it is possible. Reason being, 1) In the future, it is easy to replace rte_log with trace _if needed_. 2) Avoid the new API learning curve. /Jerin > > > -- > David Marchand >