Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

I hate having to justify why breaking the ABI is unacceptable.


On Mon, 22 Jul 2013, Andi Kleen wrote:

> On Tue, Jul 23, 2013 at 01:40:40AM -0400, Vince Weaver wrote:
> > On Mon, 22 Jul 2013, Andi Kleen wrote:
> > 
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > Add a precise qualifier, like cpu/event=0x3c,precise=1/
> > 
> > So you're adding this to "events/" but not to "format/"?
> 
> Fair point, but note that precise_ip is a bitfield. Since there is no
> way to express that currently the parser would need to special case it anyways.

It breaks the ABI.  The events/* sysfs files are documented as only 
holding values for the bitfields described in format/*.

The perf_event ABI is there for all tools, not to just be a thin shim
for the perf tool.

Even if the maintainers for whatever are going to just ignore the ABI, 
then fine, but then I'd like to see a patch also updating the sysfs
ABI documentation and a patch to the perf_event_open.2 manpage would be 
nice as well.

> > This will break various existing programs.
> 
> Like what?

Why does it matter?  It breaks multiple programs I work on.

A major one is the trinity fuzzer, which parses perf_event sysfs to 
construct valid events.

The code won't segfault because I wrote the parser to be robust, but it 
will spit a warning when invalid junk is put in the sysfs events
files, and I'll get e-mails about it spamming trinity runs with warning 
messages within hours of such a commit hitting linus-git.

I'm a bit grumpy about this because I just finished fixing the fallout 
from your last time breaking the ABI a few weeks ago when your broken code 
started dumping non-hex fields into the sysfs event strings.  I've learned 
now that I have to go over your patches with a fine-tooth code because you 
have no regard for the ABI.

Vince

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Andi Kleen]

On Tue, Jul 23, 2013 at 05:27:43PM -0400, Vince Weaver wrote:
> 
> I hate having to justify why breaking the ABI is unacceptable.

Well it's a testing ABI, so we can do changes to it.

I hope you're not suggesting that perf cannot be extended anymore.
As you know, hardware PMUs are constantly evolving, and perf has evolve
along with them to stay useful.

> It breaks the ABI.  The events/* sysfs files are documented as only 
> holding values for the bitfields described in format/*.

Ok. Need to fix the documentation then for precise=1.  I'll send patches.

Also can add it to format/*, but since it's not in config* it will
be an extension. Since it would be awkward to teach every parser
about all the flags, we could add a "flags" field that is handled 
like config, and contains all the flags.

So format/precise would be 

flags:15-16

on little endian. Looks good?

> I'm a bit grumpy about this because I just finished fixing the fallout 
> from your last time breaking the ABI a few weeks ago when your broken code 
> started dumping non-hex fields into the sysfs event strings.  I've learned 

Not sure what you're talking about?

iirc the only recent sysfs change of mine merged recently was adding
intx/intx_cp, and format files never have had hex numbers in it.

> now that I have to go over your patches with a fine-tooth code because you 
> have no regard for the ABI.

Any useful code review is appreciated.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Sasha Levin]

On 07/23/2013 06:51 PM, Andi Kleen wrote:
> On Tue, Jul 23, 2013 at 05:27:43PM -0400, Vince Weaver wrote:
>> >
>> >I hate having to justify why breaking the ABI is unacceptable.
> Well it's a testing ABI, so we can do changes to it.

The testing ABI has a simple policy about changes:

	The interface can be changed to add new features, but the
	current interface will not break by doing this, unless grave
	errors or security problems are found in them.

It's probably fine to change a testing ABI once in a while, but when things
like trinity start breaking that often due to ABI changes in the same exact
place, that's too much IMO.


Thanks,
Sasha

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Andi Kleen]

On Tue, Jul 23, 2013 at 08:39:09PM -0400, Sasha Levin wrote:
> On 07/23/2013 06:51 PM, Andi Kleen wrote:
> >On Tue, Jul 23, 2013 at 05:27:43PM -0400, Vince Weaver wrote:
> >>>
> >>>I hate having to justify why breaking the ABI is unacceptable.
> >Well it's a testing ABI, so we can do changes to it.
> 
> The testing ABI has a simple policy about changes:
> 
> 	The interface can be changed to add new features, but the
> 	current interface will not break by doing this, unless grave
> 	errors or security problems are found in them.
> 
> It's probably fine to change a testing ABI once in a while, but when things
> like trinity start breaking that often due to ABI changes in the same exact
> place, that's too much IMO.

It sounds like trinity is breaking (well printing a message, not really
breaking) on any addition. So if we follow that the perf sysfs interface
would be completely frozen and can never be extended over today.

I don't think it's a big problem that a test tool needs to be extended
when the software it's testing changes.

If there are enough other widely used programs that actually break from
additions probably would need a v2 of the sysfs interface for extensions
(with new file or directory names), and keep v1 frozen for
compatibility. 

But I don't think that's the case today?

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

On Wed, 24 Jul 2013, Andi Kleen wrote:

> On Tue, Jul 23, 2013 at 08:39:09PM -0400, Sasha Levin wrote:
> It sounds like trinity is breaking (well printing a message, not really
> breaking) on any addition. So if we follow that the perf sysfs interface
> would be completely frozen and can never be extended over today.

Trinity is breaking on any *ABI-breaking* additions.
You can add arbitrary fields to sysfs events/* as long as there's
a corresponding format/* entry.

> I don't think it's a big problem that a test tool needs to be extended
> when the software it's testing changes.

Trinity isn't the only tool that would break, I just picked it as an 
example because it's one that kernel devs have likely heard of.

Vince

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

On Tue, 23 Jul 2013, Sasha Levin wrote:

> It's probably fine to change a testing ABI once in a while, but when things
> like trinity start breaking that often due to ABI changes in the same exact
> place, that's too much IMO.

The problem is they want the ABI to be "whatever our closely-coupled 
userspace perf tool accepts as input" which is an often-changing 
complicated (and undocumented) set of LEX and YACC parsers.

I guess we could just give in and declare that to be the official 
perf sysfs interface in the ABI documentation.  

It's frustrating trying to maintain tools that use the perf_event 
interface because the inclusion of perf in the kernel is used as an excuse 
to constantly break the ABI.

Vince

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

On Wed, 24 Jul 2013, Andi Kleen wrote:

> On Tue, Jul 23, 2013 at 05:27:43PM -0400, Vince Weaver wrote:
> > 

> So format/precise would be 
> 
> flags:15-16
> 
> on little endian. Looks good?

I'd prefer if it were

  precise_ip:0-1

as the code you use to set the value looks like 
   attr->precise_ip=2;
I don't think you can even set bits 15-16 of flags directly, can you?

Althought having to have a parser being able to parse the names of all 50+
perf_event->attr arguments, then parse hex/decimal strings from another
file in sysfs, then trying to map that to binary fields in a complicated
structure that is then passed to a syscall gets a bit stupid at some 
point; this is one of the silliest interfaces I've had to deal with and 
I've done low-level X11 programming before.

Ideally we could do something sane, such as maybe having the event 
descriptions in a library in userspace where it belongs but it's a bit
late to close the barn door now.

> > I'm a bit grumpy about this because I just finished fixing the fallout 
> > from your last time breaking the ABI a few weeks ago when your broken code 
> > started dumping non-hex fields into the sysfs event strings.  I've learned 
> 
> Not sure what you're talking about?

I'm pretty sure it was one of your patches that added the new memory stuff 
which had "ldlat=1" in one of the events, and that was not a hex value 
prefixed by 0x as specified in the ABI documentation and thus broke
various parsers.

By the time I caught this the perf_event maintainers declared it was too 
late to do anything about it.

Vince

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Andi Kleen]

> I'd prefer if it were
> 
>   precise_ip:0-1
> 
> as the code you use to set the value looks like 
>    attr->precise_ip=2;
> I don't think you can even set bits 15-16 of flags directly, can you?

Sorry I meant flags as an alias of "the 64bits currently occupied by the
bitfield". Perhaps the name choice was not very good.

"flags_bitfield" ?

So the tool would only need to know that, not every bit.

In theory it could be also generalized as a byte offset to perf_event,
but that may be overengineered.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

On Wed, 24 Jul 2013, Andi Kleen wrote:

> Sorry I meant flags as an alias of "the 64bits currently occupied by the
> bitfield". Perhaps the name choice was not very good.
> 
> "flags_bitfield" ?
> 
> So the tool would only need to know that, not every bit.
> 
> In theory it could be also generalized as a byte offset to perf_event,
> but that may be overengineered.

I somehow doubt this would be acceptable.  If it were, we could have had a 
somewhat better interface by just having the event fields be a list of 
values without involving format/* at all, something like

   config=0x58034;config1=0x20;precise_ip=0x4

For whatever reason things have to be human readable, and I don't think
just having an opaque 64-bit "flags" value will be accepted.  

I'm likely wrong though, I have a very low accuracy rate for predicting 
future perf_event design decisions.

This is all complicated by the intertwined nature of the perf_event ABI 
and the perf tool, and the way that there's at least three or four 
different ways to specify the same event from the perf tool command line.

Vince

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> On Tue, Jul 23, 2013 at 05:27:43PM -0400, Vince Weaver wrote:
> > 
> > I hate having to justify why breaking the ABI is unacceptable.
> 
> Well it's a testing ABI, so we can do changes to it.
> 
> I hope you're not suggesting that perf cannot be extended anymore.

It obviously should remain extensible, limiting it to 'config' is rather 
stupid. If a parser sees something it cannot parse it should ignore that 
event.

Your feature to export 'precise' requirements on events looks useful to 
me. We could implement it not by special casing it implicitly but by 
saying that if ../format/precise contains something like:

   attr:240-241

then that's a natural extension of the config:X-Y format and should be 
interpreted to mean mean 2 bits in the perf attr field. I.e. we could go 
beyond the config bitfield.

Basically the whole perf_event_attr can be thought of as a 'giant 
bitfield', in which we can specify values to export an enumerated list of 
events from the kernel to tooling.

(Using attr:X-Y the config and config1 variants can be expressed as well, 
as the config fields are inside the attr structure.)

The positions within the perf_attr are an ABI, so this would work pretty 
well.

Thanks,

	Ingo

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Andi Kleen]

> Your feature to export 'precise' requirements on events looks useful to 
> me. We could implement it not by special casing it implicitly but by 
> saying that if ../format/precise contains something like:
> 
>    attr:240-241
> 
> then that's a natural extension of the config:X-Y format and should be 
> interpreted to mean mean 2 bits in the perf attr field. I.e. we could go 
> beyond the config bitfield.
> 
> Basically the whole perf_event_attr can be thought of as a 'giant 
> bitfield', in which we can specify values to export an enumerated list of 
> events from the kernel to tooling.
> 
> (Using attr:X-Y the config and config1 variants can be expressed as well, 
> as the config fields are inside the attr structure.)
> 
> The positions within the perf_attr are an ABI, so this would work pretty 
> well.

Wouldn't we need different bits for each architecture then?
32bit/64bit, some archs with weird alignment rules, maybe different for
BE/LE too?

Ok I suppose it could be somehow auto generated in asm-offsets.c,
although I'm not sure how to get a bitfield offset there.

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> > Your feature to export 'precise' requirements on events looks useful to 
> > me. We could implement it not by special casing it implicitly but by 
> > saying that if ../format/precise contains something like:
> > 
> >    attr:240-241
> > 
> > then that's a natural extension of the config:X-Y format and should be 
> > interpreted to mean mean 2 bits in the perf attr field. I.e. we could go 
> > beyond the config bitfield.
> > 
> > Basically the whole perf_event_attr can be thought of as a 'giant 
> > bitfield', in which we can specify values to export an enumerated list of 
> > events from the kernel to tooling.
> > 
> > (Using attr:X-Y the config and config1 variants can be expressed as well, 
> > as the config fields are inside the attr structure.)
> > 
> > The positions within the perf_attr are an ABI, so this would work pretty 
> > well.
> 
> Wouldn't we need different bits for each architecture then? 32bit/64bit, 
> some archs with weird alignment rules, maybe different for BE/LE too?
> 
> Ok I suppose it could be somehow auto generated in asm-offsets.c, 
> although I'm not sure how to get a bitfield offset there.

That, or we could indeed start adding specific field names as well, which 
would have a natural position and order.

Thanks,

	Ingo

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Peter Zijlstra]

On Thu, Sep 12, 2013 at 07:36:17PM +0200, Andi Kleen wrote:
> > Your feature to export 'precise' requirements on events looks useful to 
> > me. We could implement it not by special casing it implicitly but by 
> > saying that if ../format/precise contains something like:
> > 
> >    attr:240-241

Since we currently have the pattern $name:bits to mean
perf_event_attr::$name the above would imply and create a possible
collision with perf_event_attr::attr.

If we're going to do this I'd propose using something like _:240-241,
for while '_' is a valid name in C its not something we're ever going to
allow in perf_event_attr.

> > then that's a natural extension of the config:X-Y format and should be 
> > interpreted to mean mean 2 bits in the perf attr field. I.e. we could go 
> > beyond the config bitfield.
> > 
> > Basically the whole perf_event_attr can be thought of as a 'giant 
> > bitfield', in which we can specify values to export an enumerated list of 
> > events from the kernel to tooling.
> > 
> > (Using attr:X-Y the config and config1 variants can be expressed as well, 
> > as the config fields are inside the attr structure.)
> > 
> > The positions within the perf_attr are an ABI, so this would work pretty 
> > well.
> 
> Wouldn't we need different bits for each architecture then?
> 32bit/64bit, some archs with weird alignment rules, maybe different for
> BE/LE too?

Typically PMU drivers are per arch and all the format stuff is per pmu
driver so I'd not worry about that just yet.

But yes, while the perf_event_attr thing is ABI its not identical across
archs.

> Ok I suppose it could be somehow auto generated in asm-offsets.c,
> although I'm not sure how to get a bitfield offset there.

Yes, that is an unfortunate situation. I (and either Acme or Jolsa)
tried wrapping the bitfield in an anonymous union to create a named
variable for the entire u64 but older GCC completely fails with that.

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Sep 12, 2013 at 07:36:17PM +0200, Andi Kleen wrote:
> > > Your feature to export 'precise' requirements on events looks useful to 
> > > me. We could implement it not by special casing it implicitly but by 
> > > saying that if ../format/precise contains something like:
> > > 
> > >    attr:240-241
> 
> Since we currently have the pattern $name:bits to mean 
> perf_event_attr::$name the above would imply and create a possible 
> collision with perf_event_attr::attr.
> 
> If we're going to do this I'd propose using something like _:240-241, 
> for while '_' is a valid name in C its not something we're ever going to 
> allow in perf_event_attr.

Ok - and I'm not against adding individual 'names' one by one as well, 
that allows us to expose only the fields that relate to event 
configuration.

For example if we added 'type' as well we could expose the generic, 
hardware-independent events via sysfs as well.

( Eventually this scheme would be fit to expose more advanced events as
  well: such as composite groups of events with simple arithmetic 
  operations between them. That would allow the exposure of E1+E2-E3 type 
  of simple calculations. )

> > Wouldn't we need different bits for each architecture then? 
> > 32bit/64bit, some archs with weird alignment rules, maybe different 
> > for BE/LE too?
> 
> Typically PMU drivers are per arch and all the format stuff is per pmu 
> driver so I'd not worry about that just yet.

ok.

> But yes, while the perf_event_attr thing is ABI its not identical across 
> archs.

Yeah, like syscalls - it's not an on-disk format.

> > Ok I suppose it could be somehow auto generated in asm-offsets.c, 
> > although I'm not sure how to get a bitfield offset there.
> 
> Yes, that is an unfortunate situation. I (and either Acme or Jolsa) 
> tried wrapping the bitfield in an anonymous union to create a named 
> variable for the entire u64 but older GCC completely fails with that.

We could be careful with bitfields and enumerate their offsets explicitly, 
with a build time testcase that makes sure that the offsets match reality.

Thanks,

	Ingo

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Peter Zijlstra]

On Fri, Sep 13, 2013 at 11:50:57AM +0200, Ingo Molnar wrote:
> For example if we added 'type' as well we could expose the generic, 
> hardware-independent events via sysfs as well.

Type is already fully implied by where you'll find the event in sysfs:

  /sys/bus/event_sources/devices/$PMU/events/

needs

  perf_event_attr::type := /sys/bus/event_sources/devices/$PMU/type

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

On Fri, 13 Sep 2013, Peter Zijlstra wrote:

> On Fri, Sep 13, 2013 at 11:50:57AM +0200, Ingo Molnar wrote:
> > For example if we added 'type' as well we could expose the generic, 
> > hardware-independent events via sysfs as well.
> 
> Type is already fully implied by where you'll find the event in sysfs:
> 
>   /sys/bus/event_sources/devices/$PMU/events/
> 
> needs
> 
>   perf_event_attr::type := /sys/bus/event_sources/devices/$PMU/type


OK, fine, another question then.

Is there any reason these values have to be human-readable?

The only reason you are using this crazy format I can see is because it 
makes maintaining your personal userspace implementation (perf) easier at 
the expense of everyone else who want to use this interface.

Honestly, an interface like
cat   /sys/bus/event_sources/devices/$PMU/events/new_event

   size=320,0xdeadbeef,0xcafef00d,....,0x000000

when you just set up an array, copy in the values, then memcpy() it into
place on top of a struct attr is a million times easier than what you are 
propsing:
   1. A huge complicated LEX/YACC parser
   2. The parser has to read in many different files under ../format/..
      and build up a tree of names and shift/masks
   3. The event is read in and then text has to be parsed, values read,
      and then shifting-masking to get a value for each register
   4. A mapping has to be in the code of the various (of the over 40+)
      fields in the struct perf_attr field, and each value has to
      be put at the proper offset
   5. If ever a new field is added to struct perf_attr, then any event
      using it breaks until your parser is updated with all the info
      about this field.

It's huge, takes up non-swappable kernel mem with lots of individual sysfs
files, requires a complex parser for what should be just a simple
config setup, and is fragile when new fields are added.

But of course since perf is tightly coupled into the kernel source tree 
you can get away with it.  I guess I should just be glad you aren't 
exporting it as XML or something.

Vince

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Andi Kleen]

> Typically PMU drivers are per arch and all the format stuff is per pmu
> driver so I'd not worry about that just yet.

That's a good point. So hard coded numbers would be fine for now.

> Yes, that is an unfortunate situation. I (and either Acme or Jolsa)
> tried wrapping the bitfield in an anonymous union to create a named
> variable for the entire u64 but older GCC completely fails with that.

In theory it's possible by parsing readelf output.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

[Vince Weaver]

On Thu, 12 Sep 2013, Ingo Molnar wrote:
 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > On Tue, Jul 23, 2013 at 05:27:43PM -0400, Vince Weaver wrote:
> > > 
> > > I hate having to justify why breaking the ABI is unacceptable.
> > 
> > Well it's a testing ABI, so we can do changes to it.
> > 
> > I hope you're not suggesting that perf cannot be extended anymore.
> 
> It obviously should remain extensible, limiting it to 'config' is rather 
> stupid. If a parser sees something it cannot parse it should ignore that 
> event.

Well 

     Documentation/ABI/testing/sysfs-bus-event_source-devices-events

implies that you won't find field names that aren't in a format file, and

     Documentation/ABI/testing/sysfs-bus-event_source-devices-format

specifies that only "config" fields will appear there.

So if this will be changing you might want to update the documentation, or 
just remove it altogether because it is getting increasingly inaccurate.

I've tried getting this documentation updated
  ( https://lkml.org/lkml/2013/7/25/2 ) 
but the patches have been ignored.



In any case, I notice perf also uses the "open" syscall, so I look forward 
to your eventual creation of
  /sys/bus/open/file_permissions/*

$ cat /sys/bus/open/file_permissions/write
flags=0101,mode=0666

Because what every system call needs is some sort of tree under sysfs
exporting various common settings in unswappable kernel memory that 
require a complex LEX/YACC parser to interpret.  

That's much simpler than using a library, or having a user looking 
things up in a manpage and placing a few comments in their code.

Vince