Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 26/07/17 12:19, Jan Glauber wrote:
> On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:
> > On 25/07/17 16:04, Jan Glauber wrote:
> > > Add support for the PMU counters on Cavium SOC memory controllers.
> > >
> > > This patch also adds generic functions to allow supporting more
> > > devices with PMU counters.
> > >
> > > Properties of the LMC PMU counters:
> > > - not stoppable
> > > - fixed purpose
> > > - read-only
> > > - one PCI device per memory controller
> > >
> > > Signed-off-by: Jan Glauber <jglauber@xxxxxxxxxx>
> > > ---
> > > drivers/perf/Kconfig       |   8 +
> > > drivers/perf/Makefile      |   1 +
> > > drivers/perf/cavium_pmu.c  | 424 +++++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/cpuhotplug.h |   1 +
> > > 4 files changed, 434 insertions(+)
> > > create mode 100644 drivers/perf/cavium_pmu.c
> > >
> > > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > > index e5197ff..a46c3f0 100644
> > > --- a/drivers/perf/Kconfig
> > > +++ b/drivers/perf/Kconfig
> > > @@ -43,4 +43,12 @@ config XGENE_PMU
> > >         help
> > >           Say y if you want to use APM X-Gene SoC performance monitors.
> > >
> > > +config CAVIUM_PMU
> > > +	bool "Cavium SOC PMU"
> >
> > Is there any specific reason why this can't be built as a module ?
>
> Yes. I don't know how to load the module automatically. I can't make it
> a pci driver as the EDAC driver "owns" the device (and having two
> drivers for one device wont work as far as I know). I tried to hook
> into the EDAC driver but the EDAC maintainer was not overly welcoming
> that approach.

> And while it would be possible to have it a s a module I think it is of
> no use if it requires manualy loading. But maybe there is a simple
> solution I'm missing here?


If you are talking about a Cavium specific EDAC driver, may be we could
make that depend on this driver "at runtime" via symbols (may be even,
trigger the probe of PMU), which will be referenced only when CONFIG_CAVIUM_PMU
is defined. It is not the perfect solution, but that should do the trick.


> > > +	/*
> > > +	 * Forbid groups containing mixed PMUs, software events are acceptable.
> > > +	 */
> > > +	if (event->group_leader->pmu != event->pmu &&
> > > +	    !is_software_event(event->group_leader))
> > > +		return -EINVAL;
> > > +
> > > +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
> > > +			    group_entry)
> > > +		if (sibling->pmu != event->pmu &&
> > > +		    !is_software_event(sibling))
> > > +			return -EINVAL;
> >
> > Do we also need to check if the events in the same group can be scheduled
> > at once ? i.e, there is enough resources to schedule the requested events from
> > the group.
>
> Not sure what you mean, do I need to check for programmable counters
> that no more counters are programmed than available?

Yes. What if there are two events, both trying to use the same counter (either
due to lack of programmable counters or duplicate events).

> > > +
> > > +	hwc->config = event->attr.config;
> > > +	hwc->idx = -1;
> > > +	return 0;
> > > +}
> > > +
> > ...
> >
> > > +static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
> > > +		       u64 event_base)
> > > +{
> > > +	struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> > > +	struct hw_perf_event *hwc = &event->hw;
> > > +
> > > +	if (!cmpxchg(&pmu_dev->events[hwc->config], NULL, event))
> > > +		hwc->idx = hwc->config;
> > > +
> > > +	if (hwc->idx == -1)
> > > +		return -EBUSY;
> > > +
> > > +	hwc->config_base = config_base;
> > > +	hwc->event_base = event_base;
> > > +	hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > > +
> > > +	if (flags & PERF_EF_START)
> > > +		pmu_dev->pmu.start(event, PERF_EF_RELOAD);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void cvm_pmu_del(struct perf_event *event, int flags)
> > > +{
> > > +	struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> > > +	struct hw_perf_event *hwc = &event->hw;
> > > +	int i;
> > > +
> > > +	event->pmu->stop(event, PERF_EF_UPDATE);
> > > +
> > > +	/*
> > > +	 * For programmable counters we need to check where we installed it.
> > > +	 * To keep this function generic always test the more complicated
> > > +	 * case (free running counters won't need the loop).
> > > +	 */
> > > +	for (i = 0; i < pmu_dev->num_counters; i++)
> > > +		if (cmpxchg(&pmu_dev->events[i], event, NULL) == event)
> > > +			break;
> >
> > I couldn't see why hwc->config wouldn't give us the index where we installed
> > the event in pmu_dev->events. What am I missing ?
>
> Did you see the comment above? It is not yet needed but will be when I
> add support for programmable counters.

Is it supported in this series ?

> If it is still confusing I can
> also remove that for now and add it back later when it is needed.

What is the hwc->idx for programmable counters ? is it going to be different
than hwc->config ? If so, can we use hwc->idx to keep the idx where we installed
the event ?

Suzuki