Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <ffa56520-09b5-9c5d-7733-6767d2f8e350@intel.com>
Date:   Mon, 21 Mar 2022 15:09:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.5.0
Subject: Re: [PATCH] perf intel-pt: Synthesize cycle events
Content-Language: en-US
To:     "Steinar H. Gunderson" <sesse@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
References: <YiuKAk7SaXP7B7Ee@google.com>
 <ba2c49da-22c5-06ea-e953-82211b953ca8@intel.com>
 <YjBnet2813sEGjZY@google.com>
 <c50bb87d-9fee-c4f9-a350-8729e503e43a@intel.com>
 <YjDUO6bbyfGw/u0C@google.com>
 <52903e58-e74c-5ea0-36b4-277ea3610af4@intel.com>
 <YjGdoGy4Z2UUG9S9@google.com>
 <ef780335-7dc6-3a23-54a8-b6fc9c8a2ed3@intel.com>
 <YjHfGrZovk3N/H0f@google.com>
 <371faf0d-f794-4a2e-0a1c-9d454d7c8b12@intel.com>
 <YjhUjotmo+kYvoNP@google.com>
From:   Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <YjhUjotmo+kYvoNP@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 21.3.2022 12.33, Steinar H. Gunderson wrote:
> On Mon, Mar 21, 2022 at 11:16:56AM +0200, Adrian Hunter wrote:
>> I had another look at this and it seemed *mostly* OK for me.  One change
>> I would make is to subject the cycle period to the logic of the 'A' option
>> (approximate IPC).
>>
>> So what does the 'A' option do.
>>
>> By default, IPC is output only when the exact number of cycles and
>> instructions is known for the sample.  Decoding walks instructions
>> to reconstruct the control flow, so the exact number of instructions
>> is known, but the cycle count (CYC packet) is only produced with
>> another packet, so only indirect/async branches or the first
>> conditional branch of a TNT packet.
> 
> Ah, I hadn't thought of the fact that you only get the first branch per
> packet. It's a bit unfortunate for (exact) cycle counts, since I guess
> TNT packets can also easily cross functions?

Yes, it can cross calls and returns.  'returns' due to "Return Compression"
which can be switched off at record time with config term noretcomp, but
that may cause more overflows / trace data loss.

To get accurate times for a single function there is Intel PT
address filtering.

Otherwise LBRs can have cycle times.

> 
>> So the cycle sample function looks like this:
>>
>> static int intel_pt_synth_cycle_sample(struct intel_pt_queue *ptq)
>>
>> [...]
>>
>> With regard to the results you got with perf report, please try:
>>
>> 	perf report --itrace=y0nse --show-total-period --stdio
>>
>> and see if the percentages and cycle counts for rarely executed
>> functions make more sense.
> 
> I already run mostly with 0ns period, so I don't think that's it.
> I tried your new version, and it's very similar to your previous one;
> there are some small changes (largest is that one function goes from
> 2.5% to 2.2% or so), but the general gist of it is the same.
> I am increasingly leaning towards that my original version is wrong
> somehow, though.
> 
> By the way, I noticed that synthesized call stacks do not respect
> --inline; is that on purpose? The patch seems simple enough (just
> a call to add_inlines), although it exposes extreme slowness in libbfd
> when run over large binaries, which I'll have to look into.
> (10+ ms for each address-to-symbol lookup is rather expensive when you
> have 4M samples to churn through!)

No, not on purpose.