From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751168AbdKFA2U (ORCPT ); Sun, 5 Nov 2017 19:28:20 -0500 Received: from mga14.intel.com ([192.55.52.115]:3605 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750853AbdKFA2T (ORCPT ); Sun, 5 Nov 2017 19:28:19 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,350,1505804400"; d="scan'208";a="332428063" Subject: Re: [PATCH v5 2/6] perf record: Get the first sample time and last sample time To: Jiri Olsa , Arnaldo Carvalho de Melo Cc: jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org, ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com References: <1508542074-29483-1-git-send-email-yao.jin@linux.intel.com> <1508542074-29483-3-git-send-email-yao.jin@linux.intel.com> <20171023150436.GA10746@krava> <818cd5bf-fd40-444e-4f4d-c9e25cf90b51@linux.intel.com> <20171024071659.GB27972@krava> <20171103162942.GF3531@kernel.org> <20171104102421.GA7511@krava> From: "Jin, Yao" Message-ID: <640d3119-c695-d975-ce59-d986172b48c4@linux.intel.com> Date: Mon, 6 Nov 2017 08:28:16 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171104102421.GA7511@krava> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/4/2017 6:24 PM, Jiri Olsa wrote: > 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 > It's a good idea. Default enabling --timestamps in perf record since buildids is enabled by default as well. But if buildids is not enabled, then it needs to check if --timestamps is enabled. I will follow this rule in v6. Thanks Jin Yao >> >> 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 >>> >>> thanks, >>> jirka