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

[Namhyung Kim]

On Tue, May 30, 2023 at 6:50 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
>
> 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.

I was thinking if we could avoid IPIs for read from other
CPUs.  It has PERF_EV_CAP_READ_ACTIVE_PKG for some uncore events
which can be shared among CPUs in the same package
to skip the IPI and to read it from the local CPU.

But I think this can be a separate step.

Thanks,
Namhyung