linux-kernel - Re: [PATCH v3 08/12] perf, persistent: Exposing persistent events using sysfs

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]

Message-ID: <alpine.DEB.2.02.1308221344500.26486@pianoman.cluster.toy>
Date:	Thu, 22 Aug 2013 14:00:51 -0400 (EDT)
From:	Vince Weaver <vince@...ter.net>
To:	Robert Richter <rric@...nel.org>
cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Borislav Petkov <bp@...en8.de>, Jiri Olsa <jolsa@...hat.com>,
	linux-kernel@...r.kernel.org,
	Robert Richter <robert.richter@...aro.org>,
	Namhyung Kim <namhyung@...nel.org>
Subject: Re: [PATCH v3 08/12] perf, persistent: Exposing persistent events
 using sysfs

On Thu, 22 Aug 2013, Robert Richter wrote:

> From: Robert Richter <robert.richter@...aro.org>
> 
> Expose persistent events in the system to userland using sysfs. Perf
> tools are able to read existing pmu events from sysfs. Now we use a
> persistent pmu as an event container containing all registered
> persistent events of the system. This patch adds dynamically
> registration of persistent events to sysfs. E.g. something like this:
> 
>  /sys/bus/event_source/devices/persistent/events/mce_record:persistent,config=106
>  /sys/bus/event_source/devices/persistent/format/persistent:attr5:23

...

> @@ -15,6 +16,26 @@ Contact:	Jiri Olsa <jolsa@...hat.com>
>  			attr2 = 'config:0-7'
>  			attr3 = 'config:12-35'
>  
> -		Example: 'config1:1,6-10,44'
> -		Defines contents of attribute that occupies bits 1,6-10,44 of
> -		perf_event_attr::config1.
> +		Syntax			Description
> +
> +		config[012]*:<bits>	Each attribute of this group
> +					defines the 'hardware' bitmask
> +					we want to export, so that
> +					userspace can deal with sane
> +					name/value pairs.
> +
> +		attr<index>:<bits>	Set any field of the event
> +					attribute. The index is a
> +					decimal number that specifies
> +					the u64 value to be set within
> +					struct perf_event_attr.

Ugh this is ugly.  You might also want to specify that the "index"
value starts at 0 which threw me for a bit when I was trying to figure
out what was going on.  You might also want to clarify the previous
part of the document which uses "attr" to mean something else.

Is this endian clean?  Will attr5:23 point to the same bit on a big-endian 
machine as on little-endian?

If we're going to have to have an ugly interface like this we might
as well do something more human readable, since anything that parses this 
is going to have to rebuild the struct perf_event_attr by hand anyway
(unless you propose people blindly set bits at offsets using pointer math
which just sounds like a bad idea).

For example, just give up and let someone specify the actual field name
like "persistent" and have the tools handle that. 

/sys/bus/event_source/devices/persistent/events/mce_record
   persistent,config=106

/sys/bus/event_source/devices/persistent/format/persistent
    attr_persistent

That way you could also add things like
/sys/bus/event_source/devices/persistent/format/precise_ip
    attr_precise_ip:0-1

Although I still think exposing the full huge attr stuct via sysfs is just 
silly.  Isn't there some better way?  I'm not aware of any other syscall
that exports things like this.

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/