Re: [PATCH v7 4/5] perf: CXL Performance Monitoring Unit driver

[Jonathan Cameron]

Hi,

Tidied up the typos. Thanks,

> > +static int cxl_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> > +{
> > +       struct cxl_pmu_info *info = hlist_entry_safe(node, struct cxl_pmu_info, node);
> > +       unsigned int target;
> > +
> > +       if (info->on_cpu != cpu)
> > +               return 0;
> > +
> > +       info->on_cpu = -1;
> > +       target = cpumask_any_but(cpu_online_mask, cpu);
> > +       if (target >= nr_cpu_ids) {
> > +               dev_err(info->pmu.dev, "Unable to find a suitable CPU\n");
> > +               return 0;
> > +       }
> > +
> > +       perf_pmu_migrate_context(&info->pmu, cpu, target);
> > +       info->on_cpu = target;
> > +       /*
> > +        * CPU HP lock is held so we should be guaranteed that this CPU hasn't yet
> > +        * gone away.
> > +        */
> > +       WARN_ON(irq_set_affinity(info->irq, cpumask_of(target)));
> > +
> > +       return 0;
> > +}  
> 
> IIUC a CXL PMU hardware (say cxl_pmu_mem0.0) is shared across
> all CPUs and it would return the same value when read from any CPU,
> right?

Correct, it will return the same value when used from any CPU.
I'm not sure what issue you are indicating.

My understanding is that, even for such cases, perf uses percpu
variables that mean we still have to ensure that the interrupt
handling occurs on the CPU we have migrated the context to.

There are a lot of similar driver in perf already from a quick
git grep cpumask_any_but\(cpu_online_mask, 

It might be nice to enable perf to operate for these devices without
the percpu context though. I haven't looked into whether that
is worth doing.

Jonathan




> 
> Thanks,
> Namhyung
> 
> 
> > +
> > +static __init int cxl_pmu_init(void)
> > +{
> > +       int rc;
> > +
> > +       rc = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> > +                                    "AP_PERF_CXL_PMU_ONLINE",
> > +                                    cxl_pmu_online_cpu, cxl_pmu_offline_cpu);
> > +       if (rc < 0)
> > +               return rc;
> > +       cxl_pmu_cpuhp_state_num = rc;
> > +
> > +       rc = cxl_driver_register(&cxl_pmu_driver);
> > +       if (rc)
> > +               cpuhp_remove_multi_state(cxl_pmu_cpuhp_state_num);
> > +
> > +       return rc;
> > +}
> > +
> > +static __exit void cxl_pmu_exit(void)
> > +{
> > +       cxl_driver_unregister(&cxl_pmu_driver);
> > +       cpuhp_remove_multi_state(cxl_pmu_cpuhp_state_num);
> > +}
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(CXL);
> > +module_init(cxl_pmu_init);
> > +module_exit(cxl_pmu_exit);
> > +MODULE_ALIAS_CXL(CXL_DEVICE_PMU);
> > --
> > 2.39.2
> >  
>