Messages in this thread | | | Date | Wed, 1 Feb 2012 14:13:40 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH 5/9] perf: Adding sysfs group format attribute for pmu device |
| |
On Tue, Jan 31, 2012 at 05:25:26PM -0800, Corey Ashford wrote: > On 01/30/2012 01:52 AM, Jiri Olsa wrote: > > On Fri, Jan 27, 2012 at 01:08:38PM -0800, Corey Ashford wrote: > >> On 01/27/2012 06:34 AM, Jiri Olsa wrote: > >>> Adding sysfs group 'format' attribute for pmu device that > >>> contains a syntax description on how to construct raw events. > >>>
SNIP
> >> This might help the user understand which fields go together. I'm not > >> sold on the .1 syntax... you could do it as <dev>.<event-group-name>/ or > >> <dev>/<event-group-name>/... or whatever seems to make the most sense > >> and is relatively easy to implement and use. > > > > Though I'm not sure we want allow separate devices inside single pmu, > > I think we could have multiple format groups if necessary :) > > > > some quick ideas: > > > > 1) having format group attribute under format like: > > <dev>/format/group1/.. > > <dev>/format/group2/.. > > <dev>/format/group2/.. > > ... > > > > 2) having format group name within the format attribute name like: > > <dev>/format/group1-krava1 > > <dev>/format/group1-krava2 > > <dev>/format/group2-krava3 > > ... > > > > 3) having group name inside the foramt attributes like: > > cat <dev>/format/group1-krava1 > > group1 config:0-1,62-63 > > > > > > I think I like the most ad 1).. > > > > We could have something like default format directory if there's > > only a single format group, like: > > <dev>/format/default/krava1 > > <dev>/format/default/krava2 > > ... > > > > The perf event syntax could have something like '::' to classify > > format attribute with a group like (none would go to default dir): > > > > cpu/group1::config=1,group2::config1=2,config2=3/u > > The "[::<group>|]" syntax is good. > > Are you are suggesting that a single event could use multiple groups > because they may share some common fields, such as the event code? If > so, I think that might be confusing. I think it would be better to > have every group fully lay out the bits in the config{,1,2} fields so > that you only need to specify one group per event, even if that leads to > some redundancy (e.g. group1..n all have an eventcode field.)
ok, it'd be the 'cpu::group1/config=1,config1=2,config2=3/u' then..
but let's see what Peter thinks about this, since he first suggested to 'fix' this by having separate pmu drivers.. not format groups :)
> > Something I missed before is that your config sysfs attribute group can > look like: > > config1:0-1,62-63 > > How does the user specify a value for these two bit fields, or does he > concatenate the bit fields together, and perf will split it apart again? > e.g. cpu/group1::config1=1,2/u ?
user just set the value for the field and perf makes sure it is spread over the config[12] bits as configured
so as for your example, following format definiton: # cat <pmu>/format/krava config1:0-1,62-63
with user settings: cpu/krava=15/u
fills bits 0,1,62,63 of config1 with 1s
> > > > or > > cpu::group1/config=1,config1=2,config2=3/u > > > > > > Or we could say the format field names could not overlap and then > > we dont need to specify group at all :) It'd be just for user's > > awareness.. > > perf would then "want" to check for overlap and also for fields coming > from different groups. In some cases, I think you'd want to have the > same name for a field, but have the field a different size, or perhaps a > different interpretation. For example "busid" might be a desirable > name for fields in two different event groups, but their sizes and > position are different. Of course the quick fix is to name them > uniquely, but since they are in subdirectories, it isn't really obvious > that the names have to be unique. > > One other comment that occurs to me is that it would be nice for perf to > know when a supplied value is out of range, or will have undefined > results. For example, a field may be 8 bits wide, but not all 8-bit > values are legal. For example, there may be 208 events, and the codes > may be somehwhat or even very sparsely encoded. So, ideally, a config > field in sysfs might look like this: > > config1:0-7:0x0-0xd8,0xdb-0xe2,0xe4-0xe6 > > This way perf could check for valid values before stuffing them into > registers, and give a good error message to the user. If there is no > restriction field, it would be assumed all of the possible values are valid. > > I think the kernel code needs to check for bad values as well, because > people can bypass the restrictions exposed by sysfs and use the > perf_events API directly.
ok, I think some kind of such restriction could be added as optional ':' behind the current format if needed.. np
thanks, jirka
| |