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 DD8E9A058A; Fri, 17 Apr 2020 10:57:29 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 64D9B1DEBA; Fri, 17 Apr 2020 10:57:29 +0200 (CEST) Received: from mail-il1-f171.google.com (mail-il1-f171.google.com [209.85.166.171]) by dpdk.org (Postfix) with ESMTP id 785061DC7B for ; Fri, 17 Apr 2020 10:57:27 +0200 (CEST) Received: by mail-il1-f171.google.com with SMTP id x2so79697ilp.13 for ; Fri, 17 Apr 2020 01:57:27 -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=Tff2F2AdT9rqOekXngD+NwKDyScJjKQtoXUbbkSLpQc=; b=F5zEn3D5dVWuWX6s4zFnexfDYJfeaU31qF7YjUWbMY4CMnVfZqnGj6NwrvLKlbf3tX 1OWQu4twJO26uSVh25T7YHffNIU2Ane+6EpvFtfa5LbWEU8UQxPbeHgUOHOprveboHCt pkcp+0D0o6h78wlN5Xe4vEP4NNERub06/y2i7cPvs/6pAIr9M5Ep/lNz0ox4PFMx+J/6 IKTcMAF+nHYdvPeGEk2bHkuPxvckPAWKz1qlGYL7R5mj2+CM9GHABa87oQ5y5IBP6NuM DUvQcQ2MssOcWuTesd0DRrvcgrjJHBfNiX0O3GTRY8RkJKjeLkAydDNCmOYhMnBPHgz/ UntA== 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=Tff2F2AdT9rqOekXngD+NwKDyScJjKQtoXUbbkSLpQc=; b=MdS+Ora5aovcbbEyZR5i2WcGcEI0Q95dvTkUbXiA5d4AKFOHygUeRR3qgzVuqUUf3y zOXyi8toIWaqZdHpPOoGkJ9yUcaMpm/F0GJTECx04FvzEGqLlsvYZ5eUfHj8zWAQTduq YLSfjvZHDINBevIrlUGFGYDvvZeHNktOn+KYAJYqQKDWsaCMjlYwTBzhVRGvZCAP8uNK Y/DX3I/kERmhjLGy0m4ckjlXd8/vzRQYbvKa2o5/oS6LcMoxOLGSJI4ULz1AA8E4+Bb1 Lf+78SR1IyCvfh/kN+HESmWo/4BCzdgs3hm38WH2kG9OTkp7cvfx3A3xFxVFMLNJx1aL ZsxQ== X-Gm-Message-State: AGi0PubdeuZtI8ylmJcEA/DPdXYP5BS1UV9OeLyK1ZV9k7hzttpcS7pw muY45SHdyhAqznwrwL0NjVSYXV3j4GFnAHaHaIY= X-Google-Smtp-Source: APiQypKoSLTbA653E3P/3cQmc+sI2hKPmBKDhJh08dJy245vreQ+93v15nGjYyQ1opcugpzMY9Rd+SQoRLnYlU3hZWA= X-Received: by 2002:a92:9942:: with SMTP id p63mr2052017ili.130.1587113846508; Fri, 17 Apr 2020 01:57:26 -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: Fri, 17 Apr 2020 14:27:10 +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 Fri, Apr 17, 2020 at 1:57 PM David Marchand wrote: > > Hello Jerin, Hello David, > > On Thu, Apr 16, 2020 at 6:08 PM Jerin Jacob wrote: > > 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) > > Ok, let's go with rte_trace_point_. Ack. > > > > I will summarise the public API and file name details. Let's finalize. > > > > # rte_trace.h will have > > > > rte_trace_global_is_enabled > > global_ does not make sense anymore. Ack. I will change to rte_trace_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 > > Ok. > > > > > > 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. > > What is rte_trace_fp_is_enabled() supposed to do? > "Test if the trace datapath compile-time option is enabled." > > One implication is that dpdk must be compiled with RTE_ENABLE_TRACE_FP > enabled for users to make use of fp tracepoints later. > It would impose restrictions to final users when they did not compile > the dpdk package themselves. > > > > > > How about, > > 1) Not expose this API > > OR > > 2) rte_trace_point.h includes the rte_trace.h > > > > > > My vote is 1). OK. +1 > > On how to do without it, this should be enough in rte_trace_point_provider.h: > > #ifdef RTE_ENABLE_TRACE_FP > #define __rte_trace_emit_header_fp(t) \ > do { \ > RTE_SET_USED(t); \ > return; \ > } while (0) > #else > #define __rte_trace_emit_header_fp(t) \ > __rte_trace_emit_header(t) \ > #endif > > This way, the user can choose where he enables fp tracepoints in his > code by placing a #undef/#define RTE_ENABLE_TRACE_FP before including > the tracepoints headers. I will make the internal. I prefer to have if (0) c style scheme so that both sections of the code goes through the compilation phase. aka No compilation issue when RTE_ENABLE_TRACE_FP enabled. We have a separate macro declaration for RTE_TRACE_POINT and RTE_TRACE_POINT_FP for Tracepoint selection. > > > > > > # 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? > > 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); > > > ) > > > > > > > > Reading again what I wrote. > > > > 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. > > This part still stands. > > > > 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. > > But this part is inconsistent, I will blame my son (EINTR/EAGAIN). > > I meant: we can have all per-type helpers in rte_trace_point.h. > > rte_trace_point_register.h and rte_trace_point_provider.h both define > a macro (with a common signature) rte_trace_point_emit_data(type, in). > > This way, the rte_trace_point_emit_data implementation in > rte_trace_point_register.h can do the type checking. > rte_trace_point_provider.h macro just ignores the type argument. Yes, if we have rte_trace_point.h as a separate file. I will work on this. Let me see if I can get it right. > > If you don't like it, let's drop this last part. > Thanks. Thanks for the review. > > > -- > David Marchand >