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 43ED5A0588;
	Thu, 16 Apr 2020 18:08:52 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 0CC051D914;
	Thu, 16 Apr 2020 18:08:52 +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 0269B1D904
 for <dev@dpdk.org>; Thu, 16 Apr 2020 18:08:50 +0200 (CEST)
Received: by mail-io1-f66.google.com with SMTP id e127so6274587iof.6
 for <dev@dpdk.org>; Thu, 16 Apr 2020 09:08:50 -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=Uyp+j+K+W42UhM98RJR+BiXmDy2mMqAldlXpwr4NMnQ=;
 b=QHCA9hSI/s0JVQp/om05D2atf40GfradYUk+VQxHkTo4PLQBc/ATAxLgFfI3jL4DY3
 tGOtFwUC+eAAGjbBPV6yJlZ54YhTgq26LsFsDCsQV6igLzEXv6Ot+EP3/0aVLi3gLmJ6
 2g+BtNlRo2mHZW3PD0F7QYWrDSO8RfcBc2qwmL7fgf8el1kaQya755A/0nTAPdY9VuCD
 bufXzsEgkaEOYEKqTOM8C5U8SAdYbu0ByAU0Z48aiZaSAsR7MrdIHWvh20GYYBe00el2
 4p/ZPgIjXwIynXXQnFYRnWz7Fy4teffPU6YtirmIOlfY1N9B73bSGeiPfyLeP3GYZ+pE
 3Hdg==
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=Uyp+j+K+W42UhM98RJR+BiXmDy2mMqAldlXpwr4NMnQ=;
 b=riIXZjR2Bh37lt1VEozpSKKq7tXrrjmIf/H2eHShPZUh+fXPNkrP9gQFWPYP64v8a8
 qp/XRBXxAX7ORS3D8aXp36eptPG23EA0In0KVVig3HusUCS+grYps4Pj6NJyV8aL5qMn
 t5f2GWt1ZDU6mpu7neKpJAGGuSDLNqpEdb1LLETuxVhBhCuCiMOHWGxlNnv69ZpQp5RV
 6qNKHLlD9BIQjn2oFsNQ35rCGOIexFPIEdqt1JETxNdrcM2qXABn75XqNJ20a02CDuUz
 YJJoSyibA/zZlS/VHhQxBm80cJh00HG7p3HdgsmbBhtErukSN9PS+DFG1J2nFIUdc19i
 MOqQ==
X-Gm-Message-State: AGi0PuYs8O6H5ZU6EpDy5No3NthP60GajtDQyEBA15bHQlFVB5BeCKt+
 dyKtRvXr2wxDOPPgu9M7zi47TmLZ8o6iYMusWOc=
X-Google-Smtp-Source: APiQypJXI24OoqNyczRL7AE77e1kgdz/9tIKSjZ0uhcISY8LW4oa2hZ8yWS8M41fmePGoQ3ihsFKsIIy01UND7Y6yU0=
X-Received: by 2002:a05:6602:2f87:: with SMTP id
 u7mr30922535iow.94.1587053329335; 
 Thu, 16 Apr 2020 09:08:49 -0700 (PDT)
MIME-Version: 1.0
References: <20200403153709.3703448-1-jerinj@marvell.com>
 <20200413150116.734047-1-jerinj@marvell.com>
 <CAJFAV8zkBf-aMV3_B9Ddt4s7NQTEPMSy_aFE1e5H3OtQU3F7fA@mail.gmail.com>
 <CALBAE1OFGN2N8k4jNtynzZrjK-VOngAU0ATi2fFYup2kkhN1rg@mail.gmail.com>
 <CAJFAV8xepQX+tatdwwMWjmD-NyGfjoA+pXyjxf5JUMf_9PVQgQ@mail.gmail.com>
In-Reply-To: <CAJFAV8xepQX+tatdwwMWjmD-NyGfjoA+pXyjxf5JUMf_9PVQgQ@mail.gmail.com>
From: Jerin Jacob <jerinjacobk@gmail.com>
Date: Thu, 16 Apr 2020 21:38:33 +0530
Message-ID: <CALBAE1N4neDU0W5Da6FQBbiTzS+1tpyoDDmLPPKfgaFtqbTnVw@mail.gmail.com>
To: David Marchand <david.marchand@redhat.com>
Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>, dev <dev@dpdk.org>, 
 Thomas Monjalon <thomas@monjalon.net>,
 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 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.

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)));


>
>
> > > - 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.

OK. I will remove is_disabled()


>
>
> > > - 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.
>
> Ok, thanks for the explanation.
> It is worth a few words in the associated commitlog for posterity.

OK. I add it in git commit.


>