Re: [PATCH v4 15/21] PM / devfreq: rockchip-dfi: Add perf support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, May 09, 2023 at 09:04:58PM +0100, Robin Murphy wrote:
> On 2023-05-05 12:38, Sascha Hauer wrote:
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index eae010644935a..400b1b360e3c9 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/of_device.h>
> >   #include <linux/bitfield.h>
> >   #include <linux/bits.h>
> > +#include <linux/perf_event.h>
> >   #include <soc/rockchip/rockchip_grf.h>
> >   #include <soc/rockchip/rk3399_grf.h>

[...]

> > +static int rockchip_ddr_perf_event_init(struct perf_event *event)
> > +{
> > +	struct rockchip_dfi *dfi = container_of(event->pmu, struct rockchip_dfi, pmu);
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (event->attach_state & PERF_ATTACH_TASK)
> > +		return -EOPNOTSUPP;
> 
> IMO this should be -EINVAL - the event isn't something that the driver would
> consider valid in general but happens to not be supported by this particular
> PMU instance, it's something that's fundamentally meaningless because the
> memory controller has no notion of what a task or even a CPU is, much less
> the ability to ever attribute low-level DRAM accesses to one.
> 
> > +
> > +	if (event->cpu < 0) {
> > +		dev_warn(dfi->dev, "Can't provide per-task data!\n");
> > +		return -EOPNOTSUPP;
> 
> Ditto.
> 
> > +	}
> > +
> 
> If you can't snapshot multiple counters atomically (and don't want to
> start/stop the whole monitor to achieve the same effect - I guess it would
> be hard to do that without getting in the way of devfreq operation), then
> you should ideally also check for and reject event groups containing
> multiple hardware events.

Yes, starting/stopping the monitor for atomic snapshots is hard to do
without influencing devfreq operation, that's why I decided against it.

OTOH I consider it very useful being able to monitor read-bytes and
write-bytes at the same time (or to simultaneously monitor multiple
channels in the next version).

Indeed non atomically reading the snapshots introduces a small error,
but normally the time it takes to read out all channels should be rather
small compared to the time between the snapshots, so I think this error
is negligible.


> > +static int rockchip_ddr_perf_init(struct rockchip_dfi *dfi)
> > +{
> > +	struct pmu *pmu = &dfi->pmu;
> > +	int ret;
> > +
> > +	pmu->module = THIS_MODULE;
> > +	pmu->capabilities = PERF_PMU_CAP_NO_EXCLUDE;
> > +	pmu->task_ctx_nr = perf_invalid_context;
> > +	pmu->attr_groups = attr_groups;
> > +	pmu->event_init  = rockchip_ddr_perf_event_init;
> > +	pmu->add = rockchip_ddr_perf_event_add;
> > +	pmu->del = rockchip_ddr_perf_event_del;
> > +	pmu->start = rockchip_ddr_perf_event_start;
> > +	pmu->stop = rockchip_ddr_perf_event_stop;
> > +	pmu->read = rockchip_ddr_perf_event_update;
> > +
> > +	dfi->cpu = raw_smp_processor_id();
> > +
> > +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
> > +				      "rockchip_ddr_perf_pmu",
> > +				      NULL,
> > +				      ddr_perf_offline_cpu);
> 
> So, each instance gets its own distinct multi-instance state to only support
> a single instance each, except there can clearly only ever be a single
> instance globally anyway, since the PMU is registered with a fixed name... I
> can only guess that maybe such a contrivance is born of the notion of
> "global variables are bad", but honestly, this is worse :/

So you are suggesting that struct pmu should be statically initialized
rather than dynamically allocated, right?
Are you referring to struct pmu only or also to struct rockchip_dfi?

I am not sure I would like this. Yes, the fixed name makes the driver
inherently single instance only, nevertheless I think the driver code is
easier to follow when it's written the usual way, with lifetime of the
dynamic data between probe() and remove().

As a compromise I could allocate the name dynamically as well, but I
have no idea how to make up a good id. Some other drivers use the base
address of the device, but that would mean the 'perf stat' calls would
differ between different SoCs without any real gain.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip