DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Jerin Jacob <jerinjacobk@gmail.com>
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, 05 May 2020 22:10:23 +0200
Message-ID: <4414218.rnE6jSC6OK@thomas> (raw)
In-Reply-To: <CALBAE1Oy0SUnbZ4-LF6PmqWFBKs1h=9QpEyTfGRNxhfCJwqMag@mail.gmail.com>

05/05/2020 19:28, Jerin Jacob:
> On Tue, May 5, 2020 at 10:50 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 05/05/2020 19:09, Jerin Jacob:
> > > On Tue, May 5, 2020 at 10:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > On Tue, May 5, 2020 at 10:28 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > > 05/05/2020 18:46, Jerin Jacob:
> > > > > > On Tue, May 5, 2020 at 9:58 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > On Tue, May 5, 2020 at 5:25 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > > On Tue, May 5, 2020 at 5:56 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > > > On Tue, May 5, 2020 at 5:06 PM David Marchand <david.marchand@redhat.com> wrote:
> > > > > > > > > > On Tue, May 5, 2020 at 12:13 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > > > > > > > > > Please share the data.
> > > > > > > > > > > >
> > > > > > > > > > > > Measured time between first rte_trace_point_register and last one with
> > > > > > > > > > > > a simple patch:
> > > > > > > > > > >
> > > > > > > > > > > I will try to reproduce this, once we finalize on the above synergy
> > > > > > > > > > > with rte_log.
> > > > > > > > > >
> > > > > > > > > > I took the time to provide measure but you won't take the time to look at this.
> > > > > > > > >
> > > > > > > > > I will spend time on this. I would like to test with a shared library
> > > > > > > > > also and more tracepoints.
> > > > > > > > > I was looking for an agreement on using the constructor for rte_log as
> > > > > > > > > well(Just make sure the direction is correct).
> > > > > > > > >
> > > > > > > > > Next steps:
> > > > > > > > > - I will analyze the come back on this overhead on this thread.
> > > > > > > >
> > > > > > > > I have added 500 constructors for testing the overhead with the shared
> > > > > > > > build and static build.
> > > > > > > > My results inline with your results aka negligible overhead.
> > > > > > > >
> > > > > > > > David,
> > > > > > > > Do you have plan for similar RTE_LOG_REGISTER as mentioned earlier?
> > > > > > > > I would like to have rte_log and rte_trace semantics similar to registration.
> > > > > > > > If you are not planning to submit the rte_log patch then I can send
> > > > > > > > one for RC2 cleanup.
> > > > > > >
> > > > > > > It won't be possible for me.
> > > > > >
> > > > > > I can do that if we agree on the specifics.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Relying on the current rte_log_register is buggy with shared builds,
> > > > > > > as drivers are calling rte_log_register, then impose a default level
> > > > > > > without caring about what the user passed.
> > > > > > > So if we introduce a RTE_LOG_REGISTER macro now at least this must be fixed too.
> > > > > > >
> > > > > > > What I wanted to do:
> > > > > > > - merge rte_log_register_and_pick_level() (experimental) into
> > > > > > > rte_log_register, doing this should be fine from my pov,
> > > > > > > - reconsider the relevance of a fallback logtype when registration fails,
> > > > > > > - shoot the default level per component thing: levels meaning is
> > > > > > > fragmented across the drivers/libraries because of it, but this will
> > > > > > > open a big box of stuff,
> > > > > >
> > > > > > This you are referring to internal implementation improvement. Right?
> > > > > > I was referring to remove the current clutter[1]
> > > > > > If we stick the following as the interface. Then you can do other
> > > > > > improvements when you get time
> > > > > > that won't change the consumer code or interference part.
> > > > > >
> > > > > > #define RTE_LOG_REGISTER(type, name, level)
> > > > >
> > > > > This discussion is interesting but out of scope for rte_trace.
> > > > > I am also interested in rte_log registration cleanup,
> > > > > but I know it is too much work for the last weeks of 20.05.
> > > > >
> > > > > As Olivier said about rte_trace,
> > > > > "Since it's a new API, it makes sense to make
> > > > > it as good as possible for the first version."
> > > > >
> > > > > So please let's conclude on this rte_trace patch for 20.05-rc2,
> > > > > and commit to fix rte_log registration in the first days of 20.08.
> > > >
> > > > Why not hold the trace registration patch 2/8 and apply rest for RC2.
> > > > Once we have synergy between the registration scheme between rte_log
> > > > and rte_trace
> > > > apply the patch for RC2.
> > >
> > > I meant, Once we have synergy between the registration scheme between
> > > rte_log and rte_trace
> > > apply the patch for _20.08_?
> >
> > Because of what I wrote above:
> > As Olivier said about rte_trace,
> > "Since it's a new API, it makes sense to make
> > it as good as possible for the first version."
> >
> > The intent is to show an API as simple as possible
> > in order to have a maximum of developers integrating it,
> > and getting more interesting feedbacks.
> >
> > In other words, we want to make your work shine for prime time.
> 
> I like that, If it is not shining just because of 2/8 not applying now
> then I fine with that.
> Anyway, it is an experimental API, There is still room to change and
> nothing is set and stone.
> For me, the synergy between log/trace interface important as trace
> needs to replace rte_log.

Now that I better understand what rte_trace (and tracing in general) is,
I believe rte_log cannot be replaced.
I think we can write logs in traces, as a log option, but it should be
just one possible output among others.

I think everybody agree to use one constructor per log type and
per trace type. We are ready to do this change for rte_trace first.
This is your call to accept it or not, even if don't understand
why you would like both to be done at the exact same time.



  reply	other threads:[~2020-05-05 20:10 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
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 [this message]
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=4414218.rnE6jSC6OK@thomas \
    --to=thomas@monjalon.net \
    --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=jerinjacobk@gmail.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=olivier.matz@6wind.com \
    --cc=skori@marvell.com \
    /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