perf group read for inherited events

The thing that seems difficult is PERF_SAMPLE_READ vs inherited, irrespective of PERF_FORMAT_GROUP.

The error seems to be in that patch you fingered:

3dab77fb1bf8 ("perf: Rework/fix the whole read vs group stuff")

-       PERF_SAMPLE_GROUP                       = 1U << 4,
+       PERF_SAMPLE_READ                        = 1U << 4,

- if (attr->inherit && (attr->sample_type & PERF_SAMPLE_GROUP))
+ if (attr->inherit && (attr->read_format & PERF_FORMAT_GROUP))

is a clear fail :/

This is more recent. Here I failed to find a coherent text to explain the locking. It is correct through. I think its something like:

@@ -4426,8 +4426,9 @@ static int perf_read_group(struct perf_event *event,
values[0] = 1 + leader->nr_siblings;

/*
- * By locking the child_mutex of the leader we effectively
- * lock the child list of all siblings.. XXX explain how.
+ * By locking the child_mutex of the leader we effectively lock the
+ * child list of all siblings. Since inherit_group() will first clone
+ * the leader and will this be blocked on us holding its child_mutex.
*/
mutex_lock(&leader->child_mutex);

The below seems to be the correct thing. It is rather unfortunate that this would break/significantly change existing semantics :/

---
 kernel/events/core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index 8d6acaeeea17..2d9de6fb9a5a 100644
@@ -5722,9 +5722,6 @@ static void perf_output_read_one(struct perf_output_handle *handle,
 	__output_copy(handle, values, n * sizeof(u64));
 }
 
-/*
- * XXX PERF_FORMAT_GROUP vs inherited events seems difficult.
- */
 static void perf_output_read_group(struct perf_output_handle *handle,
 			    struct perf_event *event,
 			    u64 enabled, u64 running)

@@ -5769,6 +5766,12 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 #define PERF_FORMAT_TOTAL_TIMES (PERF_FORMAT_TOTAL_TIME_ENABLED|\
 				 PERF_FORMAT_TOTAL_TIME_RUNNING)
 
+/*
+ * XXX PERF_SAMPLE_READ vs inherited events seems difficult.
+ *
+ * The problem is that its both hard and excessively expensive to iterate the
+ * child list from interrupt/NMI context.
+ */
 static void perf_output_read(struct perf_output_handle *handle,
 			     struct perf_event *event)
 {

@@ -9434,9 +9437,10 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	local64_set(&hwc->period_left, hwc->sample_period);
 
 	/*
-	 * we currently do not support PERF_FORMAT_GROUP on inherited events
+	 * We currently do not support PERF_SAMPLE_READ on inherited events.
+	 * See perf_output_read().
 	 */
-	if (attr->inherit && (attr->read_format & PERF_FORMAT_GROUP))
+	if (attr->inherit && (attr->sample_type & PERF_SAMPLE_READ))
 		goto err_ns;
 
 	if (!has_branch_stack(event))

The "existing semantics" as in ignoring the PERF_SAMPLE_READ in sample_type, even though it wasn't implemented? It seems reasonable to me.

If you really worry about it could drop a printk_once in and see if it triggers anywhere (and if yes drop the if completely)

The patch looks good to me. Please consider adding it. I have some patches to use this in perf stat, will submit later.

-Andi

Right, so where we used to accept PERF_SAMPLE_READ on inherited events, we now no longer will.

Note that it currently doesn't work right, even if it doesn't fail like with the proposed patch.

Typically Vince will (rightly) complain when I change things like this. But seeing how even if we accept it, it is fairly terminally buggered in any case, we could change it.

Vince, do you know of anybody that would be immediately affected by this?

I often only hear about breakage months later after it's too late.

So the issue is currently if you were sampling, and you were sampling on an event group, and you had set PERF_SAMPLE_READ to get all counts for a group, and the event was also inherited.... perf_event_open() would let you do this even though the results would probably be wrong?

I'm not aware of anyone trying to do this, or at least I should say PAPI isn't trying to do this (PAPI currently has pretty simplistic sampling support, though that's getting a major overhaul this summer).

Vince

No, anything PERF_SAMPLE_READ (group or not) on inherited events is wrong.

It would only report the event count of the current task count + all dead child counts (if you hit the parent event). It would not include the current count of any other live tasks in the hierarchy.

And the problem is that fixing this is rather tricky and iterating the hierarchy can be excessively expensive in any case (imagine having to iterate several hundred tasks tasks from that NMI/interrupt).

And since its from NMI/interrupt context it is impossible to get the current count of any other live tasks that are running on another CPU.

Right, currently we have the filter on PERF_FORMAT_GROUP, but it should be PERF_SAMPLE_READ.

So a !group SAMPLE_READ on inherited is currently allowed but returns 'interesting' values.

ah, yes. I should fix the wording in the manpage as it's a bit confusing.

I'm not aware of anything that will break on fixing this.

Especially if we are always returning bad results then it would be better to break things so we find people (if any) who are depending on the wrong results.

Vince