lkml.org 
[lkml]   [2019]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/8] perf/x86: Add msr probe interface
On Wed, Mar 20, 2019 at 05:03:29PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 20, 2019 at 04:48:33PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 18, 2019 at 07:21:09PM +0100, Jiri Olsa wrote:
> > > Adding perf_msr_probe function to provide interface for
> > > checking up on MSR register and add its related event
> > > attributes if it passes the check.
> > >
> > > User defines following struct for each MSR register:
> > >
> > > struct perf_msr {
> > > u64 msr;
> > > struct attribute **attrs;
>
> Please use attribute groups where ever possible. I've been working to
> fix up the remaining places that use list of attributes as that is not
> flexible at all (and broken in places.)
>
> And this is a device, so why not device attributes?

ok, will check

>
> > > bool (*test)(int idx, void *data);
> > > bool no_check;
> > > };
> > >
> > > Where:
> > > msr - is the MSR address
> > > attrs - is attributes array to add if the check passed
> > > test - is test function pointer
> > > no_check - is bool that bypass the check and adds the
> > > attribute without any test
> > >
> > > The array of struct perf_msr is passed into:
> > >
> > > perf_msr_probe(struct perf_msr *msr, int cnt,
> > > struct attribute **attrs, void *data)
> > >
> > > Together with:
> > > cnt - which is the number of struct msr array elements
> > > attrs - which is an array placeholder for added attributes
> > > and needs to be big enough
> > > data -which is user pointer passed to the test function
> > >
> > > The perf_msr_probe will executed test code, read the MSR and
> > > check the value is != 0. If all these tests pass, related
> > > attributes are added into attrs array.
> > >
> > > Also adding MSR_ATTR macro helper to define attribute array
> > > from single attribute. It will be used in following patches.
>
> Please no, don't we have enough ATTR macros? Why do you need another
> one? What are you trying to save code on?
>
> > Somewhere along the line you lost the explanation of _why_ we're doing
> > this; namely: virt sucks.
> >
> > Also, recently GregKH had a chance to look at perf code and we scored
> > fairly high on the WTF'o'meter for what we're doing with the attribute
> > stuff.
> >
> > He pointed me to sysfs attribute_group::is_visible functions to replace
> > some of our 'creative' code.
>
> Yes, that would be very good to do. If no one is working on it, I can
> take a look next week as I have long plane rides...

if I dont send v2 till then, it's all yours ;-)

thanks,
jirka

\
 
 \ /
  Last update: 2019-03-21 12:10    [W:0.040 / U:1.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site