-----Original Message-----
From: Robin Murphy <robin.murphy@arm.com>
Sent: 2019年8月1日 18:00
To: Joakim Zhang <redacted>; will@kernel.org;
mark.rutland@arm.com; Frank Li [off-list ref]
Cc: linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de; dl-linux-imx
[off-list ref]
Subject: Re: [PATCH V4 1/2] perf: imx8_ddr_perf: add AXI ID filter support

On 2019-08-01 6:25 am, Joakim Zhang wrote:
[...]

quoted

quoted

quoted

@@ -195,6 +214,18 @@ static int ddr_perf_event_init(struct

perf_event

*event)

quoted

   	struct hw_perf_event *hwc = &event->hw;
   	struct perf_event *sibling;

+	if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) {
+		if (event->attr.config == 0x41)
+			pmu->axi_id_read = event->attr.config1;
+
+		if (event->attr.config == 0x42)
+			pmu->axi_id_write = event->attr.config1;
+
+		if (pmu->axi_id_read && pmu->axi_id_write &&
+		    (pmu->axi_id_read != pmu->axi_id_write))
+			return -EINVAL;
+	}

This isn't the correct approach that Mark outlined :(

In event_init, you should validate that any filtering for the given
event is compatible with any other sibling events in the same group,
but you should not consider (and should definitely not change) the
current state of the PMU at that point. This step is about rejecting
event configurations which could
*never* be successfully scheduled (since a group represents a set of
events which must be scheduled all at the same time).

In event_add, you know the given event/group is sufficiently valid to
*potentially* be scheduled, given that it has passed the event_init
checks, but you then need to check that the filtering is compatible
with all other events
*currently* counting on the PMU. If this fails, perf core will try to
reschedule the current events until the new one is able to run.
That's why you need the additional step of validating groups
beforehand, because otherwise you could end up with contradictory
scheduling constraints at this point and never make progress.

Hi Mark and Robin,

Thanks for all your kindly detailed explanation firstly.

My understanding from your comments, I need to validate the filtering (i.e.

config1/axi_id) for *all* events in same group during event_init, right?

quoted

But it's so strange for that axi_id is only for axi-id-read and axi-id-write event.

We don't need to specify axi_id for any other events when mixed with these
two events.

Sorry, I implicitly meant all *relevant* events - obviously there's nothing to
check for events which don't have filtering anyway. All that matters is the case
where we're asked to create a read/write event in a group which already has at
least one other read/write event as a sibling. I've sketched out a quick
(completely untested) example of one way to do that part below. The logic for
event_add would be very similar, except instead of comparing the sibling
against the event, there you'd compare the event against the current PMU
state.

quoted

If I can just check the axi-id-read and axi-id-write event during
event_add and then pass the axi_id value to the filter register. Don't
check the case that user specify both of them at the same time with different

filtering value. Instead of checking it in the driver, I add the doc in
Documentation/admin-guide/perf/ directory to note that axi-id-read and
axi-id-write event should be specified separately, or specified at the same time
with same axi_id value.

Sure, we could just rely on the user to get it right, but that means there's a fair
chance that the user can inadvertently get it wrong, get nonsensical results,
and waste a week trying to debug a perceived problem which doesn't actually
exist. It's not difficult for the driver to perform the correct validation, so it's
better for everyone if it does.

It also seems reasonable that a user might want to intentionally measure
events on different IDs over the same run (but not in the same group), e.g. to
compare the relative average bandwidth of two devices, perhaps to tune QoS
parameters. That requires perf core to know it needs to rotate the events
during the run, which will only happen if event_add does the right thing.

Robin.

Hi Robin,

I completely understood what you said now, thank you very much. But I came across another issue when I
test this case. You can see below.

[...]

         for_each_sibling_event(sibling, event->group_leader) {
                 if (sibling->pmu != event->pmu &&
                                 !is_software_event(sibling))
                         return -EINVAL;
+
+               if (is_filtered && ddr_perf_is_filtered(sibling) &&
+                   ddr_perf_filter_val(sibling) != filter_val)
+                       return -EINVAL;
         }

[...]

Need to check the axi id value of sibling events in one same group with for_each_sibling_event ():
#define for_each_sibling_event(sibling, event)                  \
         if ((event)->group_leader == (event))                   \
                 list_for_each_entry((sibling), &(event)->sibling_list, sibling_list)

But I found that all check in this For loop will never be checked, that means the code never runs into this For loop.
               if (sibling->pmu != event->pmu &&
                              !is_software_event(sibling))
                       return -EINVAL;

               if (is_filtered && ddr_perf_is_filtered(sibling) &&
                  ddr_perf_filter_val(sibling) != filter_val)
                       return -EINVAL;

Finally I found that it can't iterate over the list with list_for_each_entry((sibling), &(event)->sibling_list, sibling_list). So driver
can't reject event group with axi id illegal value during event_init() now. Do you know why it can't iterate to sibling events?

Best Regards,
Joakim Zhang
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel