lkml.org 
[lkml]   [2017]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 6/6] perf: ARM DynamIQ Shared Unit PMU support
From
Date
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@arm.com> 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@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 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.

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

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

>> +}
>> +
>> +/**
>> + * 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.

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


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


Suzuki

\
 
 \ /
  Last update: 2017-07-24 17:46    [W:0.055 / U:0.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site