DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerinjacobk@gmail.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: David Marchand <david.marchand@redhat.com>,
	dpdk-dev <dev@dpdk.org>,  Jerin Jacob <jerinj@marvell.com>,
	Sunil Kumar Kori <skori@marvell.com>,
	 John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	 Declan Doherty <declan.doherty@intel.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	 Andrew Rybchenko <arybchenko@solarflare.com>,
	Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH 2/8] trace: simplify trace point registration
Date: Tue, 5 May 2020 09:13:22 +0530
Message-ID: <CALBAE1MePBx5j56Z8-TbjP2RzzDWRZfFXjTrd1Z-Ydtu+WvQ-w@mail.gmail.com> (raw)
In-Reply-To: <7788528.NyiUUSuA9g@thomas>

On Tue, May 5, 2020 at 3:01 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 04/05/2020 19:54, Jerin Jacob:
> > On Mon, May 4, 2020 at 11:10 PM David Marchand
> > > On Mon, May 4, 2020 at 7:19 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > On Mon, May 4, 2020 at 10:38 PM David Marchand
> > > > > On Mon, May 4, 2020 at 4:39 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > On Mon, May 4, 2020 at 7:34 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > On Mon, May 4, 2020 at 4:47 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > > On Mon, May 4, 2020 at 2:02 AM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > RTE_TRACE_POINT_DEFINE and RTE_TRACE_POINT_REGISTER must come in pairs.
> > > > > > > > > Merge them and let RTE_TRACE_POINT_REGISTER handle the constructor part.
> > > > > > > >
> > > > > > > >
> > > > > > > > Initially, I thought of doing the same. But, later I realized that
> > > > > > > > this largely grows the number of constructors been called.
> > > > > > > > I had concerns about the boot time of the application and/or loading
> > > > > > > > the shared library, that the reason why spitting
> > > > > > > > as two so that constructor registers a burst of traces like rte_log.
> > > > > > >
> > > > > > > I am a bit skeptical.
> > > > > > > In terms of cycles and looking at __rte_trace_point_register() (which
> > > > > > > calls malloc), the cost of calling multiple constructors instead of
> > > > > > > one is negligible.
> > > > > >
> > > > > > We will have a lot tracepoints latter, each one translates to the
> > > > > > constructor may not be a good
> > > > > > improvement. The scope is limited only to register function so IMO it
> > > > > > is okay to have split
> > > > > > just like rte_log. I don't see any reason why it has to be a different
> > > > > > than rte_log.
> > > > >
> > > > > What is similar to rte_log?
> > > > > There is neither RTE_LOG_REGISTER macro, nor two-steps declaration of
> > > > > dynamic logtypes.
> > > >
> > > >
> > > > Here is an example of rte_log registration. Which has _one_
> > > > constructor and N number of
> > > > rte_log_register() underneath.
> > >
> > > rte_log is one thing, rte_trace is already different.
> > >
> > > There is _no macro_ in rte_log for registration.
> > > The reason being in that a rte_log logtype is a simple integer without
> > > any special declaration requiring a macro.
> >
> > I just wrapped in macro for convincing, but it has the same semantics.
> > global variable and API/macro to register.
> >
> >
> > >
> > > For tracepoints, we have a special two steps thing: the tracepoint
> > > handle must be derived from the tracepoint name.
> > > Then this handle must be registered.
> > > What I proposed is to make life easier for developers that want to add
> > > tracepoints and I suppose you noticed patch 1 of this series.
> >
> > To reduce the constructors. I don't want trace libraries to add lot of
> > constructors.
> > I don't think it simplifies a lot as the scope of only for registration.
> >
> >
> > >
> > >
> > > > > > One of the thought process is, we probably remove the constructor
> > > > > > scheme to all other with DPDK
> > > > > > and replace it with a more register scheme. In such a case, we can
> > > > > > skip calling the constructor all tother
> > > > > > when trace is disabled.
> > > > >
> > > > > Sorry, but I have a hard time understanding your point.
> > > > > Are you talking about application boot time?
> > > >
> > > > Yes. The optimization of application boottime time in case of static
> > > > binary and/or shared library(.so) load time.
> > >
> > > As Thomas mentioned, do you have numbers?
> >
> > No. But I know, it is obvious that current code is better in terms of
> > boot time than the proposed one.
> > I would like to not add a lot of constructor for trace as the FIRST
> > module in DPDK.
>
> No, it is not obvious.
> The version from David looks simpler to use and to understand.
> Without any number, I consider usability (and maintenance) wins.

Thanks for the feedback.

As the trace maintainer, I would like not to explode constructor usage
for trace library.
My reasoning, We could do trace registration without this constructor scheme.

If you think, it is better usability, lets add an option for rte_log
for the constructor
scheme. Analyze the impact wrt boot time and cross-platform pov as the log
has a lot of entries to test. If the usage makes sense then it should make sense
for rte_log too. I would like to NOT have trace to be the first
library to explode
with the constructor scheme. I suggest removing this specific patch from RC2 and
revisit later.


>
>

  reply	other threads:[~2020-05-05  3:43 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-03 20:31 [dpdk-dev] [PATCH 0/8] Traces cleanup for rc2 David Marchand
2020-05-03 20:31 ` [dpdk-dev] [PATCH 1/8] cryptodev: fix trace points registration David Marchand
2020-05-04  7:41   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-05-03 20:31 ` [dpdk-dev] [PATCH 2/8] trace: simplify trace point registration David Marchand
2020-05-04  2:46   ` Jerin Jacob
2020-05-04 14:02     ` Thomas Monjalon
2020-05-04 14:04     ` David Marchand
2020-05-04 14:39       ` Jerin Jacob
2020-05-04 17:08         ` David Marchand
2020-05-04 17:19           ` Jerin Jacob
2020-05-04 17:40             ` David Marchand
2020-05-04 17:54               ` Jerin Jacob
2020-05-04 21:31                 ` Thomas Monjalon
2020-05-05  3:43                   ` Jerin Jacob [this message]
2020-05-05  7:01                     ` Thomas Monjalon
2020-05-05  7:17                       ` Jerin Jacob
2020-05-05  7:24                         ` Thomas Monjalon
2020-05-05  7:33                           ` Jerin Jacob
2020-05-05  8:23                             ` David Marchand
2020-05-05 10:12                               ` Jerin Jacob
2020-05-05 10:26                                 ` Thomas Monjalon
2020-05-05 10:46                                   ` Jerin Jacob
2020-05-05 11:48                                     ` Olivier Matz
2020-05-05 11:35                                 ` David Marchand
2020-05-05 12:26                                   ` Jerin Jacob
2020-05-05 15:25                                     ` Jerin Jacob
2020-05-05 16:28                                       ` David Marchand
2020-05-05 16:46                                         ` Jerin Jacob
2020-05-05 16:58                                           ` Thomas Monjalon
2020-05-05 17:08                                             ` Jerin Jacob
2020-05-05 17:09                                               ` Jerin Jacob
2020-05-05 17:20                                                 ` Thomas Monjalon
2020-05-05 17:28                                                   ` Jerin Jacob
2020-05-05 20:10                                                     ` Thomas Monjalon
2020-05-06  6:11                                                       ` Jerin Jacob
2020-07-04 14:31   ` Jerin Jacob
2020-07-04 15:14   ` [dpdk-dev] [PATCH v2] " David Marchand
2020-07-05 19:41     ` Thomas Monjalon
2020-05-03 20:31 ` [dpdk-dev] [PATCH 3/8] trace: simplify trace point headers David Marchand
2020-05-04  6:12   ` Jerin Jacob
2020-05-03 20:31 ` [dpdk-dev] [PATCH 4/8] trace: avoid confusion on optarg David Marchand
2020-05-04  7:55   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-05-04 14:09     ` David Marchand
2020-05-05  5:45       ` Sunil Kumar Kori
2020-05-05  5:47         ` Sunil Kumar Kori
2020-05-03 20:31 ` [dpdk-dev] [PATCH 5/8] trace: remove unneeded checks in internal API David Marchand
2020-05-04  8:16   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-05-03 20:31 ` [dpdk-dev] [PATCH 6/8] trace: remove limitation on patterns number David Marchand
2020-05-04  8:48   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-05-04 14:14     ` David Marchand
2020-05-05  5:54       ` Sunil Kumar Kori
2020-05-03 20:31 ` [dpdk-dev] [PATCH 7/8] trace: remove string duplication David Marchand
2020-05-04  9:01   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-05-03 20:31 ` [dpdk-dev] [PATCH 8/8] trace: fix build with gcc 10 David Marchand
2020-05-06 13:06 ` [dpdk-dev] [PATCH 0/8] Traces cleanup for rc2 David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALBAE1MePBx5j56Z8-TbjP2RzzDWRZfFXjTrd1Z-Ydtu+WvQ-w@mail.gmail.com \
    --to=jerinjacobk@gmail.com \
    --cc=arybchenko@solarflare.com \
    --cc=david.marchand@redhat.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=olivier.matz@6wind.com \
    --cc=skori@marvell.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git