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 355F2A0563; Wed, 15 Apr 2020 16:40:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0A68B1D73B; Wed, 15 Apr 2020 16:40:13 +0200 (CEST) Received: from mail-io1-f65.google.com (mail-io1-f65.google.com [209.85.166.65]) by dpdk.org (Postfix) with ESMTP id 7E46A1D6A5 for ; Wed, 15 Apr 2020 16:40:11 +0200 (CEST) Received: by mail-io1-f65.google.com with SMTP id b12so17312488ion.8 for ; Wed, 15 Apr 2020 07:40:11 -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=F/j2tRRjfqbdTTjY97g+QwUYmyOtnheJcjBzgmA+G3M=; b=SRayig2fz3UvS/ZIHkCNkFRS6bPcUOzW83uzBuTviu0duqnGuBZZ9NNW4L2NNz98Wq 7sgXLeDadyhDVtmfy6Iv6Iq5Fa4pr9mPG3mbI8MdQOoCdiWfW6KAGSRO+71hwCUo9Wrp aXxLcUlc8tzx1DQluKIBskrK3iG+ko4/QuQnYam0iuWqGRh2RdsmGPUixmaFMP6HgjMH wr7E2B39h/hFLmsuLh4hwoOUcmfGkjkB6GkEH1hZuOfug3gHyHWUHVkPylpQTQwsSTpk fkykTENED70jTlND2349ZZaufTTqHekiB7GOP1zk5OGIY48sQAzqwE8kqsYZeO2jJvyB EBmg== 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=F/j2tRRjfqbdTTjY97g+QwUYmyOtnheJcjBzgmA+G3M=; b=YqKEKzVGyKl/6w+8larw85r4jCLmL6sjOzGcB7bIQ8eQrTpJI9PPxtFMJ1vZK/pi8K jJvtv+gofy5oOHzwirvA19qQJ2AJ0AhLu3kwcxP5R5eraRjpirrBYO0+NkvznilygXPq kSJJnCTlw6i8Szg3TSJSkqTCdXBp7pXQDv966bdLPrWIiRmx5piiR8jDPMv+a3SUcQgg LV3Wlf9zpiDihsv0E/fl/cn8N9lhQRLaoj2iGNqqyYYMjvt+MC/5BRNMd+44f6VIjKUN t3Jw7I0CxOzDYgkIpupfpCSL2MO7IAXC8yltKFocdcY3pIR2Ebo+9p8XAGcyCiQrSsg+ /I+g== X-Gm-Message-State: AGi0PuaQ9kV0D7IobET0OR5bHEF42TZhH7fF+4si3trLTxRxQ1iplqdj 9Fc870ot5VPfFF+MPUVxD3XS60yTxu0rsmP7yNM= X-Google-Smtp-Source: APiQypIe4ogztlxa2e28ZIgudhCuMYq2K6e46oFLZyKIpcZ+/yPLUJjapHaqeFXBXOq7QAVxgSVa2SOvw1Zn59OtjzM= X-Received: by 2002:a05:6602:2f87:: with SMTP id u7mr25750013iow.94.1586961610644; Wed, 15 Apr 2020 07:40:10 -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: Wed, 15 Apr 2020 20:09:54 +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 Wed, Apr 15, 2020 at 6:56 PM David Marchand wrote: > > Thanks Jerin for this new version. > New round of comments. Thanks for the review. > > - 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; 2) rte_trace_fp_is_enabled() Used in both the header file. i.e rte_tracepoint.h needed to include rte_trace.h. If that's is the case, I don't see any value. # 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. > > > - 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. > > > - What is the value of having a _public_ rte_trace_is_invalid() ? > A final user would need to lookup by name to get a trace descriptor > and we should hope that such a lookup returns a valid descriptor :-). > A developer would already have the descriptor hook point in his own > code: by construction, if the tracepoint is registered, then the > descriptor is valid, else, it is unknown. Yep. I will make rte_trace_is_invalid as internal trace_is_invalid() API. > > > - 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. > > - I can see no protection on the tracepoint list. Could we have issues > with control/application threads that dpdk does not control, dynamic > loading of libraries.. ? Based on my understanding, all constructors in the .so file should be called in eal_plugins_init() one by one. So there is no synchronization issue. eal_trace_init() which is called after eal_plugins_init(). So all the .so files with constructor should be called in eal_plugins_init() be it DPDK shared lib or non DPDK shared lib. rte_log_register() does the similar thing. > > > - Following comment on v4 and the removal of the mode per tracepoint > api, don't we need to put the current select mode in each tracepoint > descriptor when registering a trace point ? RTE_TRACE_POINT_REGISTER has only trace and name. RTE_TRACE_POINT_REGISTER(trace, name) Not sure why we need to add "mode" in the trace register. Reason for adding in the descriptor we discussed last time. --------------------------------------------------------------- > 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); Yes, you can keep the information in the tracepoint descriptor and expose a single api that impacts all tracepoints. +1 for me. -------------------------------------------------------------------- > > > -- > David Marchand >