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: <20171104102421.GA7511@krava>
Date:   Sat, 4 Nov 2017 11:24:21 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
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

On Fri, Nov 03, 2017 at 01:29:42PM -0300, Arnaldo Carvalho de Melo wrote:
> 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?

maybe we can also silently enable that when processing buildids,
(which is set by default), there's no big performance hit once
we already go through samples

jirka

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