From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org, Andi Kleen <ak@linux.intel.com>,
Andi Kleen <andi@firstfloor.org>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Stephane Eranian <eranian@google.com>,
Thomas Gleixner <tglx@linutronix.de>,
Jiri Olsa <jolsa@redhat.com>
Subject: Re: [PATCH 05/33] perf record: Allow the user to disable time stamps
Date: Tue, 5 Aug 2014 10:33:47 -0300 [thread overview]
Message-ID: <20140805133347.GH13375@kernel.org> (raw)
In-Reply-To: <20140805060856.GA9453@gmail.com>
Em Tue, Aug 05, 2014 at 08:08:56AM +0200, Ingo Molnar escreveu:
> * Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> This patch looks dangerous and misleading to me.
I took it more from the angle: hey, it fixes a regression, i.e.
-T/--time somehow became a noop, and also it doesn't change any default.
It also states that you will get misaccounting, why that would happen,
althought wording could've been better, as mmaps won't be "missing",
i.e. they will be there as this governs just PERF_SAMPLE_TIME, not
perf_event_attr.mmap, so it MMAPs may come after samples for its maps.
So, I took it.
How should we proceed?
- Arnaldo
> > Time stamps are always implicitely enabled for record currently. The
> > old --time/-T option is a nop.
> >
> > Allow the user to disable timestamps by using --no-time
> >
> > This can cause some minor misaccounting (by missing mmaps), but
> > significantly lowers the size of perf.data
>
> I'm not any big change in size:
>
> -rw------- 1 mingo mingo 384768 Aug 5 08:01 perf.data.timestamps
> -rw------- 1 mingo mingo 336952 Aug 5 08:00 perf.data.notimestamps
>
> That's a ~14% reduction - it's certainly noticeable but not
> earth-shattering.
I wonder if this is for things like intel PT or some other super high
volume scenario where 14% indeed looks like huge, but then not using
PERF_SAMPLE_TIME there may have implications, but then, this is one of
those use-only-if-you-know-what-are-the-implications knob, where the
default is the sane one.
> And what is called 'minor misaccounting' in the description is
> actually a potentially broken profile that people might rely on.
>
> So the patch description misleads twice: by exaggerating the
> advantages and also by downplaying the disadvantages.
>
> So either remove the --time option altogether, or fix its
> 'misaccounting' so that the profile can be relied on.
>
> Thanks,
>
> Ingo
next prev parent reply other threads:[~2014-08-05 13:33 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-04 16:17 [GIT PULL 00/33] perf/core improvements and fixes Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 01/33] perf tools: Fix arm64 build error Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 02/33] perf evlist: Don't run workload if not told to Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 03/33] perf symbols: Make sure --symfs usage includes the path separator Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 04/33] perf kvm stat: Properly show submicrosecond times Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 05/33] perf record: Allow the user to disable time stamps Arnaldo Carvalho de Melo
2014-08-05 6:08 ` Ingo Molnar
2014-08-05 13:33 ` Arnaldo Carvalho de Melo [this message]
2014-08-12 14:58 ` Ingo Molnar
2014-08-12 15:29 ` Arnaldo Carvalho de Melo
2014-08-13 5:08 ` Ingo Molnar
2014-08-05 14:17 ` Andi Kleen
2014-08-05 14:31 ` Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 06/33] perf tools: Rename ordered_samples bool to ordered_events Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 07/33] perf tools: Rename ordered_samples struct " Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 08/33] perf tools: Rename ordered_events members Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 09/33] perf tools: Add ordered_events__(new|delete) interface Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 10/33] perf tools: Factor ordered_events__flush to be more generic Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 11/33] perf tools: Limit ordered events queue size Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 12/33] perf tools: Flush ordered events in case of allocation failure Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 13/33] perf tools: Make perf_session__deliver_event global Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 14/33] perf tools: Create ordered-events object Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 15/33] perf tools: Use list_move in ordered_events_delete function Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 16/33] perf tools: Add ordered_events__init function Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 17/33] perf tools: Add ordered_events__free function Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 18/33] perf tools: Add perf_config_u64 function Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 19/33] perf tools: Add report.queue-size config file option Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 20/33] perf tools: Add debug prints for ordered events queue Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 21/33] perf tools: Allow out of order messages in forced flush Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 22/33] perf tools: Show better error message in case we fail to open counters due to EBUSY error Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 23/33] perf kmem: Do not ignore mmap events Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 24/33] perf tools: Fix make PYTHON override Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 25/33] perf tools: Left-align output contents Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 26/33] perf tools: Make __hpp__fmt() receive an additional len argument Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 27/33] perf tools: Save column length in perf_hpp_fmt Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 28/33] perf report: Honor column width setting Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 29/33] perf top: Add -w option for setting column width Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 30/33] perf tools: Add name field into perf_hpp_fmt Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 31/33] perf tools: Fix column alignment when headers aren't shown on TUI Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 32/33] perf tools: Fix PERF_FLAG_FD_CLOEXEC flag probing event type open counters due to EBUSY error Arnaldo Carvalho de Melo
2014-08-04 16:17 ` [PATCH 33/33] perf tools: Default to python version 2 Arnaldo Carvalho de Melo
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=20140805133347.GH13375@kernel.org \
--to=acme@kernel.org \
--cc=a.p.zijlstra@chello.nl \
--cc=ak@linux.intel.com \
--cc=andi@firstfloor.org \
--cc=eranian@google.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=tglx@linutronix.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).