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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 13 Sep 2013 10:31:59 -0400 (EDT)
From:	Vince Weaver <vincent.weaver@...ne.edu>
To:	Peter Zijlstra <peterz@...radead.org>
cc:	Ingo Molnar <mingo@...nel.org>, Andi Kleen <andi@...stfloor.org>,
	Vince Weaver <vince@...ter.net>,
	Andi Kleen <ak@...ux.intel.com>, acme@...radead.org,
	linux-kernel@...r.kernel.org,
	Stephane Eranian <eranian@...gle.com>,
	torvalds@...ux-foundation.org, trinity@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Jiri Olsa <jolsa@...hat.com>, Namhyung Kim <namhyung@...il.com>
Subject: Re: [PATCH 1/2] tools, perf: Add a precise event qualifier v2

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/