[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <90f4abd0-2314-19e4-176c-00f15100ada0@arm.com>
Date: Mon, 28 Nov 2022 13:54:46 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Mark Rutland <mark.rutland@....com>
Cc: linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, peterz@...radead.org,
acme@...nel.org, will@...nel.org, catalin.marinas@....com,
Mark Brown <broonie@...nel.org>,
James Clark <james.clark@....com>,
Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
Suzuki Poulose <suzuki.poulose@....com>,
Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH V5 4/7] driver/perf/arm_pmu_platform: Add support for BRBE
attributes detection
On 11/21/22 17:09, Mark Rutland wrote:
> On Mon, Nov 21, 2022 at 12:06:31PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 11/18/22 23:31, Mark Rutland wrote:
>>> On Mon, Nov 07, 2022 at 11:55:11AM +0530, Anshuman Khandual wrote:
>>>> This adds arm pmu infrastrure to probe BRBE implementation's attributes via
>>>> driver exported callbacks later. The actual BRBE feature detection will be
>>>> added by the driver itself.
>>>>
>>>> CPU specific BRBE entries, cycle count, format support gets detected during
>>>> PMU init. This information gets saved in per-cpu struct pmu_hw_events which
>>>> later helps in operating BRBE during a perf event context.
>>>
>>> Do we expect this to vary between CPUs handled by the same struct arm_pmu ?
>>
>> BRBE registers are per CPU, and the spec does not assert about BRBE properties
>> being the same across the system, served via same the struct arm_pmu.
>
> The same is true of the PMU, and struct arm_pmu does not cover the whole
> system, it covers each *micro-architecture* within the system.
>
> I think BRBE should be treated the same, i.e. uniform *within* a struct
> arm_pmu.
Understood, detected on one and verified on all ?
>
>> Hence it would be inaccurate to make that assumption, which might have just
>> avoided all these IPI based probes during boot.
>
> FWIW, I would be happy to IPI all CPUs during boot to verify uniformity of CPUs
> within an arm_pmu; I just don't think that BRBE should be treated differently
> from the rest of the PMU features.
Hence BRBE probing should be done inside an updated __armv8pmu_probe_pmu().
static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu)
{
struct armv8pmu_probe_info probe = {
.pmu = cpu_pmu,
.present = false,
};
int ret;
ret = smp_call_function_any(&cpu_pmu->supported_cpus,
__armv8pmu_probe_pmu,
&probe, 1);
if (ret)
return ret;
return probe.present ? 0 : -ENODEV;
}
But if BRBE is assumed (and verified) to be same across the micro-architecture,
then following BRBE attributes when captured should be part of 'struct arm_pmu'
instead of 'struct pmu_hw_events' as is the case currently.
/* Detected BRBE attributes */
bool brbe_v1p1;
int brbe_cc;
int brbe_nr;
int brbe_format;
>
> [...]
>
>>>> + hw_events = per_cpu_ptr(armpmu->hw_events, smp_processor_id());
>>>> + armpmu->brbe_probe(hw_events);
>>>> +}
>>>> +
>>>> +static int armpmu_request_brbe(struct arm_pmu *armpmu)
>>>> +{
>>>> + int cpu, err = 0;
>>>> +
>>>> + for_each_cpu(cpu, &armpmu->supported_cpus) {
>>>> + err = smp_call_function_single(cpu, arm_brbe_probe_cpu, armpmu, 1);
>>>
>>> Why does this need to be called on each CPU in the supported_cpus mask?
>>
>> Is not supported_cpus derived after partitioning the IRQ in pmu_parse_percpu_irq().
>> The idea is to fill up BRBE buffer attributes, on all such supported cpus which could
>> trigger PMU interrupt. Is the concern, that not all cpus in supported_cpus mask might
>> not be online during boot, hence IPIs could not be served, hence BRBE attributed for
>> them could not be fetched ?
>
> As above, I think this is solvable if we mandate that BRBE must be uniform
> *within* an arm_pmu's supported CPUs; then we only need one CPU in the
> supported_cpus mask to be present at boot time, as with the rest of the PMU
> code.
>
> We could *verify* that when onlining a CPU.
Understood.
>
>>> I don't see anything here to handle late hotplug, so this looks suspicious.
>>
>> Right, I should add cpu hotplug handling, otherwise risk loosing BRBE support on cpus
>> which might have been offline during boot i.e when above IPI based probe happened ?
>>
>>> Either we're missing something, or it's redundant at boot time.
>>
>> Should we add cpu hotplug online-offline handlers like some other PMU drivers ? Let
>> me know if there are some other concerns.
>>
>> cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, DRVNAME,
>> arm_brbe_cpu_startup,
>> arm_brbe_cpu_teardown)
>
> We *could* add that, but that's going to require ordering against the existing
> hooks for probing arm_pmu.
Right.
>
> Why can't this hang off the exising hooks for arm_pmu? We're treating this as
> part of the PMU anyway, so I don't understand why we should probe it
> separately.
Okay, will try and see what all changes are required to move the probing into generic
arm_pmu probe, and capture the BRBE attributes inside struct arm_pmu.
Powered by blists - more mailing lists