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 A2CD8A059F; Fri, 10 Apr 2020 16:38:05 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 063C91D537; Fri, 10 Apr 2020 16:38:05 +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 9C6441D501 for ; Fri, 10 Apr 2020 16:38:03 +0200 (CEST) Received: by mail-il1-f193.google.com with SMTP id i14so1952399ilr.11 for ; Fri, 10 Apr 2020 07:38:03 -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=ool25waUQte2LpRzCAZrYGHG1ON7g5/4ETXRa2nS+V0=; b=NI07WuhHM7DhA5MF5+M6iDeyvt2oWJjjS1Owut47CM+WxYaeZUWo9ZdIzrDt4BN3gK PsMC4/xNHtQ/HNWb5/H2tTllSF52mBbFkkLLAg4IZilL3t9LTq/lHNutrHjQzrf8XsQr qh2VQqPBt0RP7tcKM+D491YOrsIi3lgv2bFGVxB0p/VB4AmpRZYXO/SYHT30Tt2e1Mf7 iwV8mdHamGR91vud4nPMjPtFd/h+ZUke1IV6lUDepijAJJ1dMAqjS79A8EstpSxtfMAv xihEg4GXqlpJhx+wMkVvsUbG62MH08AC0KwAtbMzPcIeyzkinMetn68FlTzSx0rHc+iq GAqQ== 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=ool25waUQte2LpRzCAZrYGHG1ON7g5/4ETXRa2nS+V0=; b=G2pNeropRQ/fJIK7VcQEaObld6TEYjG1kjJfRGtBiTH6JV5vWw9q7kFVTJcVaVgvqn fniNmIc8gzJq3hCk3iJLFMWA1aGGtxiErWXg4/J32jaq3R56Q5WSAtUOL8dDBb+T+vvI wKj1mNclQb5lHOCtyRPRdm1yE3f4MITv0WPsiGdBF0EYq1hohFMFA5LU5Q9DaYl+SHJB BxT5rme++0ikfxOVAX3zr1M1iuhViR4dtPeMxrvopA5Rj718IG/EYXpn0pBpGOYc5ASR 02u/JpaVlXUNvSpe8Y6pI0DiAFzQG5TKd2rugS78D4UQFkpy1yVyvHqk5GOWOHDRhzNK 5wxA== X-Gm-Message-State: AGi0PuZO9Ab++1zjWGwflEUNp7kNSNMKrh2/B8ze+uArkee638y9suIN pXLnRRXjpv8Bp2ZvvnGxTSjtgZ2mNIqgtMnMqMU= X-Google-Smtp-Source: APiQypIcqHqjZlateMotu9zlIM2eUIQyq2wHOOkr//VZvIDMCgGxlLCEDOIi0mbyJo5Bo37+dlpXlkm0wamkv6w9sHc= X-Received: by 2002:a92:cecd:: with SMTP id z13mr5383545ilq.271.1586529482907; Fri, 10 Apr 2020 07:38:02 -0700 (PDT) MIME-Version: 1.0 References: <20200329144342.1543749-1-jerinj@marvell.com> <6115809.K2JlShyGXD@thomas> <14100064.JCcGWNJJiE@thomas> In-Reply-To: From: Jerin Jacob Date: Fri, 10 Apr 2020 20:07:46 +0530 Message-ID: To: David Marchand Cc: Thomas Monjalon , Jerin Jacob , dpdk-dev , "Richardson, Bruce" , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Sunil Kumar Kori , Olivier Matz Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v4 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 Fri, Apr 10, 2020 at 7:15 PM David Marchand wrote: > > On Fri, Apr 10, 2020 at 3:30 PM Jerin Jacob wrote: > > > > On Fri, Apr 10, 2020 at 6:45 PM David Marchand > > wrote: > > > > > > On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob wrote: > > > > > The global level is just disabling some logs even if it is enabled > > > > > in the logtype level. > > > > > It only makes usage complicate. > > > > > We should consider only logtype levels. > > > > > > > > OK. Do we care about the following use case? > > > > # Trace only specific types of events(example, DEBUG or CRITICAL)? > > > > > > The event types can be encoded in the tracepoint names if we want to > > > organise trace points with types (maybe as a suffix). > > > > > > > > > > > IF NOT, > > > > > > > > There is no need for, > > > > # rte_trace_global_* API > > > > # No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove > > > > rte_trace_level_set() API etc > > > > > > > > > > > > # In summary: > > > > ~~~~~~~~~~~~~ > > > > # In the existing code: > > > > The trace will be emitted when > > > > a) When the trace is enabled > > > > AND > > > > b) trace level than the global level. > > > > > > > > # in new suggestion > > > > The trace will be emitted when > > > > a) When it is enabled > > > > > > > > And the EAL argument will be --trace=regex/globbing, This EAL argument > > > > will call rte_trace_regexp() and enable the selected tracepoints. > > > > > > > > The downside will be it will not be similar to log API. I am fine with > > > > either way. > > > > > > The level notion in the traces api is artificial. > > > I prefer a simple api where tracepoints state is controlled via a > > > single criteria: with the new suggestion that would be names. > > > So +1. > > > > I thought it may be helpful to replace log and I decided to pick the > > level in the trace. > > > > Looks like the consensus is to remove "level" from Trace. > > OK. I will rework the Trace API to remove the "level" and > > rte_trace_global_* API from Trace library. > > Let me know If you have any other top-level architecture comments if any. > > I will send v5 with new changes. > > Thanks Jerin. Thanks, David for the comments. > > > - I am still looking at the event record mode. > I just wonder why we have this notion per tracepoint. > The documentation talks about it being an attribute of the trace > buffers, and the described behavior looks fine. > But if we can set this mode per tracepoints, once a trace buffer is > full, won't it be hard to figure out why some events have been caught > and not others? In fastpath, I tried to avoid reading multiple control variables to improve performance. i.e the tracepoint(uint64_t) has all control information. So it has given a feature to control mode per tracepoint. I thought it may be useful. No strong option here. If you think, it is not usefull and creates more problems, I will change to the following: 1) Remove int rte_trace_mode_set(rte_trace_t trace, enum rte_trace_mode_e mode); 2) Change From: void rte_trace_global_mode_set(enum rte_trace_mode_e mode); to: void rte_trace_mode_set(enum rte_trace_mode_e mode); Let me know. > > > - Another comment, on the form, I can see that we talk about dataplane > tracepoints and fastpath API. > Is there a difference? or could everything be named in a consistent No. Both are the same. > way (the headers would have to be aligned too)? Personally I like the fast path. The log calls it as DP( see RTE_LOG_DP). In order to have consistency, I will pick the DP and change it everywhere to DP, and header files to xxxxx_dp.h from xxxx_fp.h. If you have a preference then I will change to the same. Let me know. > > > - Do we really need to export the rte_trace_lib_eal_generic_* trace points? > They seem more like examples and I don't see a usage outside of the tests. This generic tracepoint for any quick debug, For example, if some need to add tracepoint to just emit string in the middle of debugging a problem, they can use rte_trace_lib_eal_generic_str. That's the reason for exporting it. > > > -- > David Marchand >