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 B9C09A058A; Fri, 17 Apr 2020 10:28:00 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 907C21DE88; Fri, 17 Apr 2020 10:28:00 +0200 (CEST) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by dpdk.org (Postfix) with ESMTP id 9A9C61DE37 for ; Fri, 17 Apr 2020 10:27:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587112078; 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=PapRJpbbCKIJHW+Eg+IVvr/Tw+QxgG6StogcSbWKKCA=; b=XvnJBKjawOKcatlNIhIqE3uM1npdFtdp+DrHgjm1lhgfjY5oO7G8bLv6n5q9E/x9Y7eylI pSRCUkg8gwhfosFwz1JcGaO9loWSryTvI9jEIixYmLiWRik7Zx3Y2NN0kcEQNf8DA9PB+8 Jy6BqEx7juvNkTqYbzSjr8WxD6xlurM= Received: from mail-ua1-f71.google.com (mail-ua1-f71.google.com [209.85.222.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-46-QH0l4JySMA6IfUhcy-D19g-1; Fri, 17 Apr 2020 04:27:56 -0400 X-MC-Unique: QH0l4JySMA6IfUhcy-D19g-1 Received: by mail-ua1-f71.google.com with SMTP id v27so606402uaa.22 for ; Fri, 17 Apr 2020 01:27:56 -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=UDk27nZ+JoIuo86q6z8MehqvqX9gOK3SyvBnKqs+/3Y=; b=GQaQZcngY4VR+Nk5q2zKufcx9PWkYYkzvaKJkIOwTcGD7JUd3sl2V0r3OYIo9eBBg6 OnmDJVH3svNkJdZyqJlvFL9Iqllzu13RyY5VomBdo+ldAeIHLoaoxS0gB4uH196Drkjn f+0/HYLZRzPqDPoApgQj5V977aSpOCD0tVrezGsagoabYU5TQ5a6joHbp3d6d5F+VCyl rwF965OzC3voCiNrv084+0i4+PUa8Pz7G4pAHmoyXqRHdMdd8MUpW//TM/uIQVWYzLCI VjmK+AVA+PqK8GRL+/ZBrKCMSzRq1+MVxbF/q9Ltj95oavMGYVAOry5S5E3pV9D7gcFR mv/Q== X-Gm-Message-State: AGi0PubYdoEHOs7eX/1atlahVlwOZubP3vCBIhYSxM9UokKIRL14O8ZJ qlHgNXNzrwIxq5xVWMf7Pf9xCi2UwLA/VA/Nn74YTsk7spYAKnLmoWTEuZTm4zbcqgwHBLw3Xp/ SCeSx1aa63ZoGynNW7Zk= X-Received: by 2002:a1f:aa4f:: with SMTP id t76mr1560593vke.18.1587112075821; Fri, 17 Apr 2020 01:27:55 -0700 (PDT) X-Google-Smtp-Source: APiQypK0X7pmK3xyE8K4lJdQk5DS699Ax709zIfXC8Dw6kyNM08gh7jcY45gklXCsWXGN6OreWyZJ2Rt0lpDDRWirJI= X-Received: by 2002:a1f:aa4f:: with SMTP id t76mr1560569vke.18.1587112075330; Fri, 17 Apr 2020 01:27:55 -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: Fri, 17 Apr 2020 10:27:41 +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" Hello Jerin, 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_. > 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. > 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 definitio= n. 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). 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. > > > # 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 r= c), > > 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. If you don't like it, let's drop this last part. Thanks. --=20 David Marchand