linux-kernel - Re: [PATCH v5 2/6] perf record: Get the first sample time and last sample time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171103162942.GF3531@kernel.org>
Date:   Fri, 3 Nov 2017 13:29:42 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     "Jin, Yao" <yao.jin@...ux.intel.com>, jolsa@...nel.org,
        peterz@...radead.org, mingo@...hat.com,
        alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH v5 2/6] perf record: Get the first sample time and last
 sample time

Em Tue, Oct 24, 2017 at 09:16:59AM +0200, Jiri Olsa escreveu:
> On Tue, Oct 24, 2017 at 10:03:05AM +0800, Jin, Yao wrote:
> 
> SNIP
> 
> > > hum, could you still unset the sample if there's no time given?
> > > and keep the speed in this case..
> > > 
> > > jirka
> > > 
> > 
> > Hi Jiri,
> > 
> > I check this question again. The '--time' option is for perf report but not
> > for perf record.
> > 
> > For perf record, we have to always walk on all samples to get the time of
> > first sample and the time of last sample whatever buildid_all is enabled or
> > not enabled. So 'rec->tool.sample = NULL' is removed.
> > 
> > Sorry, the previous mail was replied at midnight, I was drowsy. :(
> > 
> > If my answer is correct, I will not send v6. If my understanding is still
> > not correct, please let me know.
> 
> right, I did not realize we store this unconditionaly.. then yes, it's ok

And should we store this unconditionally? What this patch is doing is
making 'perf record' unconditionally slower so that the generated
perf.data file becomes useful for some usecases, but not for all, only
people interested in using 'perf report/script --time' will benefit,
right?

I thought that we could get this sorted out in a different fashion, i.e.
getting the first timestamp is easy, even if we don't process build-ids,
right? To get the last one we could ask the kernel to insert an extra
dummy sample at the end, one that we know the size and thus can to to
the end of the file, rewind that size, get the event and parse the
sample, agreed?

So I suggest that first make this conditional, i.e. 'perf record
--timestamps' will enable the logic you implemented, and as a followup,
if you agree, add the dummy, known size event at the end, and then even
when build-ids are not processed, the cost for getting the timestamps
will be next to zero.

- Arnaldo

- Arnaldo
 
> I think I've already acked this, anyway for the patchset:
> 
> Acked-by: Jiri Olsa <jolsa@...nel.org>
> 
> thanks,
> jirka