Re: [PATCH 00/12] libperf: Add events to perf/event.h

From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>, lkml <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Michael Petlan <mpetlan@redhat.com>
Subject: Re: [PATCH 00/12] libperf: Add events to perf/event.h
Date: Mon, 26 Aug 2019 19:08:10 -0300	[thread overview]
Message-ID: <20190826220810.GA21761@kernel.org> (raw)
In-Reply-To: <20190826164734.GE17554@krava>

Em Mon, Aug 26, 2019 at 06:47:34PM +0200, Jiri Olsa escreveu:
> On Mon, Aug 26, 2019 at 12:41:38PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Aug 25, 2019 at 08:17:40PM +0200, Jiri Olsa escreveu:
> > > hi,
> > > as a preparation for sampling libperf interface, moving event
> > > definitions into the library header. Moving just the kernel 
> > > non-AUX events now.
> > > 
> > > In order to keep libperf simple, we switch 'u64/u32/u16/u8'
> > > types used events to their generic '__u*' versions.
> > > 
> > > Perf added 'u*' types mainly to ease up printing __u64 values
> > > as stated in the linux/types.h comment:
> > > 
> > >   /*
> > >    * We define u64 as uint64_t for every architecture
> > >    * so that we can print it with "%"PRIx64 without getting warnings.
> > >    *
> > >    * typedef __u64 u64;
> > >    * typedef __s64 s64;
> > >    */
> > > 
> > > Adding and using new PRI_lu64 and PRI_lx64 macros to be used for
> > > that.  Using extra '_' to ease up the reading and differentiate
> > > them from standard PRI*64 macros.
> > 
> > I think we should take advantage of this moment to rename those structs
> > to have the 'perf_record_' prefix on them, I guess we could even remove
> > the _event from them, i.e.:
> > 
> > 'struct mmap_event' becomes 'perf_record_mmap', as it is the description
> > for the PERF_RECORD_MMAP meta-data event, are you ok with that?
> 
> hum, not sure about loosing the '_event' here, but we are
> not public yet, so we can always change back ;-) I do like
> it'd follow the enum name

So I'm making the already exported to libperf to be renamed to have the
same name as the PERF_RECORD_ enum they map to, just all lowercase.

Looks nice and also makes something exported by libperf to have a perf_
namespace prefix.

BTW: you forgot to move PERF_RECORD_CONTEXT_SWITCH :-)

> > I can go ahead and do it myself, updating each patch on this series to
> > do that.
> 
> sure, I thought we'd do it later, but feel free to do it,
> maybe in separate changes?

I did it as a separate patch, one patch for all the PERF_RECORD_ already
moved to libperf.

Also some other minor stuff, like having that
perf_event.{bpf,ksymbol}_event renamed to play perf_event.{bpf,ksymbol},
like the other records. so to make this idiom more compact and less
redundant:

    event->bpf_event

becomes:

    event->bpf

ditto for ksymbol_event.

- Arnaldo