Re: [PATCH V4 1/2] perf: imx8_ddr_perf: add AXI ID filter support

[Robin Murphy <robin.murphy@arm.com>]

On 2019-08-01 6:25 am, Joakim Zhang wrote:
[...]
>>> @@ -195,6 +214,18 @@ static int ddr_perf_event_init(struct perf_event
>> *event)
>>>    	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?
> 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.

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

----->8-----
diff --git a/drivers/perf/fsl_imx8_ddr_perf.c 
b/drivers/perf/fsl_imx8_ddr_perf.c
index 63fe21600072..f0f643831fda 100644
--- a/drivers/perf/fsl_imx8_ddr_perf.c
+++ b/drivers/perf/fsl_imx8_ddr_perf.c
@@ -189,11 +189,23 @@ static u32 ddr_perf_read_counter(struct ddr_pmu 
*pmu, int counter)
         return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
  }

+static bool ddr_perf_is_filtered(struct perf_event *event)
+{
+       return event->attr.config == 0x41 || event->attr.config == 0x42;
+}
+
+static u32 ddr_perf_filter_val(struct perf_event *event)
+{
+       return event->attr.config1;
+}
+
  static int ddr_perf_event_init(struct perf_event *event)
  {
         struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
         struct hw_perf_event *hwc = &event->hw;
         struct perf_event *sibling;
+       bool is_filtered;
+       u32 filter_val;

         if (event->attr.type != event->pmu->type)
                 return -ENOENT;
@@ -215,10 +227,17 @@ static int ddr_perf_event_init(struct perf_event 
*event)
                         !is_software_event(event->group_leader))
                 return -EINVAL;

+       is_filtered = ddr_perf_is_filtered(event);
+       filter_val = ddr_perf_filter_val(event);
+
         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;
         }

         event->cpu = pmu->cpu;

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