Re: [PATCH v7 1/4] perf: Support PERF_SAMPLE_READ with inherit

[Ben Gainey]

On Fri, 2024-06-07 at 13:02 +0200, Peter Zijlstra wrote:
> On Fri, Jun 07, 2024 at 10:16:15AM +0000, Ben Gainey wrote:
> 
> > > > @@ -3532,11 +3544,18 @@ perf_event_context_sched_out(struct
> > > > task_struct *task, struct task_struct *next)
> > > > � perf_ctx_disable(ctx, false);
> > > > �
> > > > � /* PMIs are disabled; ctx->nr_pending is stable. */
> > > > - if (local_read(&ctx->nr_pending) ||
> > > > + if (ctx->nr_inherit_read ||
> > > > + ��� next_ctx->nr_inherit_read ||
> > > > + ��� local_read(&ctx->nr_pending) ||
> > > > � ��� local_read(&next_ctx->nr_pending)) {
> > > 
> > > This seems unfortunate, nr_pending and nr_inherit_read are both
> > > used
> > > exclusively to inhibit this context switch optimization. Surely
> > > they
> > > can
> > > share the exact same counter.
> > > 
> > > That is, rename nr_pending and use it for both?
> > 
> > Sure, how about "nr_no_switch_fast" ?
> 
> Yeah, I suppose.
> 
> 
> > Sure, presumably you are happy with just calling
> > "perf_event_count(event, false)" everywhere it is currently used,
> > rather than renaming it to something shorter and keeping the two
> > functions?
> 
> Yeah, there aren't *that* many instances. Your current patch already
> touches them all anyway.
> 
> > > That is, I would really rather you had:
> > > 
> > > static inline u64 perf_event_count(struct perf_event *event, bool
> > > self)
> > > {
> > > �if (self)
> > > �return local64_read(&event->count);
> > > 
> > > �return local64_read(&event->count) + local64_read(&event-
> > > > child_count);
> > > }
> > > 
> > > And then actually use that argument as intended.
> > 
> > 
> > Fair point.
> > 
> > I was trying to avoid the 3 subsequent uses all having to repeat
> > "from_sample && has_inherit_and_sample_read(&event->attr)", which
> > feels
> > a bit of a pit-trappy.�
> > 
> > I suppose I could pull that into a
> > "use_self_value(from_sample,event)"?
> 
> IIRC they all originate in a single location around
> perf_output_read(),
> that already has the event and could easily 'correct' the semantic
> meaning by doing the above once or so.
> 

As far as I can tell, you can mix events in a group with inconsistent
values of PERF_SAMPLE_READ which means that doing it at the top level
introduces an inconsistency/confusing behaviour since it makes the
"thread-local-ness" of the read_format values a property of the event
that caused the sample, rather than of the specific event to which the
value belongs. The current implementation makes it a property of the
specific event not the sample event. Specifically, when
perf_output_read_group reads a child event that does not have
PERF_SAMPLE_READ, the sample event must have PERF_SAMPLE_READ, meaning
the child event will give the thread-local value even though it was not
created as inherit+PERF_SAMPLE_READ

I can either:

 * Keep it so that the perf_output_read_group uses per-event value for
self
 * Rework the deliver_sample_value in session.c to base its decision on
the sample event rather than the specific event
 * Forbid inconsistent PERF_SAMPLE_READ for events in a group
 * Something else?



> > > 
> > > > @@ -7205,13 +7232,14 @@ void
> > > > perf_event__output_id_sample(struct
> > > > perf_event *event,
> > > > �
> > > > �static void perf_output_read_one(struct perf_output_handle
> > > > *handle,
> > > > � struct perf_event *event,
> > > > - u64 enabled, u64 running)
> > > > + u64 enabled, u64 running,
> > > > + bool from_sample)
> > > > �{
> > > > � u64 read_format = event->attr.read_format;
> > > > � u64 values[5];
> > > > � int n = 0;
> > > > �
> > > > - values[n++] = perf_event_count(event);
> > > > + values[n++] = perf_event_count(event, from_sample);
> > > 
> > > ...observe the fail... from_sample != self-value-only
> > 
> > By fail you are referring to the difference in names?
> 
> The difference in meaning, one is from-sample, the other is self-
> value.
> Per the extra condition squirrelled away they are not equivalent.