lkml.org 
[lkml]   [2019]   [Mar]   [20]   [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 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?

> > 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...

thanks,

greg k-h

\
 
 \ /
  Last update: 2019-03-20 17:04    [W:0.874 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site