Messages in this thread | | | Date | Thu, 21 Mar 2019 12:09:01 +0100 | From | Jiri Olsa <> | Subject | Re: [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
| |