Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <006be03c-43da-6482-6bc4-83fe65dcd706@arm.com>
Date:   Fri, 9 Jun 2023 10:00:09 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V11 06/10] arm64/perf: Enable branch stack events via
 FEAT_BRBE
Content-Language: en-US
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        will@kernel.org, catalin.marinas@arm.com,
        Mark Brown <broonie@kernel.org>,
        James Clark <james.clark@arm.com>,
        Rob Herring <robh@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Suzuki Poulose <suzuki.poulose@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        linux-perf-users@vger.kernel.org
References: <20230531040428.501523-1-anshuman.khandual@arm.com>
 <20230531040428.501523-7-anshuman.khandual@arm.com>
 <ZH3mhorKNo77hsv5@FVFF77S0Q05N>
From:   Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <ZH3mhorKNo77hsv5@FVFF77S0Q05N>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/5/23 19:13, Mark Rutland wrote:
>> +static u64 branch_type_to_brbcr(int branch_type)
>> +{
>> +	u64 brbcr = BRBCR_EL1_DEFAULT_TS;
>> +
>> +	/*
>> +	 * BRBE need not be paused on PMU interrupt while tracing only
>> +	 * the user space, bcause it will automatically be inside the
>> +	 * prohibited region. But even after PMU overflow occurs, the
>> +	 * interrupt could still take much more cycles, before it can
>> +	 * be taken and by that time BRBE will have been overwritten.
>> +	 * Let's enable pause on PMU interrupt mechanism even for user
>> +	 * only traces.
>> +	 */
>> +	brbcr |= BRBCR_EL1_FZP;
> I think this is trying to say that we *should* use FZP when sampling the
> kernel (due to IRQ latency), and *can* safely use it when sampling userspace,
> so it would be good to explain it that way around.

Agreed, following updated comment explains why we should enable FZP
when sampling kernel, otherwise BRBE will capture unwanted records.
It also explains why we should enable FZP even when sampling user
space due to IRQ latency.

        /*
         * BRBE should be paused on PMU interrupt while tracing kernel
         * space to stop capturing further branch records. Otherwise
         * interrupt handler branch records might get into the samples
         * which is not desired.
         *
         * BRBE need not be paused on PMU interrupt while tracing only
         * the user space, because it will automatically be inside the
         * prohibited region. But even after PMU overflow occurs, the
         * interrupt could still take much more cycles, before it can
         * be taken and by that time BRBE will have been overwritten.
         * Hence enable pause on PMU interrupt mechanism even for user
         * only traces as well.
         */
        brbcr |= BRBCR_EL1_FZP;

> 
> It's a bit unfortunate, because where this matters we'll always be losing some
> branches either way, but I guess we don't have much say in the matter.