From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 9875CA0588;
	Thu, 16 Apr 2020 18:27:12 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 7C5921DD51;
	Thu, 16 Apr 2020 18:27:12 +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 38A771DD45
 for <dev@dpdk.org>; Thu, 16 Apr 2020 18:27:11 +0200 (CEST)
Received: by mail-io1-f66.google.com with SMTP id e127so6338917iof.6
 for <dev@dpdk.org>; Thu, 16 Apr 2020 09:27: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=muF4fb6fItYhNTiNmZkTghfkWA9Kzc4dgWj0Kl8fasg=;
 b=W6/z3AD7/Oeiy05XPwkLr/X9lBPcSdGSiYRturxTswt53BAbim/nEYGjciwBL/G0Cq
 N8opVwe8HV4m9TutotNNmh5epS17NuMzdExyODzjP3tfhQ0XtYqPBInSTKnCzMVLCSd3
 WZu6/ZIk2pA7/9q5aU9U6yEyxeItDItCa0RY8UFjL2XrAaQhsRpLdERpP7RMcTLLsAWR
 PWQINxMxGrlxnan45GRLab6egNkzBxpJOMG/xRDzZnq4T2Yz9WuEEse3XZ16z2ecyQPn
 6sQNY9dCxFZz4n4akgInc96EIV3n9gqztRJnuDgxw7KCGIerctyVYsTEM4XVT+qsSUnz
 Eo0w==
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=muF4fb6fItYhNTiNmZkTghfkWA9Kzc4dgWj0Kl8fasg=;
 b=gwaQ49H9qvh3N34YMURcyKEaLYBaOpmcTaevKOYE6w7md28A1tNWlijNJBFKMW1SQn
 dKXk8mcnlQD/yPhJTGbz6E/8uFy3tdEXJb/KvLqGhXXse9MFydRuPpnFWZO3NvTfD+Nh
 wDswtSSo3zkM0pj2sNT+i7VhbGax6xVaq4iDec6JsF7czzShn1loq61rqi/lRRpJCTe0
 7LHfQxqoCgnHdD0wwVBwC9Z3WpnSvZIWtOt6Y1ACq/SBtBenGo/B5/7OMZe082Qw5EMa
 D80I4gNy357xcELNW3X34qxbroiBwsq3FK7w4/HNFFIKYOVUgJfTbLShjrTDK+Y5shvR
 0aBQ==
X-Gm-Message-State: AGi0PuaE9zjVMKqAFzAgZNQIgsCvUSKWAZJ1ccTtKFMaWC1aj+59mBfg
 dxPrxBntb9+O3Ir784E42n01iKIGIGWp97Spmdc=
X-Google-Smtp-Source: APiQypKkMqCoFwnsgwpyYmEZyGprxFv4cALUZ5U+/Pz5SdQbyCUJhcRlhQhoOiYH14nZPSi0Irx275O3JUX7kOvwpho=
X-Received: by 2002:a05:6638:5ad:: with SMTP id
 b13mr24577904jar.113.1587054430339; 
 Thu, 16 Apr 2020 09:27:10 -0700 (PDT)
MIME-Version: 1.0
References: <20200403153709.3703448-1-jerinj@marvell.com>
 <CAJFAV8xepQX+tatdwwMWjmD-NyGfjoA+pXyjxf5JUMf_9PVQgQ@mail.gmail.com>
 <CALBAE1N4neDU0W5Da6FQBbiTzS+1tpyoDDmLPPKfgaFtqbTnVw@mail.gmail.com>
 <3598012.22IY78Rhhi@thomas>
In-Reply-To: <3598012.22IY78Rhhi@thomas>
From: Jerin Jacob <jerinjacobk@gmail.com>
Date: Thu, 16 Apr 2020 21:56:54 +0530
Message-ID: <CALBAE1PcM5E3urru=TDh1KOBQct-wi_Xcy=y8rKFOAiq8j60kA@mail.gmail.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: David Marchand <david.marchand@redhat.com>, 
 Jerin Jacob Kollanukkaran <jerinj@marvell.com>, dev <dev@dpdk.org>, 
 Bruce Richardson <bruce.richardson@intel.com>, 
 =?UTF-8?Q?Mattias_R=C3=B6nnblom?= <mattias.ronnblom@ericsson.com>, 
 Sunil Kumar Kori <skori@marvell.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On Thu, Apr 16, 2020 at 9:53 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 16/04/2020 18:08, Jerin Jacob:
> > On Thu, Apr 16, 2020 at 7:09 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob <jerinjacobk@gmail.com> 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.
>
> Semantically, rte_trace.h must be the API for simple users enabling traces,
> while rte_trace_point.h would be used by those adding traces.

Yes. The problem is where will be the definition of
rte_trace_fp_is_enabled() come?
It is used for both needs.


>
>
> > 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)));
>
> Is it possible to implement it with a common helper as David suggests?

It is already present.

See.

#define __rte_trace_emit_datatype(in, type)\
do {\
        RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in)));\
        __rte_trace_emit_ctf_field(sizeof(type), RTE_STR(in), RTE_STR(type));\
} while (0)

#define rte_trace_ctf_u64(in) __rte_trace_emit_datatype(in, uint64_t)
#define rte_trace_ctf_i64(in) __rte_trace_emit_datatype(in, int64_t)
#define rte_trace_ctf_u32(in) __rte_trace_emit_datatype(in, uint32_t)
#define rte_trace_ctf_i32(in) __rte_trace_emit_datatype(in, int32_t)
#define rte_trace_ctf_u16(in) __rte_trace_emit_datatype(in, uint16_t)
#define rte_trace_ctf_i16(in) __rte_trace_emit_datatype(in, int16_t)
#define rte_trace_ctf_u8(in) __rte_trace_emit_datatype(in, uint8_t)
#define rte_trace_ctf_i8(in) __rte_trace_emit_datatype(in, int8_t)
#define rte_trace_ctf_int(in) __rte_trace_emit_datatype(in, int32_t)
#define rte_trace_ctf_long(in) __rte_trace_emit_datatype(in, long)
#define rte_trace_ctf_float(in) __rte_trace_emit_datatype(in, float)
#define rte_trace_ctf_double(in) __rte_trace_emit_datatype(in, double)
#define rte_trace_ctf_ptr(in) __rte_trace_emit_datatype(in, uintptr_t)

>
>
>