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

[Jiri Olsa <jolsa@redhat.com>]

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