Re: BUG in "perf: Suppress AUX/OVERWRITE records"?

[Ben Gainey]

On Tue, 2019-03-26 at 09:02 +0200, Alexander Shishkin wrote:
> Ben Gainey <Ben.Gainey@xxxxxxx> writes:
>
> > Hi all
> >
> > Regarding commit 1627314fb54a33ebd23bd08f2e215eaed0f44712 "perf:
> > Suppress AUX/OVERWRITE records", I have found that I no longer
> > receive
> > PERF_RECORD_AUX on context switch when collecting data from the
> > arm_spe
> > PMU driver. This is because, on context switch, the arm_spe driver
> > calls perf_aux_output_end with `handle->aux_flags == 0`, failing
> > the
> > test added in this commit.
> >
> > This is a problem as it means when capturing data for multiple
> > threads
> > (using perf_event_open) where AUX data is written to a per-cpu
> > buffer,
> > I can no longer accurately attribute SPE AUX data to an individual
> > thread.
>
> Sounds like PERF_RECORD_SWITCH should be sufficient for your
> purposes,
> have you considered that?


I'm not trying to track the context switch its self. I want know which
chunks of AUX data were associated with a given thread.

Consider:

Thread A executes
 ... context switch    [WRITES AUX DATA FROM OFFSET n...n+l]
Thread B executes
 ... context switch    [WRITES AUX DATA FROM OFFSET n+l...n+l+m]

From the point of view of someone reading back the ring buffer data in
userspace, depending on when the consumer wakes whilst waiting on the
ring buffer, it may only see aux_head/tail covering the range
[n...n+l+m]. Prior to this change I would see two AUX records, one with
offset n, length l, and the second offset n+l, length m. After the
change, there are no AUX records.


>
> > If I read the intent of the commit as to remove OVERWRITE AUX
> > records,
> > then it seems the added if condition is incorrect and should
> > probably
> > be formulated as:
> >
> >     if ((handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE) ||
> > !handle-
> > > aux_flags)
> >
> > Is this correct (and would you like a patch?), or is my use of
> > PERF_RECORD_AUX incorrect in this case?
>
> No, the point of AUX records is to communicate useful things about
> the
> data in AUX buffer.

Sure, and knowing when the end of the AUX data for one thread ends and
the next starts is useful, surely?

Additionally the change introduced is preventing writing an
PERF_RECORD_AUX when flags does not contain PERF_AUX_FLAG_OVERWRITE
(flags==0) which seems to not be the intention of the commit?

> It was an unintentional side effect that it also
> happened to coincide with context switches in the overwrite mode.

I'm not using overwrite mode, I'm opening the mmap with PROT_WRITE
(i.e. in truncate mode).

Regards
Ben


>
> Thanks,
> --
> Alex
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.