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 43ED5A0588; Thu, 16 Apr 2020 18:08:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0CC051D914; Thu, 16 Apr 2020 18:08:52 +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 0269B1D904 for ; Thu, 16 Apr 2020 18:08:50 +0200 (CEST) Received: by mail-io1-f66.google.com with SMTP id e127so6274587iof.6 for ; Thu, 16 Apr 2020 09:08:50 -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=Uyp+j+K+W42UhM98RJR+BiXmDy2mMqAldlXpwr4NMnQ=; b=QHCA9hSI/s0JVQp/om05D2atf40GfradYUk+VQxHkTo4PLQBc/ATAxLgFfI3jL4DY3 tGOtFwUC+eAAGjbBPV6yJlZ54YhTgq26LsFsDCsQV6igLzEXv6Ot+EP3/0aVLi3gLmJ6 2g+BtNlRo2mHZW3PD0F7QYWrDSO8RfcBc2qwmL7fgf8el1kaQya755A/0nTAPdY9VuCD bufXzsEgkaEOYEKqTOM8C5U8SAdYbu0ByAU0Z48aiZaSAsR7MrdIHWvh20GYYBe00el2 4p/ZPgIjXwIynXXQnFYRnWz7Fy4teffPU6YtirmIOlfY1N9B73bSGeiPfyLeP3GYZ+pE 3Hdg== 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=Uyp+j+K+W42UhM98RJR+BiXmDy2mMqAldlXpwr4NMnQ=; b=riIXZjR2Bh37lt1VEozpSKKq7tXrrjmIf/H2eHShPZUh+fXPNkrP9gQFWPYP64v8a8 qp/XRBXxAX7ORS3D8aXp36eptPG23EA0In0KVVig3HusUCS+grYps4Pj6NJyV8aL5qMn t5f2GWt1ZDU6mpu7neKpJAGGuSDLNqpEdb1LLETuxVhBhCuCiMOHWGxlNnv69ZpQp5RV 6qNKHLlD9BIQjn2oFsNQ35rCGOIexFPIEdqt1JETxNdrcM2qXABn75XqNJ20a02CDuUz YJJoSyibA/zZlS/VHhQxBm80cJh00HG7p3HdgsmbBhtErukSN9PS+DFG1J2nFIUdc19i MOqQ== X-Gm-Message-State: AGi0PuYs8O6H5ZU6EpDy5No3NthP60GajtDQyEBA15bHQlFVB5BeCKt+ dyKtRvXr2wxDOPPgu9M7zi47TmLZ8o6iYMusWOc= X-Google-Smtp-Source: APiQypJXI24OoqNyczRL7AE77e1kgdz/9tIKSjZ0uhcISY8LW4oa2hZ8yWS8M41fmePGoQ3ihsFKsIIy01UND7Y6yU0= X-Received: by 2002:a05:6602:2f87:: with SMTP id u7mr30922535iow.94.1587053329335; Thu, 16 Apr 2020 09:08:49 -0700 (PDT) MIME-Version: 1.0 References: <20200403153709.3703448-1-jerinj@marvell.com> <20200413150116.734047-1-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Thu, 16 Apr 2020 21:38:33 +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 Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v5 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 Thu, Apr 16, 2020 at 7:09 PM David Marchand wrote: > > On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob wrote: > > > - What do you think of splitting the API in two headers, thinking > > > about who will use them? > > > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > > > users of the trace framework that want to > > > * get the status of the whole trace subsystem, > > > * enable/disable tracepoints by pattern/regexp, > > > * dump the current events, > > > * rte_tracepoint.h (rte_tracepoint_ prefix for all > > > functions/macros/types) for developers that want to add tracepoints to > > > their code > > > > # Initially, I thought of doing the same. > > Later realized that some of the definitions such as following > > > > 1) > > /** The trace object. The trace APIs are based on this opaque object. */ > > typedef uint64_t rte_trace_t; > > As a user, I would ask the trace framework to enable tracepoints by > calling rte_trace_pattern()/rte_trace_regexp(). > This does not require the tracepoint descriptor to be exposed in rte_trace.h. > > > If some application wants to store/manipulate the descriptors, then it > will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object > and API are: > - rte_tracepoint_lookup (currently named rte_trace_by_name) > - rte_tracepoint_enable > - rte_tracepoint_disable > - rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid, > should be private, as discussed) > - rte_tracepoint_is_enabled > - RTE_TRACEPOINT/_FP macros > - rte_tracepoint_register etc... >From the prototype onwards, Myself shuffled abound multiple times on the API name to satisfying names. If you would like to classify based on the tracepoint object dependency to a new header file, it is fine. Let's go the last round for API naming details. I think, trace being the domain, IMO, it better to call the trace point API with rte_trace_point_* and trace point object to rte_trace_point_t (vs rte_tracepoint_t) I will summarise the public API and file name details. Let's finalize. # rte_trace.h will have rte_trace_global_is_enabled rte_trace_mode_set rte_trace_mode_get rte_trace_pattern rte_trace_regexp rte_trace_save rte_trace_metadata_dump rte_trace_dump # rte_trace_point.h will have all operation related to rte_trace_point_t object # rte_trace_provider.h renamed rte_trace_point_provider.h # rte_trace_register.h renamed to rte_trace_point_register.h # rte_trace_eal.h renamed to rte_trace_point_eal.h > > > > > > 2) rte_trace_fp_is_enabled() > > As a user, what information would this give me? > "Some fastpath tracepoints are not available" > > Moving to rte_tracepoint.h is enough to me. IMO, semantically not correct as we are splitting based on some definition. How about, 1) Not expose this API OR 2) rte_trace_point.h includes the rte_trace.h > > > > # Regarding the API change the following to rte_tracepoint_* > > > > #define rte_trace_ctf_u64(val) > > #define rte_trace_ctf_i64(val) > > #define rte_trace_ctf_u32(val) > > #define rte_trace_ctf_i32(val) > > #define rte_trace_ctf_u16(val) > > #define rte_trace_ctf_i16(val) > > #define rte_trace_ctf_u8(val) > > #define rte_trace_ctf_i8(val) > > #define rte_trace_ctf_int(val) > > #define rte_trace_ctf_long(val) > > #define rte_trace_ctf_float(val) > > #define rte_trace_ctf_double(val) > > #define rte_trace_ctf_ptr(val) > > #define rte_trace_ctf_string(val) > > It could be done. Just concerned the length of API will be more. like > > rte_trace_point_ctf_u64 > > If you have a strong opinion on this then I can change it. > > I don't like mentioning ctf here. > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > If we keep one rte_tracepoint_emit_ per line in tracepoint > declarations, the length is not an issue by looking at how they are > used. OK to remove ctf to make it as rte_trace_point_emit_*. OK? > > Example: > RTE_TRACEPOINT( > rte_trace_lib_eal_intr_disable, > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > rte_tracepoint_emit_int(rc); > rte_tracepoint_emit_int(handle->vfio_dev_fd); > rte_tracepoint_emit_int(handle->fd); > rte_tracepoint_emit_int(handle->type); > rte_tracepoint_emit_u32(handle->max_intr); > rte_tracepoint_emit_u32(handle->nb_efd); > ) > > > Besides, we don't need to define all those > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > rte_tracepoint_provider.h and rte_tracepoint_register.h. > If we define a helper rte_tracepoint_emit_data(type, in) in > rte_tracepoint.h, then the "provider" and "register" headers must only > define how to emit a header (generic and fp cases), then > rte_tracepoint_emit_data and rte_tracepoint_emit_string. The reason for rte_tracepoint_emit_(u|i)(8|16|32|64) to get compile to check to correct time type used. See: rte_trace_point_emit_u32 defintion has RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(uint32_t))); > > > > > - Having functions "is_disabled" has little value when a "is_enabled" > > > counterpart exists. > > > > I thought so. I like to have "is_enabled". But in the code, > > "is_disabled" has been used more. > > Then finally added both in API. Please share your preference, I will > > do the same in v6. > > is_enabled is enough. OK. I will remove is_disabled() > > > > > - I did not get why we put the trace descriptors in a specific elf > > > section, can you explain the benefits? > > > > Those are global variables of size 8B. Since it is used in fastpath. > > I just want all of them to same area for > > 1) It is a mostly a read-only area. Not mix with other "write" global variables > > 2) Chances that same subsystem FP variables comes same fastpath cache line. > > In short, We will be more predictable performance number from build to build i.e > > not depended on other global variables from build to build. > > Ok, thanks for the explanation. > It is worth a few words in the associated commitlog for posterity. OK. I add it in git commit. >