[RFC,2/2] perf: Marker software event and ioctl

Message ID 1410522513-1045-3-git-send-email-pawel.moll@arm.com
State New
Headers show

Commit Message

Pawel Moll Sept. 12, 2014, 11:48 a.m.
This patch adds a PERF_COUNT_SW_MARKER event type, which
can be requested by user and a PERF_EVENT_IOC_MARKER
ioctl command which will inject an event of said type into
the perf buffer. The ioctl can take a zero-terminated
string argument, similar to tracing_marker in ftrace,
which will be kept in the "raw" field of the sample.

The main use case for this is synchronisation of
performance data generated in user space with the perf
stream coming from the kernel. For example, the marker
can be inserted by a JIT engine after it generated
portion of the code, but before the code is executed
for the first time, allowing the post-processor to
pick the correct debugging information. Other example
is a system profiling tool taking data from other
sources than just perf, which generates a marker
at the beginning at at the end of the session
(also possibly periodically during the session) to
synchronise kernel timestamps with clock values
obtained in userspace (gtod or raw_monotonic).

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 include/uapi/linux/perf_event.h |  2 ++
 kernel/events/core.c            | 43 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

Comments

Christopher Covington Sept. 12, 2014, 12:43 p.m. | #1
Hi Pawel,

On 09/12/2014 07:48 AM, Pawel Moll wrote:
> This patch adds a PERF_COUNT_SW_MARKER event type, which
> can be requested by user and a PERF_EVENT_IOC_MARKER
> ioctl command which will inject an event of said type into
> the perf buffer. The ioctl can take a zero-terminated
> string argument, similar to tracing_marker in ftrace,
> which will be kept in the "raw" field of the sample.
> 
> The main use case for this is synchronisation of
> performance data generated in user space with the perf
> stream coming from the kernel. For example, the marker
> can be inserted by a JIT engine after it generated
> portion of the code, but before the code is executed
> for the first time, allowing the post-processor to
> pick the correct debugging information. Other example
> is a system profiling tool taking data from other
> sources than just perf, which generates a marker
> at the beginning at at the end of the session
> (also possibly periodically during the session) to
> synchronise kernel timestamps with clock values
> obtained in userspace (gtod or raw_monotonic).

> @@ -5960,6 +5965,44 @@ static struct pmu perf_swevent = {
>  	.event_idx	= perf_swevent_event_idx,
>  };
>  
> +static int perf_sw_event_marker(struct perf_event *event, char __user *arg)
> +{
> +	struct perf_sample_data data;
> +	struct pt_regs *regs = current_pt_regs();
> +	struct perf_raw_record raw = { 0, };
> +
> +	if (!static_key_false(&perf_swevent_enabled[PERF_COUNT_SW_MARKER]))
> +		return 0;
> +
> +	perf_sample_data_init(&data, 0, 0);
> +
> +	if (arg) {
> +		long len = strnlen_user(arg, PAGE_SIZE);

Just to ask the dumb questions in case the answers I've come up with are
wrong: What is PAGE_SIZE on an arm64 kernel? How does userspace know?

Thanks,
Christopher
Pawel Moll Sept. 12, 2014, 12:57 p.m. | #2
On Fri, 2014-09-12 at 13:43 +0100, Christopher Covington wrote:
> Just to ask the dumb questions in case the answers I've come up with are
> wrong: What is PAGE_SIZE on an arm64 kernel? 

It's either 4 or 64k, depending on CONFIG_ARM64_64K_PAGES.

> How does userspace know?
> 
#include <unistd.h>
#include <stdio.h>

int main(void)
{
	printf("%ld\n", sysconf(_SC_PAGESIZE));
	return 0;
}

Now a word of explanation. The PAGE_SIZE limitation was shamelessly
stolen from perf_event_set_filter() (so PERF_EVENT_IOC_SET_FILTER) as an
attempt to address a problem of passing a zero-terminated string from
userspace. Simply speaking - there must be some limitation, and a page
size seem as good as any other. I have strong doubts about this myself,
so all alternative ideas are more than welcome.

As I mentioned in the cover letter, maybe this simply shouldn't be a
string? I made it like this to mimic trace_marker, but maybe an integer
value + some kind of a dictionary in userspace is a better approach? I
belive that ftrace's maker is taking a string, because it's: 1. natural
for its interface and 2. anyone (sort of) can write to it, so it's hard
to assume anything. In this case the user "owns" the perf data, so he
could handle int<->whatever-else relation table...

Pawel
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Arnaldo Carvalho de Melo Sept. 12, 2014, 1:49 p.m. | #3
Em Fri, Sep 12, 2014 at 01:57:52PM +0100, Pawel Moll escreveu:
> On Fri, 2014-09-12 at 13:43 +0100, Christopher Covington wrote:
> > Just to ask the dumb questions in case the answers I've come up with are
> > wrong: What is PAGE_SIZE on an arm64 kernel? 
 
> It's either 4 or 64k, depending on CONFIG_ARM64_64K_PAGES.
 
> > How does userspace know?

> #include <unistd.h>
> #include <stdio.h>
 
> int main(void)
> {
> 	printf("%ld\n", sysconf(_SC_PAGESIZE));
> 	return 0;
> }
 
> Now a word of explanation. The PAGE_SIZE limitation was shamelessly
> stolen from perf_event_set_filter() (so PERF_EVENT_IOC_SET_FILTER) as an
> attempt to address a problem of passing a zero-terminated string from
> userspace. Simply speaking - there must be some limitation, and a page
> size seem as good as any other. I have strong doubts about this myself,
> so all alternative ideas are more than welcome.
 
> As I mentioned in the cover letter, maybe this simply shouldn't be a
> string? I made it like this to mimic trace_marker, but maybe an integer
> value + some kind of a dictionary in userspace is a better approach? I
> belive that ftrace's maker is taking a string, because it's: 1. natural
> for its interface and 2. anyone (sort of) can write to it, so it's hard
> to assume anything. In this case the user "owns" the perf data, so he
> could handle int<->whatever-else relation table...

Perhaps both? I.e. an u64 followed from a string, if the u64 is zero,
then there is a string right after it?

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pawel Moll Sept. 12, 2014, 1:58 p.m. | #4
On Fri, 2014-09-12 at 14:49 +0100, Arnaldo Carvalho de Melo wrote:
> Perhaps both? I.e. an u64 followed from a string, if the u64 is zero,
> then there is a string right after it?

How would this look like in userspace? Something like this?

8<----
struct perf_event_marker {
	uint64_t value;
	char *string;
} arg;

arg.value = 0x1234;

/* or */

arg.value = 0;
arg.string = "abcd";

ioctl(fd, PERF_EVENT_IOC_MARKER, &arg)
8<----

If so, maybe it would simpler just to go for classic size/data
structure?

8<-----
struct perf_event_marker {
	uint32_t size;
	void *data;
}
8<-----

This would directly map into struct perf_raw_record...

Paweł

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Christopher Covington Sept. 12, 2014, 2 p.m. | #5
On 09/12/2014 08:57 AM, Pawel Moll wrote:
> On Fri, 2014-09-12 at 13:43 +0100, Christopher Covington wrote:
>> Just to ask the dumb questions in case the answers I've come up with are
>> wrong: What is PAGE_SIZE on an arm64 kernel? 
> 
> It's either 4 or 64k, depending on CONFIG_ARM64_64K_PAGES.
> 
>> How does userspace know?
>>
> #include <unistd.h>
> #include <stdio.h>
> 
> int main(void)
> {
> 	printf("%ld\n", sysconf(_SC_PAGESIZE));
> 	return 0;
> }

Oh excellent, that actually works. Based on a misreading of the glibc code I
thought it was hard-coded to 64K.

Thanks,
Christopher
Arnaldo Carvalho de Melo Sept. 12, 2014, 4:19 p.m. | #6
Em Fri, Sep 12, 2014 at 02:58:55PM +0100, Pawel Moll escreveu:
> On Fri, 2014-09-12 at 14:49 +0100, Arnaldo Carvalho de Melo wrote:
> > Perhaps both? I.e. an u64 followed from a string, if the u64 is zero,
> > then there is a string right after it?
 
> How would this look like in userspace? Something like this?
 
> 8<----
> struct perf_event_marker {
> 	uint64_t value;
> 	char *string;
> } arg;
 
> arg.value = 0x1234;
 
> /* or */
 
> arg.value = 0;
> arg.string = "abcd";
 
> ioctl(fd, PERF_EVENT_IOC_MARKER, &arg)
> 8<----
 
> If so, maybe it would simpler just to go for classic size/data
> structure?
 
> 8<-----
> struct perf_event_marker {
> 	uint32_t size;
> 	void *data;
> }
> 8<-----
 
> This would directly map into struct perf_raw_record...

I can see the usefulness of having it all, i.e. if we do just:

perf trace --pid `pidof some-tool-in-debug-mode-using-this-interface`

Then 'perf trace' doesn't know about any binary format a tool may have,
getting strings there (hey, LD_PRELOADing some logging library to hook
into this comes to mind) and having it merged with other events
(syscalls, pagefaults, etc) looks useful.

As well as some specialized version of 'perf trace' that knows about
some binary protocol that would get app specific stats or lock status,
etc, perhaps even plugins for 'perf trace' that would be selected by
that first u64? Also seems useful.

I.e. having a way to provide just strings and another that would allow
passing perf_raw_record.

- Arnaldo

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
David Ahern Sept. 12, 2014, 5:37 p.m. | #7
On 9/12/14, 4:48 AM, Pawel Moll wrote:
> This patch adds a PERF_COUNT_SW_MARKER event type, which
> can be requested by user and a PERF_EVENT_IOC_MARKER
> ioctl command which will inject an event of said type into
> the perf buffer. The ioctl can take a zero-terminated
> string argument, similar to tracing_marker in ftrace,
> which will be kept in the "raw" field of the sample.
>
> The main use case for this is synchronisation of
> performance data generated in user space with the perf
> stream coming from the kernel. For example, the marker
> can be inserted by a JIT engine after it generated
> portion of the code, but before the code is executed
> for the first time, allowing the post-processor to
> pick the correct debugging information. Other example
> is a system profiling tool taking data from other
> sources than just perf, which generates a marker
> at the beginning at at the end of the session
> (also possibly periodically during the session) to
> synchronise kernel timestamps with clock values
> obtained in userspace (gtod or raw_monotonic).

Seems really similar to what I proposed in the past:

https://lkml.org/lkml/2011/2/27/159

Which was rejected.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Arnaldo Carvalho de Melo Sept. 12, 2014, 8:44 p.m. | #8
Em Fri, Sep 12, 2014 at 10:37:39AM -0700, David Ahern escreveu:
> On 9/12/14, 4:48 AM, Pawel Moll wrote:
> >This patch adds a PERF_COUNT_SW_MARKER event type, which
> >can be requested by user and a PERF_EVENT_IOC_MARKER
> >ioctl command which will inject an event of said type into
> >the perf buffer. The ioctl can take a zero-terminated
> >string argument, similar to tracing_marker in ftrace,
> >which will be kept in the "raw" field of the sample.
> >
> >The main use case for this is synchronisation of
> >performance data generated in user space with the perf
> >stream coming from the kernel. For example, the marker
> >can be inserted by a JIT engine after it generated
> >portion of the code, but before the code is executed
> >for the first time, allowing the post-processor to
> >pick the correct debugging information. Other example
> >is a system profiling tool taking data from other
> >sources than just perf, which generates a marker
> >at the beginning at at the end of the session
> >(also possibly periodically during the session) to
> >synchronise kernel timestamps with clock values
> >obtained in userspace (gtod or raw_monotonic).
> 
> Seems really similar to what I proposed in the past:
> 
> https://lkml.org/lkml/2011/2/27/159
> 
> Which was rejected.

I took a look at that thread, but just barely, emphasis on that.

Injecting something from userspace, a la ftrace, seems to be something,
as tglx mentioned, "buried" in that patchset.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
David Ahern Sept. 14, 2014, 3:43 p.m. | #9
On 9/12/14, 2:44 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 12, 2014 at 10:37:39AM -0700, David Ahern escreveu:
>> On 9/12/14, 4:48 AM, Pawel Moll wrote:
>>> This patch adds a PERF_COUNT_SW_MARKER event type, which
>>> can be requested by user and a PERF_EVENT_IOC_MARKER
>>> ioctl command which will inject an event of said type into
>>> the perf buffer. The ioctl can take a zero-terminated
>>> string argument, similar to tracing_marker in ftrace,
>>> which will be kept in the "raw" field of the sample.
>>>
>>> The main use case for this is synchronisation of
>>> performance data generated in user space with the perf
>>> stream coming from the kernel. For example, the marker
>>> can be inserted by a JIT engine after it generated
>>> portion of the code, but before the code is executed
>>> for the first time, allowing the post-processor to
>>> pick the correct debugging information. Other example
>>> is a system profiling tool taking data from other
>>> sources than just perf, which generates a marker
>>> at the beginning at at the end of the session
>>> (also possibly periodically during the session) to
>>> synchronise kernel timestamps with clock values
>>> obtained in userspace (gtod or raw_monotonic).
>>
>> Seems really similar to what I proposed in the past:
>>
>> https://lkml.org/lkml/2011/2/27/159
>>
>> Which was rejected.
>
> I took a look at that thread, but just barely, emphasis on that.
>
> Injecting something from userspace, a la ftrace, seems to be something,
> as tglx mentioned, "buried" in that patchset.

Thomas object to an ioctl buried deep in a patch -- newbie mistake.

Peter objected to the ioctl https://lkml.org/lkml/2011/3/1/229

It was not userspace injecting random data into the stream but rather 
forcing the sample to be generated and added to the stream.

David

David


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pawel Moll Sept. 15, 2014, 5:18 p.m. | #10
On Sun, 2014-09-14 at 16:43 +0100, David Ahern wrote:
> >> Seems really similar to what I proposed in the past:

Yeah, it wasn't really hard to come with similar conclusions :-)

> >> https://lkml.org/lkml/2011/2/27/159
> >>
> >> Which was rejected.
> >
> > I took a look at that thread, but just barely, emphasis on that.
> >
> > Injecting something from userspace, a la ftrace, seems to be something,
> > as tglx mentioned, "buried" in that patchset.
> 
> Thomas object to an ioctl buried deep in a patch -- newbie mistake.
> 
> Peter objected to the ioctl https://lkml.org/lkml/2011/3/1/229
> 
> It was not userspace injecting random data into the stream but rather 
> forcing the sample to be generated and added to the stream.

I would like to hear from Peter and others. If not here, I'll get them
to talk next month on Linux Plumbers :-)

Pawel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pawel Moll Sept. 15, 2014, 5:27 p.m. | #11
On Fri, 2014-09-12 at 17:19 +0100, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 12, 2014 at 02:58:55PM +0100, Pawel Moll escreveu:
> > On Fri, 2014-09-12 at 14:49 +0100, Arnaldo Carvalho de Melo wrote:
> > > Perhaps both? I.e. an u64 followed from a string, if the u64 is zero,
> > > then there is a string right after it?
>  
> > How would this look like in userspace? Something like this?
>  
> > 8<----
> > struct perf_event_marker {
> > 	uint64_t value;
> > 	char *string;
> > } arg;
>  
> > arg.value = 0x1234;
>  
> > /* or */
>  
> > arg.value = 0;
> > arg.string = "abcd";
>  
> > ioctl(fd, PERF_EVENT_IOC_MARKER, &arg)
> > 8<----
>  
> > If so, maybe it would simpler just to go for classic size/data
> > structure?
>  
> > 8<-----
> > struct perf_event_marker {
> > 	uint32_t size;
> > 	void *data;
> > }
> > 8<-----
>  
> > This would directly map into struct perf_raw_record...
> 
> I can see the usefulness of having it all, i.e. if we do just:
> 
> perf trace --pid `pidof some-tool-in-debug-mode-using-this-interface`

Hm. I haven't thought about a situation when 3rd party wants to inject
something into "my" data stream... I guess it could be implemented (a
"pid" member of the struct perf_event_marker with default 0 meaning
"myself"?), but will definitely complicate the patch. Should I have a
look at it now or maybe leave it till we get a general agreement about
the marker ioctl existence?

> Then 'perf trace' doesn't know about any binary format a tool may have,
> getting strings there (hey, LD_PRELOADing some logging library to hook
> into this comes to mind) and having it merged with other events
> (syscalls, pagefaults, etc) looks useful.

But do you still mean a "magic" u64 before the rest? Injecting a string
would just mean:

	marker.size = strlen(s) + 1;
	marker.data = s;

> As well as some specialized version of 'perf trace' that knows about
> some binary protocol that would get app specific stats or lock status,
> etc, perhaps even plugins for 'perf trace' that would be selected by
> that first u64? Also seems useful.
> 	  
> I.e. having a way to provide just strings and another that would allow
> passing perf_raw_record.

Sounds interesting. But then maybe this stuff shouldn't go into "raw"
then? It could be something like this in the sample:

	{ u64 type; /* 0 means zero-terminated string in data */
	  u32 size;
	  char data[size]; } && PERF_SAMPLE_MARKER

Pawel


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Arnaldo Carvalho de Melo Sept. 15, 2014, 6:31 p.m. | #12
Em Mon, Sep 15, 2014 at 06:27:14PM +0100, Pawel Moll escreveu:
> On Fri, 2014-09-12 at 17:19 +0100, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Sep 12, 2014 at 02:58:55PM +0100, Pawel Moll escreveu:
> > > On Fri, 2014-09-12 at 14:49 +0100, Arnaldo Carvalho de Melo wrote:
> > > > Perhaps both? I.e. an u64 followed from a string, if the u64 is zero,
> > > > then there is a string right after it?
> >  
> > > How would this look like in userspace? Something like this?
> >  
> > > 8<----
> > > struct perf_event_marker {
> > > 	uint64_t value;
> > > 	char *string;
> > > } arg;
> >  
> > > arg.value = 0x1234;
> >  
> > > /* or */
> >  
> > > arg.value = 0;
> > > arg.string = "abcd";
> >  
> > > ioctl(fd, PERF_EVENT_IOC_MARKER, &arg)
> > > 8<----
> >  
> > > If so, maybe it would simpler just to go for classic size/data
> > > structure?
> >  
> > > 8<-----
> > > struct perf_event_marker {
> > > 	uint32_t size;
> > > 	void *data;
> > > }
> > > 8<-----
> >  
> > > This would directly map into struct perf_raw_record...
> > 
> > I can see the usefulness of having it all, i.e. if we do just:
> > 
> > perf trace --pid `pidof some-tool-in-debug-mode-using-this-interface`
> 
> Hm. I haven't thought about a situation when 3rd party wants to inject
> something into "my" data stream... I guess it could be implemented (a

I was thinking about intercepting calls that pass some logging data, as
strings, and 'tee' them to the 'perf trace' 'data stream'.

> "pid" member of the struct perf_event_marker with default 0 meaning

Humm, Isn't PERF_SAMPLE_TID enough for that?

> "myself"?), but will definitely complicate the patch. Should I have a
> look at it now or maybe leave it till we get a general agreement about
> the marker ioctl existence?
> 
> > Then 'perf trace' doesn't know about any binary format a tool may have,
> > getting strings there (hey, LD_PRELOADing some logging library to hook
> > into this comes to mind) and having it merged with other events
> > (syscalls, pagefaults, etc) looks useful.
> 
> But do you still mean a "magic" u64 before the rest? Injecting a string
> would just mean:
> 
> 	marker.size = strlen(s) + 1;
> 	marker.data = s;
> 
> > As well as some specialized version of 'perf trace' that knows about
> > some binary protocol that would get app specific stats or lock status,
> > etc, perhaps even plugins for 'perf trace' that would be selected by
> > that first u64? Also seems useful.
> > 	  
> > I.e. having a way to provide just strings and another that would allow
> > passing perf_raw_record.
> 
> Sounds interesting. But then maybe this stuff shouldn't go into "raw"
> then? It could be something like this in the sample:
> 
> 	{ u64 type; /* 0 means zero-terminated string in data */
> 	  u32 size;
> 	  char data[size]; } && PERF_SAMPLE_MARKER

Yes, this is how I think it should be.
 
> Pawel
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Ingo Molnar Sept. 16, 2014, 7:44 a.m. | #13
* David Ahern <dsahern@gmail.com> wrote:

> On 9/12/14, 2:44 PM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Sep 12, 2014 at 10:37:39AM -0700, David Ahern escreveu:
> >>On 9/12/14, 4:48 AM, Pawel Moll wrote:
> >>>This patch adds a PERF_COUNT_SW_MARKER event type, which
> >>>can be requested by user and a PERF_EVENT_IOC_MARKER
> >>>ioctl command which will inject an event of said type into
> >>>the perf buffer. The ioctl can take a zero-terminated
> >>>string argument, similar to tracing_marker in ftrace,
> >>>which will be kept in the "raw" field of the sample.
> >>>
> >>>The main use case for this is synchronisation of
> >>>performance data generated in user space with the perf
> >>>stream coming from the kernel. For example, the marker
> >>>can be inserted by a JIT engine after it generated
> >>>portion of the code, but before the code is executed
> >>>for the first time, allowing the post-processor to
> >>>pick the correct debugging information. Other example
> >>>is a system profiling tool taking data from other
> >>>sources than just perf, which generates a marker
> >>>at the beginning at at the end of the session
> >>>(also possibly periodically during the session) to
> >>>synchronise kernel timestamps with clock values
> >>>obtained in userspace (gtod or raw_monotonic).
> >>
> >>Seems really similar to what I proposed in the past:
> >>
> >>https://lkml.org/lkml/2011/2/27/159
> >>
> >>Which was rejected.
> >
> >I took a look at that thread, but just barely, emphasis on that.
> >
> >Injecting something from userspace, a la ftrace, seems to be something,
> >as tglx mentioned, "buried" in that patchset.
> 
> Thomas object to an ioctl buried deep in a patch -- newbie 
> mistake.
> 
> Peter objected to the ioctl https://lkml.org/lkml/2011/3/1/229
> 
> It was not userspace injecting random data into the stream but 
> rather forcing the sample to be generated and added to the 
> stream.

I think adding an ioctl to inject user-provided data into the 
event stream is sensible, as long as there's a separate 'user 
generated data' event for it, etc.

The main usecase I could see would be to introduce a 
perf_printf() variant, supported by 'perf trace' by default, to 
add various tracable printouts to apps.

Timestamps generated by apps would be another usecase. It would 
probably be wise to add a 32-bit (or 64-bit) message type ID, 
plus a length field, with a message type registry somewhere in 
tools/perf/ (and reference implementation for each new subtype), 
to keep things organized yet flexible going forward.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pawel Moll Sept. 16, 2014, 4:33 p.m. | #14
On Mon, 2014-09-15 at 19:31 +0100, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 15, 2014 at 06:27:14PM +0100, Pawel Moll escreveu:
> > On Fri, 2014-09-12 at 17:19 +0100, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Sep 12, 2014 at 02:58:55PM +0100, Pawel Moll escreveu:
> > > > On Fri, 2014-09-12 at 14:49 +0100, Arnaldo Carvalho de Melo wrote:
> > > > > Perhaps both? I.e. an u64 followed from a string, if the u64 is zero,
> > > > > then there is a string right after it?
> > >  
> > > > How would this look like in userspace? Something like this?
> > >  
> > > > 8<----
> > > > struct perf_event_marker {
> > > > 	uint64_t value;
> > > > 	char *string;
> > > > } arg;
> > >  
> > > > arg.value = 0x1234;
> > >  
> > > > /* or */
> > >  
> > > > arg.value = 0;
> > > > arg.string = "abcd";
> > >  
> > > > ioctl(fd, PERF_EVENT_IOC_MARKER, &arg)
> > > > 8<----
> > >  
> > > > If so, maybe it would simpler just to go for classic size/data
> > > > structure?
> > >  
> > > > 8<-----
> > > > struct perf_event_marker {
> > > > 	uint32_t size;
> > > > 	void *data;
> > > > }
> > > > 8<-----
> > >  
> > > > This would directly map into struct perf_raw_record...
> > > 
> > > I can see the usefulness of having it all, i.e. if we do just:
> > > 
> > > perf trace --pid `pidof some-tool-in-debug-mode-using-this-interface`
> > 
> > Hm. I haven't thought about a situation when 3rd party wants to inject
> > something into "my" data stream... I guess it could be implemented (a
> 
> I was thinking about intercepting calls that pass some logging data, as
> strings, and 'tee' them to the 'perf trace' 'data stream'.

Right, ok, like LD_PRELOADing printf (a stupid example of course) and
piping it inside perf... So if I'm getting it right, it's the perf
process that would eventually do the ioctl(PERF_EVENT_IOC_MARKER), not
the traced process, correct? This makes sense. Another use case for
ioctl justification, thanks :-)

> > "myself"?), but will definitely complicate the patch. Should I have a
> > look at it now or maybe leave it till we get a general agreement about
> > the marker ioctl existence?
> > 
> > > Then 'perf trace' doesn't know about any binary format a tool may have,
> > > getting strings there (hey, LD_PRELOADing some logging library to hook
> > > into this comes to mind) and having it merged with other events
> > > (syscalls, pagefaults, etc) looks useful.
> > 
> > But do you still mean a "magic" u64 before the rest? Injecting a string
> > would just mean:
> > 
> > 	marker.size = strlen(s) + 1;
> > 	marker.data = s;
> > 
> > > As well as some specialized version of 'perf trace' that knows about
> > > some binary protocol that would get app specific stats or lock status,
> > > etc, perhaps even plugins for 'perf trace' that would be selected by
> > > that first u64? Also seems useful.
> > > 	  
> > > I.e. having a way to provide just strings and another that would allow
> > > passing perf_raw_record.
> > 
> > Sounds interesting. But then maybe this stuff shouldn't go into "raw"
> > then? It could be something like this in the sample:
> > 
> > 	{ u64 type; /* 0 means zero-terminated string in data */
> > 	  u32 size;
> > 	  char data[size]; } && PERF_SAMPLE_MARKER
> 
> Yes, this is how I think it should be.

Seems that Ingo had exactly the same thing on mind. I'll get a patch
done.

Paweł

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pawel Moll Sept. 16, 2014, 4:37 p.m. | #15
On Tue, 2014-09-16 at 08:44 +0100, Ingo Molnar wrote:
> I think adding an ioctl to inject user-provided data into the 
> event stream is sensible, as long as there's a separate 'user 
> generated data' event for it, etc.
> 
> The main usecase I could see would be to introduce a 
> perf_printf() variant, supported by 'perf trace' by default, to 
> add various tracable printouts to apps.
> 
> Timestamps generated by apps would be another usecase. It would 
> probably be wise to add a 32-bit (or 64-bit) message type ID, 
> plus a length field, with a message type registry somewhere in 
> tools/perf/ (and reference implementation for each new subtype), 
> to keep things organized yet flexible going forward.

Right, so this is pretty much what I got talking to Arnaldo...

>       { u64 type; /* 0 means zero-terminated string in data */
>         u32 size;
>         char data[size]; } && PERF_SAMPLE_MARKER

... with one type - 0 - defined as a "universal" string (so any possible
tool knows what to do about it), the rest being left to userspace (this
"registry" you mention).

Before I proceed any further, is the term "marker" acceptable? Maybe a
"printf" instead? Or a "log"? As we know naming is often single most
discussed subject when it comes to new things in the kernel ;-)

Pawel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Ingo Molnar Sept. 16, 2014, 5:58 p.m. | #16
* Pawel Moll <pawel.moll@arm.com> wrote:

> On Tue, 2014-09-16 at 08:44 +0100, Ingo Molnar wrote:
> > I think adding an ioctl to inject user-provided data into the 
> > event stream is sensible, as long as there's a separate 'user 
> > generated data' event for it, etc.
> > 
> > The main usecase I could see would be to introduce a 
> > perf_printf() variant, supported by 'perf trace' by default, to 
> > add various tracable printouts to apps.
> > 
> > Timestamps generated by apps would be another usecase. It would 
> > probably be wise to add a 32-bit (or 64-bit) message type ID, 
> > plus a length field, with a message type registry somewhere in 
> > tools/perf/ (and reference implementation for each new subtype), 
> > to keep things organized yet flexible going forward.
> 
> Right, so this is pretty much what I got talking to Arnaldo...
> 
> >       { u64 type; /* 0 means zero-terminated string in data */
> >         u32 size;
> >         char data[size]; } && PERF_SAMPLE_MARKER
> 
> ... with one type - 0 - defined as a "universal" string (so any 
> possible tool knows what to do about it), the rest being left 
> to userspace (this "registry" you mention).
> 
> Before I proceed any further, is the term "marker" acceptable? 
> Maybe a "printf" instead? Or a "log"? As we know naming is 
> often single most discussed subject when it comes to new things 
> in the kernel ;-)

Well, it's a user-space generated trace/event entry, so lets call 
it that?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch hide | download patch | download mbox

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index e5a75c5..83b0f5b 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -110,6 +110,7 @@  enum perf_sw_ids {
 	PERF_COUNT_SW_ALIGNMENT_FAULTS		= 7,
 	PERF_COUNT_SW_EMULATION_FAULTS		= 8,
 	PERF_COUNT_SW_DUMMY			= 9,
+	PERF_COUNT_SW_MARKER			= 10,
 
 	PERF_COUNT_SW_MAX,			/* non-ABI */
 };
@@ -350,6 +351,7 @@  struct perf_event_attr {
 #define PERF_EVENT_IOC_SET_OUTPUT	_IO ('$', 5)
 #define PERF_EVENT_IOC_SET_FILTER	_IOW('$', 6, char *)
 #define PERF_EVENT_IOC_ID		_IOR('$', 7, __u64 *)
+#define PERF_EVENT_IOC_MARKER		_IOR('$', 8, 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 df093e3..dbce284 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3655,6 +3655,7 @@  static inline int perf_fget_light(int fd, struct fd *p)
 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_sw_event_marker(struct perf_event *event, char __user *arg);
 
 static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -3709,6 +3710,9 @@  static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case PERF_EVENT_IOC_SET_FILTER:
 		return perf_event_set_filter(event, (void __user *)arg);
 
+	case PERF_EVENT_IOC_MARKER:
+		return perf_sw_event_marker(event, (char __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
@@ -3728,6 +3732,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_MARKER):
 		/* Fix up pointer size (usually 4 -> 8 in 32-on-64-bit case */
 		if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
 			cmd &= ~IOCSIZE_MASK;
@@ -5960,6 +5965,44 @@  static struct pmu perf_swevent = {
 	.event_idx	= perf_swevent_event_idx,
 };
 
+static int perf_sw_event_marker(struct perf_event *event, char __user *arg)
+{
+	struct perf_sample_data data;
+	struct pt_regs *regs = current_pt_regs();
+	struct perf_raw_record raw = { 0, };
+
+	if (!static_key_false(&perf_swevent_enabled[PERF_COUNT_SW_MARKER]))
+		return 0;
+
+	perf_sample_data_init(&data, 0, 0);
+
+	if (arg) {
+		long len = strnlen_user(arg, PAGE_SIZE);
+
+		if (len) {
+			raw.size = ALIGN(len + sizeof(u32), sizeof(u64))
+					- sizeof(u32);
+			raw.data = kzalloc(raw.size, GFP_KERNEL);
+			if (!raw.data)
+				return -ENOMEM;
+
+			if (copy_from_user(raw.data, arg, len)) {
+				kfree(raw.data);
+				return -EFAULT;
+			}
+
+			data.raw = &raw;
+		}
+	}
+
+	perf_event_output(event, &data, regs);
+
+	if (raw.size)
+		kfree(raw.data);
+
+	return 0;
+}
+
 #ifdef CONFIG_EVENT_TRACING
 
 static int perf_tp_filter_match(struct perf_event *event,