| Message ID | 1469742143-22245-2-git-send-email-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: > This patch somewhat mimics the work done on address filters to > add the infrastructure needed to pass PMU specific HW > configuration to the driver before a session starts. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > index c66a485a24ac..90fbc5fd3925 100644 > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -407,6 +407,7 @@ struct perf_event_attr { > #define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *) > #define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32) > #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32) > +#define PERF_EVENT_IOC_SET_DRV_CONFIGS _IOW('$', 10, char *) Please also do a manpages patch.
On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: > This patch somewhat mimics the work done on address filters to > add the infrastructure needed to pass PMU specific HW > configuration to the driver before a session starts. I'm thinking we want to specify a syntax and validate the string matches the syntax in the generic code.
On 4 August 2016 at 10:58, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: >> This patch somewhat mimics the work done on address filters to >> add the infrastructure needed to pass PMU specific HW >> configuration to the driver before a session starts. >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h >> index c66a485a24ac..90fbc5fd3925 100644 >> --- a/include/uapi/linux/perf_event.h >> +++ b/include/uapi/linux/perf_event.h >> @@ -407,6 +407,7 @@ struct perf_event_attr { >> #define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *) >> #define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32) >> #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32) >> +#define PERF_EVENT_IOC_SET_DRV_CONFIGS _IOW('$', 10, char *) > > Please also do a manpages patch. Patch 3/6 in this set documents the new option (tools/perf/Documentation/perf-record.tx). Is this what you were looking for? If not please expand on the information you want to see added add and where. Thanks, Mathieu
On 4 August 2016 at 10:59, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: >> This patch somewhat mimics the work done on address filters to >> add the infrastructure needed to pass PMU specific HW >> configuration to the driver before a session starts. > > I'm thinking we want to specify a syntax and validate the string matches > the syntax in the generic code. The syntax is checked in the lexer making sure that nothing other than @cfg or @cfg=config is sent to the kernel. From there validation is done in the PMU driver that implements the set_drv_configs() interface. I am not sure to get you point here - can I ask you to provide more details? Thanks, Mathieu
On Fri, Aug 05, 2016 at 09:35:05AM -0600, Mathieu Poirier wrote: > On 4 August 2016 at 10:58, Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: > >> This patch somewhat mimics the work done on address filters to > >> add the infrastructure needed to pass PMU specific HW > >> configuration to the driver before a session starts. > >> > >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > >> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > >> index c66a485a24ac..90fbc5fd3925 100644 > >> --- a/include/uapi/linux/perf_event.h > >> +++ b/include/uapi/linux/perf_event.h > >> @@ -407,6 +407,7 @@ struct perf_event_attr { > >> #define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *) > >> #define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32) > >> #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32) > >> +#define PERF_EVENT_IOC_SET_DRV_CONFIGS _IOW('$', 10, char *) > > > > Please also do a manpages patch. > > Patch 3/6 in this set documents the new option > (tools/perf/Documentation/perf-record.tx). Is this what you were > looking for? If not please expand on the information you want to see > added add and where. Since you add an IOCTL (with preferably more structure than present in this patch, see the other email) this needs to be documented in the syscall manpage. http://git.kernel.org/cgit/docs/man-pages/man-pages.git/tree/man2/perf_event_open.2 http://www.man7.org/linux/man-pages/man2/perf_event_open.2.html
On Fri, Aug 05, 2016 at 09:41:15AM -0600, Mathieu Poirier wrote: > On 4 August 2016 at 10:59, Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: > >> This patch somewhat mimics the work done on address filters to > >> add the infrastructure needed to pass PMU specific HW > >> configuration to the driver before a session starts. > > > > I'm thinking we want to specify a syntax and validate the string matches > > the syntax in the generic code. > > The syntax is checked in the lexer making sure that nothing other than > @cfg or @cfg=config is sent to the kernel. From there validation is > done in the PMU driver that implements the set_drv_configs() > interface. > > I am not sure to get you point here - can I ask you to provide more details? What keeps random userspace from sending malformed strings in? There's more userspace than just the perf tool. The kernel needs to validate structure.
On 5 August 2016 at 09:53, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Aug 05, 2016 at 09:35:05AM -0600, Mathieu Poirier wrote: >> On 4 August 2016 at 10:58, Peter Zijlstra <peterz@infradead.org> wrote: >> > On Thu, Jul 28, 2016 at 03:42:18PM -0600, Mathieu Poirier wrote: >> >> This patch somewhat mimics the work done on address filters to >> >> add the infrastructure needed to pass PMU specific HW >> >> configuration to the driver before a session starts. >> >> >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> > >> >> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h >> >> index c66a485a24ac..90fbc5fd3925 100644 >> >> --- a/include/uapi/linux/perf_event.h >> >> +++ b/include/uapi/linux/perf_event.h >> >> @@ -407,6 +407,7 @@ struct perf_event_attr { >> >> #define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *) >> >> #define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32) >> >> #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32) >> >> +#define PERF_EVENT_IOC_SET_DRV_CONFIGS _IOW('$', 10, char *) >> > >> > Please also do a manpages patch. >> >> Patch 3/6 in this set documents the new option >> (tools/perf/Documentation/perf-record.tx). Is this what you were >> looking for? If not please expand on the information you want to see >> added add and where. > > Since you add an IOCTL (with preferably more structure than present in > this patch, see the other email) this needs to be documented in the > syscall manpage. > > http://git.kernel.org/cgit/docs/man-pages/man-pages.git/tree/man2/perf_event_open.2 > > http://www.man7.org/linux/man-pages/man2/perf_event_open.2.html After a little bit of digging around I understand that manpages have to be written _after_ the new ioctl call has been added - at least that's what I deduce when looking at what Vince Weaver did for the BPF support: commit b0f7b411bed0505937f0f51d6499d0c6c56f4b8c Author: Vince Weaver <vincent.weaver@maine.edu> Date: Thu Jul 23 13:10:21 2015 -0400 perf_event_open.2: 4.1 PERF_EVENT_IOC_SET_BPF support This manpage patch relates to the addition of the PERF_EVENT_IOC_SET_BPF ioctl in the following commit: commit 2541517c32be2531e0da59dfd7efc1ce844644f5 Author: Alexei Starovoitov <ast@plumgrid.com> tracing, perf: Implement BPF programs attached to kprobes Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> Reviewed-by: Steven Rostedt <rostedt@goodmis.org> Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Arnaldo Carvalho de Melo <acme@infradead.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: David S. Miller <davem@davemloft.net> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Peter Zijlstra <peterz@infradead.org> Link: http://lkml.kernel.org/r/1427312966-8434-4-git-send-email-ast@plumgrid.com Signed-off-by: Ingo Molnar <mingo@kernel.org> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu> Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com> Am I correct here or you want to proceed differently? Thanks for the guidance, Mathieu
On Wed, 10 Aug 2016, Mathieu Poirier wrote: > > After a little bit of digging around I understand that manpages have > to be written _after_ the new ioctl call has been added - at least > that's what I deduce when looking at what Vince Weaver did for the BPF > support: The manpage patch doesn't get applied until a feature hits a release kernel (because sometimes features are backed out at the last minute). It is good to have a manpage update ready at the time of your initial patch submission though for a variety of reasons 1. It helps the patch reviewers see what ABI change is being proposed 2. It is better to catch mistakes in the ABI early rather than trying to fix them after it's hit a released kernel and it's too late 3. By the time the kernel is released, you might have forgotten details about the change, or moved on to other things, and then it's up to me to try to figure out the purpose of the change and unfortunately this isn't always obvious from the git commit logs. Vince
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 7921f4f20a58..59d61a12cf9d 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -168,6 +168,9 @@ struct hw_perf_event { /* Last sync'ed generation of filters */ unsigned long addr_filters_gen; + /* HW specific configuration */ + void *drv_configs; + /* * hw_perf_event::state flags; used to track the PERF_EF_* state. */ @@ -442,6 +445,12 @@ struct pmu { * Filter events for PMU-specific reasons. */ int (*filter_match) (struct perf_event *event); /* optional */ + + /* + * PMU driver specific configuration. + */ + int (*set_drv_configs) (struct perf_event *event, + void __user *arg); /* optional */ }; /** diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index c66a485a24ac..90fbc5fd3925 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -407,6 +407,7 @@ struct perf_event_attr { #define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *) #define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32) #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32) +#define PERF_EVENT_IOC_SET_DRV_CONFIGS _IOW('$', 10, char *) enum perf_event_ioc_flags { PERF_IOC_FLAG_GROUP = 1U << 0, diff --git a/kernel/events/core.c b/kernel/events/core.c index 79dae188a987..9208e6ec036f 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -4457,6 +4457,8 @@ static int perf_event_set_output(struct perf_event *event, struct perf_event *output_event); static int perf_event_set_filter(struct perf_event *event, void __user *arg); static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd); +static int perf_event_set_drv_configs(struct perf_event *event, + void __user *arg); static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned long arg) { @@ -4526,6 +4528,10 @@ static long _perf_ioctl(struct perf_event *event, unsigned int cmd, unsigned lon rcu_read_unlock(); return 0; } + + case PERF_EVENT_IOC_SET_DRV_CONFIGS: + return perf_event_set_drv_configs(event, (void __user *)arg); + default: return -ENOTTY; } @@ -4558,6 +4564,7 @@ static long perf_compat_ioctl(struct file *file, unsigned int cmd, switch (_IOC_NR(cmd)) { case _IOC_NR(PERF_EVENT_IOC_SET_FILTER): case _IOC_NR(PERF_EVENT_IOC_ID): + case _IOC_NR(PERF_EVENT_IOC_SET_DRV_CONFIGS): /* Fix up pointer size (usually 4 -> 8 in 32-on-64-bit case */ if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) { cmd &= ~IOCSIZE_MASK; @@ -7633,6 +7640,15 @@ void perf_bp_event(struct perf_event *bp, void *data) } #endif +static int perf_event_set_drv_configs(struct perf_event *event, + void __user *arg) +{ + if (!event->pmu->set_drv_configs) + return -EINVAL; + + return event->pmu->set_drv_configs(event, arg); +} + /* * Allocate a new address filter */ diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h index c66a485a24ac..90fbc5fd3925 100644 --- a/tools/include/uapi/linux/perf_event.h +++ b/tools/include/uapi/linux/perf_event.h @@ -407,6 +407,7 @@ struct perf_event_attr { #define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *) #define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32) #define PERF_EVENT_IOC_PAUSE_OUTPUT _IOW('$', 9, __u32) +#define PERF_EVENT_IOC_SET_DRV_CONFIGS _IOW('$', 10, char *) enum perf_event_ioc_flags { PERF_IOC_FLAG_GROUP = 1U << 0,
This patch somewhat mimics the work done on address filters to add the infrastructure needed to pass PMU specific HW configuration to the driver before a session starts. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- include/linux/perf_event.h | 9 +++++++++ include/uapi/linux/perf_event.h | 1 + kernel/events/core.c | 16 ++++++++++++++++ tools/include/uapi/linux/perf_event.h | 1 + 4 files changed, 27 insertions(+)