Re: [PATCH 1/8] perf/x86: Add msr probe interface

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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