On Mon, Nov 21, 2022 at 12:06:31PM +0530, Anshuman Khandual wrote:

On 11/18/22 23:31, Mark Rutland wrote:

quoted

On Mon, Nov 07, 2022 at 11:55:11AM +0530, Anshuman Khandual wrote:

quoted

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.

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.

[...]

quoted

quoted

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

quoted

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 ?

quoted

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.

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.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel