-----Original Message-----
From: Joakim Zhang
Sent: 2019年8月5日 18:33
To: Robin Murphy <robin.murphy@arm.com>; 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

quoted

-----Original Message-----
From: Joakim Zhang
Sent: 2019年8月2日 15:20
To: Robin Murphy <robin.murphy@arm.com>; 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

quoted

-----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.

quoted

quoted

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.

quoted

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

quoted

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.

quoted

quoted

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.

quoted

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.

[...]

quoted

         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?

Hi Robin,

I have an another question, with below configuration, if this means cycles, read
and write events is an event group?
	perf stat -e imx8_ddr0/cycles/,imx8_ddr0/read/,imx8_ddr0/write/ sleep 1

If yes, I found that perf event core attach event group(perf_group_attach() in
kernel/events/core.c) after perf event initialization (perf_try_init_event() call
pmu->event_init() callback in kernel/events/core.c).
Is it reasonable as we always check the sibling event form event_init callback?
And each perf event pass to perf_group_attach() always point to it's group
leader, so for_each_sibling_event() in event_init() can't iterate to it's sibling
events, as it has no sibling events from perf_froup_attach().

Do I misunderstand angthing?

Hi Robin,

I have known that below is correct method to configure an event group:
	perf stat -e '{imx8_ddr0/cycles/,imx8_ddr0/read/,imx8_ddr0/write/}' sleep 1

I will send out PATCH V5, please help review, thank you.
 
Best Regards,
Joakim Zhang

Best Regards,
Joakim Zhang

quoted

Best Regards,
Joakim Zhang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel