[PATCH] Added perf functionality to mmdc driver

On Fri, Aug 05, 2016 at 09:35:25AM -0500, Zhengyu Shen wrote:

$ perf stat -e mmdc/busy-cycles/,mmdc/read-accesses/,mmdc/read-bytes/,mmdc/total-cycles/,mmdc/write-accesses/,mmdc/write-bytes/ dd if=/dev/zero of=/dev/null bs=1M count=5000
Performance counter stats for 'dd if=/dev/zero of=/dev/null bs=1M count=5000':

         898021787      mmdc/busy-cycles/
          14819600      mmdc/read-accesses/
            471.30 MB   mmdc/read-bytes/
        2815419216      mmdc/total-cycles/
          13367354      mmdc/write-accesses/
            427.76 MB   mmdc/write-bytes/

       5.334757334 seconds time elapsed

As Peter noted, you must better describe this, e.g.

* What is the MMDC (it appears to be a DRAM controller?)
* Which devices have this?
* What types of event does it support?
* Does it have many cuonters? How wide are they? Is there an overflow
  interrupt?
* Who is this useful for?

Generally, this looks far from complete, though it's difficult to tell
what the driver should be doing, given the lack of information. Comments
below.

[...]

+PMU_EVENT_ATTR_STRING(total-cycles, evattr_total_cycles, "event=0x01")
+PMU_EVENT_ATTR_STRING(busy-cycles, evattr_busy_cycles, "event=0x02")
+PMU_EVENT_ATTR_STRING(read-accesses, evattr_read_accesses, "event=0x03")
+PMU_EVENT_ATTR_STRING(write-accesses, evattr_write_accesses, "config=0x04")
+PMU_EVENT_ATTR_STRING(read-bytes, evattr_read_bytes, "event=0x05")
+PMU_EVENT_ATTR_STRING(read-bytes.unit, evattr_read_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(read-bytes.scale, evattr_read_bytes_scale, "0.000001");
+PMU_EVENT_ATTR_STRING(write-bytes, evattr_write_bytes, "event=0x06")
+PMU_EVENT_ATTR_STRING(write-bytes.unit, evattr_write_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(write-bytes.scale, evattr_write_bytes_scale, "0.000001");
+
+struct mmdc_pmu
+{
+	struct pmu pmu;
+	void __iomem *mmdc_base;
+};
+
+static struct attribute *events_attrs[] = {
+	&evattr_total_cycles.attr.attr,
+	&evattr_busy_cycles.attr.attr,
+	&evattr_read_accesses.attr.attr,
+	&evattr_write_accesses.attr.attr,
+	&evattr_read_bytes.attr.attr,
+	&evattr_read_bytes_unit.attr.attr,
+	&evattr_read_bytes_scale.attr.attr,
+	&evattr_write_bytes.attr.attr,
+	&evattr_write_bytes_unit.attr.attr,
+	&evattr_write_bytes_scale.attr.attr,
+	NULL,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-63");
+static struct attribute *format_attrs[] = {
+	&format_attr_event.attr,
+	NULL,
+};
+
+static struct attribute_group format_attr_group = {
+	.name = "format",
+	.attrs = format_attrs,
+};
+
+static struct attribute_group events_attr_group = {
+	.name = "events",
+	.attrs = events_attrs,
+};
+
+static const struct attribute_group * attr_groups[] = {
+	&events_attr_group,
+	&format_attr_group,
+	NULL,
+};

This is an uncore PMU. You need a cpumask file, along with migration
handling.

See drivers/bus/arm-ccn.c for an example.

+static int mmdc_event_init(struct perf_event *event)
+{
+	u64 val;
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;

You *must* validate the event here.

Check the config, grouping, filtering options, etc.

+	mmdc_enable_profiling(event);
+	val = mmdc_read_counter(event);
+	local64_set(&event->hw.prev_count, val);

Are the counters not writable?

+	return 0;
+}
+
+static void mmdc_event_update(struct perf_event * event)
+{
+	u64 val;
+	val = mmdc_read_counter(event);
+	local64_set(&event->count, val);
+}

What about prev_count?

+static void mmdc_event_start(struct perf_event *event, int flags)
+{
+	mmdc_event_update(event);
+}

Huh?

Are the counters always-running, with no mechanism to start/stop them?

If so, you must reject event groups, as ratios will be bogus.

+static int mmdc_event_add(struct perf_event *event, int flags)
+{
+	if (flags & PERF_EF_START)
+		mmdc_event_start(event, flags);
+	return 0;
+}
+
+static void mmdc_event_stop(struct perf_event *event, int flags)
+{
+	mmdc_event_update(event);
+}
+
+static void mmdc_event_del(struct perf_event *event, int flags)
+{
+	mmdc_event_stop(event, PERF_EF_UPDATE);
+}
+
+static void mmdc_pmu_init(struct mmdc_pmu *pmu_mmdc, void __iomem *mmdc_base)
+{
+	*pmu_mmdc = (struct mmdc_pmu) {
+		.pmu = (struct pmu) {
+			.task_ctx_nr    = perf_sw_context,

NAK. This is *not* a softwre PMU. This is a system/uncore PMU, and
should use perf_invalid_context.

+			.attr_groups    = attr_groups,
+			.event_init     = mmdc_event_init,
+			.add            = mmdc_event_add,
+			.del            = mmdc_event_del,
+			.start          = mmdc_event_start,
+			.stop           = mmdc_event_stop,
+			.read           = mmdc_event_update,
+			.capabilities   = PERF_PMU_CAP_NO_INTERRUPT,

This avoids sampling events, but you still need to poll to account for
overflow.

Nit: use sizeof(*pmu_mmdc)

+	if (!pmu_mmdc) {
+		pr_err("failed to allocate PMU device!\n");
+		return -ENOMEM;
+    }
+	mmdc_pmu_init(pmu_mmdc, mmdc_base);
+	platform_set_drvdata(pdev, pmu_mmdc);
+	perf_pmu_register(&(pmu_mmdc->pmu), "mmdc", -1);
+	return 0;
+}

Thanks,
Mark.