Re: [PATCH v2 6/6] perf: ARM DynamIQ Shared Unit PMU support

[Jonathan Cameron]

On Mon, 24 Jul 2017 16:44:51 +0100
Suzuki K Poulose <Suzuki.Poulose@xxxxxxx> wrote:

> Hi Jonathan,
> 
> 
> On 24/07/17 15:50, Jonathan Cameron wrote:
> > On Mon, 24 Jul 2017 11:29:21 +0100
> > Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
> >  
> >> Add support for the Cluster PMU part of the ARM DynamIQ Shared Unit (DSU).
> >> The DSU integrates one or more cores with an L3 memory system, control
> >> logic, and external interfaces to form a multicore cluster. The PMU
> >> allows counting the various events related to L3, SCU etc, along with
> >> providing a cycle counter.
> >>
> >> The PMU can be accessed via system registers, which are common
> >> to the cores in the same cluster. The PMU registers follow the
> >> semantics of the ARMv8 PMU, mostly, with the exception that
> >> the counters record the cluster wide events.
> >>
> >> This driver is mostly based on the ARMv8 and CCI PMU drivers.
> >>
> >> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> >> Cc: Will Deacon <will.deacon@xxxxxxx>
> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>  
> > A few quick comments.  
> 
> Thanks for the detailed look. Comments in line. Btw, please could you leave a
> blank line after the quoted text and before your comment (like what I have
> don here) ? That way, it is way may much easier to find your comments.

Sure

> 
> >
> > Jonathan  
> >> ---  
> 
> >>
> >> diff --git a/arch/arm64/include/asm/arm_dsu_pmu.h b/arch/arm64/include/asm/arm_dsu_pmu.h
> >> new file mode 100644
> >> index 0000000..e7276fd
> >> --- /dev/null
> >> +++ b/arch/arm64/include/asm/arm_dsu_pmu.h
> >> @@ -0,0 +1,124 @@  
> > <snip>  
> >> +static inline void __dsu_pmu_counter_interrupt_disable(int counter)
> >> +{
> >> +	write_sysreg_s(BIT(counter), CLUSTERPMINTENCLR_EL1);
> >> +	isb();
> >> +}
> >> +
> >> +
> >> +static inline u32 __dsu_pmu_read_pmceid(int n)
> >> +{
> >> +	switch (n) {
> >> +	case 0:
> >> +		return read_sysreg_s(CLUSTERPMCEID0_EL1);
> >> +	case 1:
> >> +		return read_sysreg_s(CLUSTERPMCEID1_EL1);
> >> +	default:
> >> +		BUILD_BUG();
> >> +		return 0;
> >> +	}
> >> +}  
> > What is the benefit of having this lot in a header?  Is it to support future
> > additional drivers? If not, why not just push them down into the c code.  
> 
> As I mentioned in the cover letter, this is to keep the architecture specific code
> separate so that we could easily add support for this on arm32 kernel.
> 

Fair enough I suppose though I'd personally have done this as
a prepatch to series that adds arm32 support.

> >> --- /dev/null
> >> +++ b/drivers/perf/arm_dsu_pmu.c  
> > <snip>  
> >> +
> >> +/*
> >> + * Make sure the group of events can be scheduled at once
> >> + * on the PMU.
> >> + */
> >> +static int dsu_pmu_validate_group(struct perf_event *event)
> >> +{
> >> +	struct perf_event *sibling, *leader = event->group_leader;
> >> +	struct dsu_hw_events fake_hw;
> >> +
> >> +	if (event->group_leader == event)
> >> +		return 0;
> >> +
> >> +	memset(fake_hw.used_mask, 0, sizeof(fake_hw.used_mask));
> >> +	if (!dsu_pmu_validate_event(event->pmu, &fake_hw, leader))
> >> +		return -EINVAL;
> >> +	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
> >> +		if (!dsu_pmu_validate_event(event->pmu, &fake_hw, sibling))
> >> +			return -EINVAL;
> >> +	}
> >> +	if (dsu_pmu_validate_event(event->pmu, &fake_hw, event))  
> > Perhaps a comment to say why in this case validate_event has the opposite
> > meaning to the others cases above? (not !dsu_pmu_validate_event())  
> 
> Ah! Thanks for spotting. Thats a mistake. It should be !dsu_pmu_validate_event().
> I will fix it in the next version. We are making sure that the event can be
> scheduled, with the other events in the group already added.
> 
> >> +
> >> +static struct dsu_pmu *dsu_pmu_alloc(struct platform_device *pdev)
> >> +{
> >> +	struct dsu_pmu *dsu_pmu;
> >> +
> >> +	dsu_pmu = devm_kzalloc(&pdev->dev, sizeof(*dsu_pmu), GFP_KERNEL);
> >> +	if (!dsu_pmu)
> >> +		return ERR_PTR(-ENOMEM);  
> > A blank line here would make it a little more readable  
> >> +	raw_spin_lock_init(&dsu_pmu->pmu_lock);  
> > And one here.  
> >> +	return dsu_pmu;  
> 
> It doesn't look that complex here, given it doesn't take the lock.
> If it does help the reading, I could add it.
> 

It was indeed a really minor point!

> >> +}
> >> +
> >> +/**
> >> + * dsu_pmu_dt_get_cpus: Get the list of CPUs in the cluster.
> >> + */
> >> +static int dsu_pmu_dt_get_cpus(struct device_node *dev, cpumask_t *mask)
> >> +{
> >> +	int i = 0, n, cpu;
> >> +	struct device_node *cpu_node;
> >> +
> >> +	n = of_count_phandle_with_args(dev, "cpus", NULL);
> >> +	if (n <= 0)
> >> +		goto out;
> >> +	for (; i < n; i++) {
> >> +		cpu_node = of_parse_phandle(dev, "cpus", i);
> >> +		if (!cpu_node)
> >> +			break;
> >> +		cpu = of_device_node_get_cpu(cpu_node);
> >> +		of_node_put(cpu_node);
> >> +		if (cpu >= nr_cpu_ids)
> >> +			break;  
> > It rather seems like this is an error we would not want to skip over.  
> 
> Ok. That makes sense to me. I can return -EINVAL here.
> 
> >> +		cpumask_set_cpu(cpu, mask);
> >> +	}
> >> +out:
> >> +	return i > 0;  
> > Cleaner to actually return appropriate errors from within
> > this function and pass them all the way up.  
> 
> Sure, will do.
> 
> >> +static int dsu_pmu_probe(struct platform_device *pdev)
> >> +{
> >> +	int irq, rc, cpu;
> >> +	struct dsu_pmu *dsu_pmu;
> >> +	char *name;
> >> +
> >> +	static atomic_t pmu_idx = ATOMIC_INIT(-1);
> >> +
> >> +  
> > One blank line only.  
> 
> Ok.
> 
> >> +	/*
> >> +	 * Find one CPU in the DSU to handle the IRQs.
> >> +	 * It is highly unlikely that we would fail
> >> +	 * to find one, given the probing has succeeded.
> >> +	 */
> >> +	cpu = dsu_pmu_get_online_cpu(dsu_pmu);
> >> +	if (cpu >= nr_cpu_ids)
> >> +		return -ENODEV;
> >> +	cpumask_set_cpu(cpu, &dsu_pmu->active_cpu);
> >> +	rc = irq_set_affinity_hint(irq, &dsu_pmu->active_cpu);
> >> +	if (rc) {
> >> +		dev_warn(&pdev->dev, "Failed to force IRQ affinity for %d\n",
> >> +					 irq);
> >> +		return rc;
> >> +	}  
> 
> > It is a little unusual that you have the above two elements inline
> > here, but have a function to unwind them.  Just makes it a little
> > harder to read and leads to missing things like...  
> 
> The unwinding was added as a function to reuse the code. The "setup" steps
> undone by the unwind doesn't look separate from what we do in the probe,
> hence didn't go for a separate function.
> 
> >> +
> >> +	platform_set_drvdata(pdev, dsu_pmu);
> >> +	rc = cpuhp_state_add_instance(dsu_pmu_cpuhp_state,
> >> +						&dsu_pmu->cpuhp_node);
> >> +	if (rc)  
> > I believe irq_set_affinity_hit(dsu_pmu->irq, NULL) would make sense
> > here.  
> 
> Yes, you're right. Otherwise we could hit a WARN_ON. I will rearrange
> the probe code to a cleaner state.
>

Cool
 
> >> +		return rc;
> >> +
> >> +	dsu_pmu->irq = irq;
> >> +	dsu_pmu->pmu = (struct pmu) {
> >> +		.task_ctx_nr	= perf_invalid_context,
> >> +
> >> +		.pmu_enable	= dsu_pmu_enable,
> >> +		.pmu_disable	= dsu_pmu_disable,
> >> +		.event_init	= dsu_pmu_event_init,
> >> +		.add		= dsu_pmu_add,
> >> +		.del		= dsu_pmu_del,
> >> +		.start		= dsu_pmu_start,
> >> +		.stop		= dsu_pmu_stop,
> >> +		.read		= dsu_pmu_read,
> >> +
> >> +		.attr_groups	= dsu_pmu_attr_groups,
> >> +	};
> >> +
> >> +	rc = perf_pmu_register(&dsu_pmu->pmu, name, -1);
> >> +
> >> +	if (!rc)
> >> +		dev_info(&pdev->dev, "Registered %s with %d event counters",
> >> +				name, dsu_pmu->num_counters);
> >> +	else
> >> +		dsu_pmu_cleanup_dev(dsu_pmu);  
> > It is cleaner to have the error handled as the 'exceptional'
> > element.  Slightly more code, but easier to read.
> > i.e.
> >
> > 	if (rc) {
> > 		dsu_pmu_cleanup_dev(dsu_pmu);
> > 		return rc;
> > 	}
> >
> > 	dev_info(...)  
> 
> Ok.
> 
> >  
> >> +	return rc;
> >> +}
> >> +
> >> +static int dsu_pmu_device_remove(struct platform_device *pdev)  
> > The difference in naming style between this and probe is a little
> > confusing.
> >  
> 
> Ok
> 
> > Why not dsu_pmu_remove?  
> 
> Because it is callback for the platform device, which should eventually
> remove the PMU and any other cleanups. I could rename the probe to match it,
> i.e, dsu_pmu_device_probe().
> 

Just as good.

> 
> >> +{
> >> +	struct dsu_pmu *dsu_pmu = platform_get_drvdata(pdev);
> >> +
> >> +	dsu_pmu_cleanup_dev(dsu_pmu);
> >> +	perf_pmu_unregister(&dsu_pmu->pmu);  
> > The remove order should be the reverse of probe.
> > It just makes it more 'obviously' right and saves reviewer time.
> > If there is a reason not to do this, there should be a comment saying
> > why.
> >  
> 
> No, you're right. It should be in the reverse order, I will fix it.
> 
> >> +
> >> +
> >> +static int __init dsu_pmu_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> >> +					DRVNAME,
> >> +					NULL,
> >> +					dsu_pmu_cpu_teardown);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	dsu_pmu_cpuhp_state = ret;  
> > I'm just curious - what prevents this initialization being done in probe
> > rather than init?
> >  
> 
> Because, you need to do that only one per system and rather than one per DSU.
> There could be multiple DSUs connected via other links on a bigger platform.
> 
That makes sense.  Thanks for the explanation.

Jonathan
> 
> Suzuki