Hi Alejandro,
On Sat, Jan 7, 2023 at 2:53 PM Alejandro Colomar <alx.manpages@xxxxxxxxx> wrote:
>
> Hi Namhyung,
>
> On 1/4/23 07:17, Namhyung Kim wrote:
> > Add missing perf_event_attr fields, new event codes and sample type.
> > Also add descriptions for PERF_FORMAT_LOST.
> >
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>
> Thanks for the revised patch! Please see some minor comments below.
Sorry for the late reply and thanks for your review!
>
> > ---
[SNIP]
> > @@ -2201,7 +2332,9 @@ struct {
> > char data[size]; /* if PERF_SAMPLE_STACK_USER */
> > u64 dyn_size; /* if PERF_SAMPLE_STACK_USER &&
> > size != 0 */
> > - u64 weight; /* if PERF_SAMPLE_WEIGHT */
> > + union perf_sample_weight;
> > + /* if PERF_SAMPLE_WEIGHT */
> > + /* || PERF_SAMPLE_WEIGHT_STRUCT */
> > u64 data_src; /* if PERF_SAMPLE_DATA_SRC */
> > u64 transaction; /* if PERF_SAMPLE_TRANSACTION */
> > u64 abi; /* if PERF_SAMPLE_REGS_INTR */
> > @@ -2209,6 +2342,12 @@ struct {
> > /* if PERF_SAMPLE_REGS_INTR */
> > u64 phys_addr; /* if PERF_SAMPLE_PHYS_ADDR */
> > u64 cgroup; /* if PERF_SAMPLE_CGROUP */
> > + u64 data_page_size;
> > + /* if PERF_SAMPLE_DATA_PAGE_SIZE */
> > + u64 code_page_size;
> > + /* if PERF_SAMPLE_CODE_PAGE_SIZE */
> > + u64 size; /* if PERF_SAMPLE_AUX */
> > + char data[size]; /* if PERF_SAMPLE_AUX */
> > };
> > .EE
> > .in
> > @@ -2410,7 +2549,7 @@ is 0.
> > .TP
> > .I weight
> > If
> > -.B PERF_SAMPLE_WEIGHT
> > +.B PERF_SAMPLE_WEIGHT "or" PERF_SAMPLE_WEIGHT_STRUCT
>
> These should go in separate lines (otherwise, whitespaces are missing):
>
> .B FOO
> or
> .B BAR
Sure, will change.
>
> > is enabled, then a 64-bit value provided by the hardware
> > is recorded that indicates how costly the event was.
> > This allows expensive events to stand out more clearly
> > @@ -2643,7 +2782,28 @@ If the
> > flag is set,
> > then the 64-bit cgroup ID (for the perf_event subsystem) is recorded.
> > To get the pathname of the cgroup, the ID should match to one in a
> > -.B PERF_RECORD_CGROUP .
> > +.BR PERF_RECORD_CGROUP .
> > +.TP
> > +.I data_page_size
> > +If the
> > +.B PERF_SAMPLE_DATA_PAGE_SIZE
> > +flag is set,
> > +then the 64-bit page size value of the
> > +.B data
> > +address is recorded.
> > +.TP
> > +.I code_page_size
> > +If the
> > +.B PERF_SAMPLE_CODE_PAGE_SIZE
> > +flag is set,
> > +then the 64-bit page size value of the
> > +.B ip
> > +address is recorded.
> > +.TP
> > +.IR size ", " data[size]
>
> I prefer having them in separate lines (I know the current page already has that
> ugly stuff, but I'd rather write new stuff properly):
>
> .TP
> .I size
> .TQ
> .IR data [ size ]
>
> TQ is a continuation tag for TP.
Ok, good to know.
>
> > +If
> > +.B PERF_SAMPLE_AUX
> > +is enabled, then a snapshot of the aux buffer is recorded.
> > .RE
> > .TP
> > .B PERF_RECORD_MMAP2
> > @@ -2653,7 +2813,9 @@ calls returning executable mappings.
> > The format is similar to that of the
> > .B PERF_RECORD_MMAP
> > record, but includes extra values that allow uniquely identifying
> > -shared mappings.
> > +shared mappings. Depending on the
>
> Please _always_ break lines after a period (and usually also after a comma; and
> before an opening parenthesis, or after a closing one). But for periods it's a
> rather hard rule.
I see. Will change.
Thanks,
Namhyung
>
> > +.B PERF_RECORD_MISC_MMAP_BUILD_ID
> > +bit in the header, the extra values have different layout and meanings.
> > .IP
> > .in +4n
> > .EX
> > @@ -2664,10 +2826,20 @@ struct {
> > u64 addr;
> > u64 len;
> > u64 pgoff;
> > - u32 maj;
> > - u32 min;
> > - u64 ino;
> > - u64 ino_generation;
> > + union {
> > + struct {
> > + u32 maj;
> > + u32 min;
> > + u64 ino;
> > + u64 ino_generation;
> > + };
> > + struct { /* if PERF_RECORD_MISC_MMAP_BUILD_ID */
> > + u8 build_id_size;
> > + u8 __reserved_1;
> > + u16 __reserved_2;
> > + u8 build_id[20];
> > + };
> > + };
> > u32 prot;
> > u32 flags;
> > char filename[];
> > @@ -2704,6 +2876,14 @@ is the inode number.
> > .I ino_generation
> > is the inode generation.
> > .TP
> > +.I build_id_size
> > +is the actual size of
> > +.I build_id
> > +field (up to 20).
> > +.TP
> > +.I build_id
> > +is a raw data to identify a binary.
> > +.TP
> > .I prot
> > is the protection information.
> > .TP
>
> --
> <http://www.alejandro-colomar.es/>
