Messages in this thread | | | Date | Wed, 20 Mar 2019 16:48:33 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH 1/8] perf/x86: Add msr probe interface |
| |
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; > 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.
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.
> Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > arch/x86/events/Makefile | 2 +- > arch/x86/events/probe.c | 36 ++++++++++++++++++++++++++++++++++++ > arch/x86/events/probe.h | 22 ++++++++++++++++++++++ > 3 files changed, 59 insertions(+), 1 deletion(-) > create mode 100644 arch/x86/events/probe.c > create mode 100644 arch/x86/events/probe.h > > diff --git a/arch/x86/events/Makefile b/arch/x86/events/Makefile > index b8ccdb5c9244..ec29a466444a 100644 > --- a/arch/x86/events/Makefile > +++ b/arch/x86/events/Makefile > @@ -1,4 +1,4 @@ > -obj-y += core.o > +obj-y += core.o probe.o > obj-y += amd/ > obj-$(CONFIG_X86_LOCAL_APIC) += msr.o > obj-$(CONFIG_CPU_SUP_INTEL) += intel/ > diff --git a/arch/x86/events/probe.c b/arch/x86/events/probe.c > new file mode 100644 > index 000000000000..0052b730c55e > --- /dev/null > +++ b/arch/x86/events/probe.c > @@ -0,0 +1,36 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/export.h> > +#include <linux/bits.h> > +#include "probe.h" > + > +unsigned long > +perf_msr_probe(struct perf_msr *msr, int cnt, > + struct attribute **attrs, void *data) > +{ > + unsigned long avail = 0; > + unsigned int bit; > + u64 val; > + > + if (cnt >= BITS_PER_LONG) > + return 0; > + > + for (bit = 0; bit < cnt; bit++) { > + struct attribute **a = msr[bit].attrs; > + > + if (!msr[bit].no_check) { > + if (msr[bit].test && !msr[bit].test(bit, data)) > + continue; > + if (rdmsrl_safe(msr[bit].msr, &val) || !val) > + continue; > + } > + > + while (*a) > + *attrs++ = *a++; > + > + avail |= bit; > + } > + > + *attrs = NULL; > + return avail; > +} > +EXPORT_SYMBOL_GPL(perf_msr_probe); > diff --git a/arch/x86/events/probe.h b/arch/x86/events/probe.h > new file mode 100644 > index 000000000000..42dd666533c3 > --- /dev/null > +++ b/arch/x86/events/probe.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ARCH_X86_EVENTS_PROBE_H__ > +#define __ARCH_X86_EVENTS_PROBE_H__ > +#include <linux/sysfs.h> > + > +#define MSR_ATTR(__n) \ > +static struct attribute *msr_##__n[] = { \ > + &__n.attr.attr, \ > + NULL, \ > +} > + > +struct perf_msr { > + u64 msr; > + struct attribute **attrs; > + bool (*test)(int idx, void *data); > + bool no_check; > +}; > + > +unsigned long > +perf_msr_probe(struct perf_msr *msr, int cnt, > + struct attribute **attrs, void *data); > +#endif /* __ARCH_X86_EVENTS_PROBE_H__ */ > -- > 2.17.2 >
| |