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 CF817A0588; Thu, 16 Apr 2020 15:39:47 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A527A1DCA8; Thu, 16 Apr 2020 15:39:46 +0200 (CEST) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by dpdk.org (Postfix) with ESMTP id 543EB1D933 for ; Thu, 16 Apr 2020 15:39:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587044384; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mGLZi7gCet9EdDAAVHPx2pvnE1aUnCYacCIUltet+rI=; b=DUMx2ditN+zgT05SFvVKvhEh8Mnhv13eJTK/4mBwck1AHOLB71gjvo2ug4JGKrFv81JVvc rmNOtR9p2EloYGFkqT87yAUyGAfF8dyrjABexWxZAWGRD3/deH8LfnrQ0954YgRZCUeBfH FMc0GybU6D/54LyN+93VbjwuMh6gSG8= Received: from mail-vk1-f199.google.com (mail-vk1-f199.google.com [209.85.221.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-295-QGd69wcFMyiUt9S6pqbPgw-1; Thu, 16 Apr 2020 09:39:43 -0400 X-MC-Unique: QGd69wcFMyiUt9S6pqbPgw-1 Received: by mail-vk1-f199.google.com with SMTP id q190so1412740vka.14 for ; Thu, 16 Apr 2020 06:39:43 -0700 (PDT) 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=zWnC2ayfuzYw0XPaEl+vAWBiAxm1JJZlbKNGEgzGBNY=; b=tM0WhGVCg7tHc2YQ4sDJMqM5yl7XOKUKB0flcR3dBv3ae63rxTuCfRsebp/9xyooes Hhy+xHFc0pgsc56ufKxB5RcnAXuwJZxABtutZrFJDCspC0+5dltWhHDtOAOWQGyuao4e e+qk1uzvcPg4ktqklFFB+MZwgb66RtanlW1Xj5wYDX/CZYGLomuaZNU022ct7pc+/HeY ATQ4AC7e2QLrVq8dC3tK2FW9C/WSLLCav/1HVeOBOkKfm0+zHMGwPXUXYM+9/PAJVArh EGicrJWVHRcz6go2b9XCKp6eWBBQgnzhH6+UbBx1Oahx/Vyhl8j5VbJpxSOta48GB6w8 l2pA== X-Gm-Message-State: AGi0PubUgh31c0J/R4hreoYJlgl0fgfoYUsZ4+Qch3U3gM3wCZ8uY/1g QTvzVYXL4YHdL5TIS41nNuxgTSWSgrMqUAaCv/8kJS6VOJiD5N6auvZyjAgeOSdy95K3BqYkGT3 1sftlQ6qNNrlBrm04GVY= X-Received: by 2002:ab0:485:: with SMTP id 5mr8715763uaw.86.1587044382393; Thu, 16 Apr 2020 06:39:42 -0700 (PDT) X-Google-Smtp-Source: APiQypKbQllBEQULmsmKzBBJ8e9/SVOGnVjP0rrw63n9T12X65LuoWwAHm/V+Ftd4ej4e8mZ8bNrgPIDQpzGvebgM4w= X-Received: by 2002:ab0:485:: with SMTP id 5mr8715700uaw.86.1587044381612; Thu, 16 Apr 2020 06:39:41 -0700 (PDT) MIME-Version: 1.0 References: <20200403153709.3703448-1-jerinj@marvell.com> <20200413150116.734047-1-jerinj@marvell.com> In-Reply-To: From: David Marchand Date: Thu, 16 Apr 2020 15:39:29 +0200 Message-ID: To: Jerin Jacob Cc: Jerin Jacob Kollanukkaran , dev , Thomas Monjalon , Bruce Richardson , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Sunil Kumar Kori X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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... > > 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. > # 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. 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. > > - 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. > > - 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 var= iables > 2) Chances that same subsystem FP variables comes same fastpath cache lin= e. > In short, We will be more predictable performance number from build to bu= ild 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. > > - 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. As long as we register in constructor, this is ok. Time will tell us if users want to use this in a different way. > > - 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. I thought of registers happening out of a constructor. But as long as we are in constructors and thanks to the eal_trace_init() after plugin load, the mode is set in all descriptors. So we are fine. --=20 David Marchand