Re: [PATCH v3 08/12] perf, persistent: Exposing persistent eventsusing sysfs

[Vince Weaver]

On Thu, 22 Aug 2013, Robert Richter wrote:

> From: Robert Richter <robert.richter@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
>  			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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/