Re: [PATCH 0/4] Add support of guest user space symbols for perf kvm command.

Re: [PATCH 0/4] Add support of guest user space symbols for perf kvm command.

[Arnaldo Carvalho de Melo]

Em Thu, Dec 19, 2013 at 05:54:50PM -0500, Dongsheng Yang escreveu:
> Hi all,
> 	This patch add support of guest user space symbols for perf kvm command.

Cool stuff! Please see some comments on the patches, with requests for
possible patch splits and better changeset comments.
 
> Example:
>         # perf kvm --guestmount /tmp/guestmount/ top
>         Samples: 1K of event 'cycles', Event count (approx.): 259112905
>          17.34%  libcrypto.so.1.0.1e  [u] 0x000000000007d971
>           5.60%  [guest.kernel]       [g] kallsyms_expand_symbol
>           5.44%  libcrypto.so.1.0.1e  [u] md5_block_asm_data_order
>           4.09%  [guest.kernel]       [g] number.isra.1
>           3.59%  [guest.kernel]       [g] vsnprintf
>           3.52%  sshd                 [u] 0x00000000000441c0
>           2.37%  [guest.kernel]       [g] format_decode
>           2.36%  [guest.kernel]       [g] memcpy
>           2.11%  [guest.kernel]       [g] strnlen
> 
>         # perf kvm --guestmount /tmp/guestmount/ record -a sleep 1
>         [ perf record: Woken up 1 times to write data ]
>         [ perf record: Captured and wrote 0.828 MB perf.data.guest (~36163 samples) ]
>     
>         # perf kvm --guestmount /tmp/guestmount/ report
>         Samples: 4K of event 'cycles', Event count (approx.): 2662750816
>           8.67%  [guest/9217]  dd                            [u] 0x0000000000004e90
>           6.62%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] fget_light
>           6.17%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] system_call
>           5.97%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __srcu_read_lock
>           5.53%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __srcu_read_unlock
>           5.47%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __audit_syscall_exit
>           5.38%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] fsnotify
>           5.32%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] system_call_after_swapgs
>           4.45%  [guest/9217]  libc-2.17.so                  [u] __GI___libc_write
>           4.15%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] sys_write
>           3.97%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] vfs_read
>           3.78%  [guest/9217]  libc-2.17.so                  [u] __GI___libc_read
> 
> 
> Dongsheng Yang (4):
>   perf tools: Add support of guest in synthesize_threads.
>   perf tools: Add support for PERF_RECORD_MISC_GUEST_USER in
>     thread__find_addr_map().
>   perf tools: Add support of user space symbols for guest in perf kvm
>     top.
>   perf tools: Add support of user space symbols for guest in perf kvm
>     record.
> 
>  tools/perf/builtin-record.c | 10 ++++++++++
>  tools/perf/builtin-top.c    | 15 ++++++++++-----
>  tools/perf/util/event.c     | 44 ++++++++++++++++++++++++++++++--------------
>  3 files changed, 50 insertions(+), 19 deletions(-)
> 
> -- 
> 1.8.2.1

Re: [PATCH 1/4] perf tools: Add support of guest in synthesize_threads.

[Arnaldo Carvalho de Melo]

Em Thu, Dec 19, 2013 at 05:54:51PM -0500, Dongsheng Yang escreveu:
> We are using XXX__synthesize_threads() function to synthesize the
> symbols of user space for host. This patch add support of guest
> for these functions.

I think this patch should be split in at least two, please read below.

And please write here an outline of what will happen so that it can find
the right /proc files using machine->root_dir, etc, so that casual (and
experienced too) hackers can figure out what your patch does more
quickly.

- Arnaldo
 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/util/event.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 6948768..6e36bbb 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -106,8 +106,12 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
>  
>  	memset(&event->comm, 0, sizeof(event->comm));
>  
> -	tgid = perf_event__get_comm_tgid(pid, event->comm.comm,
> -					 sizeof(event->comm.comm));
> +	if (machine__is_host(machine))
> +		tgid = perf_event__get_comm_tgid(pid, event->comm.comm,
> +						 sizeof(event->comm.comm));
> +	else
> +		tgid = machine->pid;
> +
>  	if (tgid < 0)
>  		goto out;
>  
> @@ -129,7 +133,11 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
>  		goto out;
>  	}
>  
> -	snprintf(filename, sizeof(filename), "/proc/%d/task", pid);
> +	if (machine__is_default_guest(machine))
> +		return 0;

So the hunk above, the one that makes it bail out if it is a default
guest, I think it should be a separate patch with an explanation that
since pid is equal to 0:

#define DEFAULT_GUEST_KERNEL_ID         (0)

opendir("/proc/0/task") will fail, so its better to check if it is the
default guest instead of trying to opendir a directory that doesn't
exists and print a meaningless message in debug mode:

                pr_debug("couldn't open %s\n", filename);

Then, the rest, which is to use machine->root_dir to find the right proc
dir, probably via sshfs (right?), will work as expected _and_ is a
separate, unrelated to the above (bailing out on default guest pid))
patch, ok?

> +	snprintf(filename, sizeof(filename), "%s/proc/%d/task",
> +		 machine->root_dir, pid);
>  
>  	tasks = opendir(filename);
>  	if (tasks == NULL) {
> @@ -178,7 +186,11 @@ static int perf_event__synthesize_mmap_events(struct perf_tool *tool,
>  	FILE *fp;
>  	int rc = 0;
>  
> -	snprintf(filename, sizeof(filename), "/proc/%d/maps", pid);
> +	if (machine__is_default_guest(machine))

This one should be on the first patch, together with the other
machine__is_default_guest() test, please look for any other such test
and group them in a separate patch, as outlined above.

> +		return 0;
> +
> +	snprintf(filename, sizeof(filename), "%s/proc/%d/maps",
> +		 machine->root_dir, pid);
>  
>  	fp = fopen(filename, "r");
>  	if (fp == NULL) {
> @@ -218,7 +230,10 @@ static int perf_event__synthesize_mmap_events(struct perf_tool *tool,
>  		/*
>  		 * Just like the kernel, see __perf_event_mmap in kernel/perf_event.c
>  		 */
> -		event->header.misc = PERF_RECORD_MISC_USER;
> +		if (machine__is_host(machine))
> +			event->header.misc = PERF_RECORD_MISC_USER;
> +		else
> +			event->header.misc = PERF_RECORD_MISC_GUEST_USER;
>  
>  		if (prot[2] != 'x') {
>  			if (!mmap_data || prot[0] != 'r')
> @@ -387,6 +402,7 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
>  				   struct machine *machine, bool mmap_data)
>  {
>  	DIR *proc;
> +	char proc_path[PATH_MAX];
>  	struct dirent dirent, *next;
>  	union perf_event *comm_event, *mmap_event;
>  	int err = -1;
> @@ -399,7 +415,12 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
>  	if (mmap_event == NULL)
>  		goto out_free_comm;
>  
> -	proc = opendir("/proc");
> +	if (machine__is_default_guest(machine))
> +		return 0;

One more :-)

> +	snprintf(proc_path, sizeof(proc_path), "%s/proc", machine->root_dir);
> +	proc = opendir(proc_path);
> +
>  	if (proc == NULL)
>  		goto out_free_mmap;
>  
> -- 
> 1.8.2.1

Re: [PATCH 0/4] Add support of guest user space symbols for perf kvm command.

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Thu, Dec 19, 2013 at 05:54:50PM -0500, Dongsheng Yang escreveu:
> > Hi all,
> > 	This patch add support of guest user space symbols for perf kvm command.
> 
> Cool stuff! Please see some comments on the patches, with requests for
> possible patch splits and better changeset comments.
>  
> > Example:
> >         # perf kvm --guestmount /tmp/guestmount/ top
> >         Samples: 1K of event 'cycles', Event count (approx.): 259112905

Btw., having to specify a '--guestmount /tmp/guestmount/' just to 
achieve a natural feature (proper guest symbol resolution) is a poor 
user interface, obviously.

Is there perhaps a predictable pattern as to where qemu (or libvirt) 
puts a user's guest mounts, or some other discovery method?

If yes then it would be possible to automatically look for that 
pattern, and use the guest filesystem when it's available, without the 
user having to manually configure the path.

( Ideally we'd have kernel help for discovering this, but last time I
  raised that with the KVM folks there was resistence. )

Thanks,

	Ingo

Re: [PATCH 0/4] Add support of guest user space symbols for perf kvm command.

[David Ahern]

On 12/19/13, 8:34 AM, Ingo Molnar wrote:

> Btw., having to specify a '--guestmount /tmp/guestmount/' just to
> achieve a natural feature (proper guest symbol resolution) is a poor
> user interface, obviously.
>
> Is there perhaps a predictable pattern as to where qemu (or libvirt)
> puts a user's guest mounts, or some other discovery method?

No, qemu does not export guest info to the host. A user has to create it 
-- either using something like sshfs or copying the files. I tend to use 
the latter as it is simpler.

David

>
> If yes then it would be possible to automatically look for that
> pattern, and use the guest filesystem when it's available, without the
> user having to manually configure the path.
>
> ( Ideally we'd have kernel help for discovering this, but last time I
>    raised that with the KVM folks there was resistence. )

[PATCH 0/4] Add support of guest user space symbols for perf kvm command.

[Dongsheng Yang]

Hi all,
	This patch add support of guest user space symbols for perf kvm command.

Example:
        # perf kvm --guestmount /tmp/guestmount/ top
        Samples: 1K of event 'cycles', Event count (approx.): 259112905
         17.34%  libcrypto.so.1.0.1e  [u] 0x000000000007d971
          5.60%  [guest.kernel]       [g] kallsyms_expand_symbol
          5.44%  libcrypto.so.1.0.1e  [u] md5_block_asm_data_order
          4.09%  [guest.kernel]       [g] number.isra.1
          3.59%  [guest.kernel]       [g] vsnprintf
          3.52%  sshd                 [u] 0x00000000000441c0
          2.37%  [guest.kernel]       [g] format_decode
          2.36%  [guest.kernel]       [g] memcpy
          2.11%  [guest.kernel]       [g] strnlen

        # perf kvm --guestmount /tmp/guestmount/ record -a sleep 1
        [ perf record: Woken up 1 times to write data ]
        [ perf record: Captured and wrote 0.828 MB perf.data.guest (~36163 samples) ]
    
        # perf kvm --guestmount /tmp/guestmount/ report
        Samples: 4K of event 'cycles', Event count (approx.): 2662750816
          8.67%  [guest/9217]  dd                            [u] 0x0000000000004e90
          6.62%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] fget_light
          6.17%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] system_call
          5.97%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __srcu_read_lock
          5.53%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __srcu_read_unlock
          5.47%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __audit_syscall_exit
          5.38%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] fsnotify
          5.32%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] system_call_after_swapgs
          4.45%  [guest/9217]  libc-2.17.so                  [u] __GI___libc_write
          4.15%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] sys_write
          3.97%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] vfs_read
          3.78%  [guest/9217]  libc-2.17.so                  [u] __GI___libc_read


Dongsheng Yang (4):
  perf tools: Add support of guest in synthesize_threads.
  perf tools: Add support for PERF_RECORD_MISC_GUEST_USER in
    thread__find_addr_map().
  perf tools: Add support of user space symbols for guest in perf kvm
    top.
  perf tools: Add support of user space symbols for guest in perf kvm
    record.

 tools/perf/builtin-record.c | 10 ++++++++++
 tools/perf/builtin-top.c    | 15 ++++++++++-----
 tools/perf/util/event.c     | 44 ++++++++++++++++++++++++++++++--------------
 3 files changed, 50 insertions(+), 19 deletions(-)

-- 
1.8.2.1

[PATCH 1/4] perf tools: Add support of guest in synthesize_threads.

[Dongsheng Yang]

We are using XXX__synthesize_threads() function to synthesize the
symbols of user space for host. This patch add support of guest
for these functions.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/event.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 6948768..6e36bbb 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -106,8 +106,12 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
 
 	memset(&event->comm, 0, sizeof(event->comm));
 
-	tgid = perf_event__get_comm_tgid(pid, event->comm.comm,
-					 sizeof(event->comm.comm));
+	if (machine__is_host(machine))
+		tgid = perf_event__get_comm_tgid(pid, event->comm.comm,
+						 sizeof(event->comm.comm));
+	else
+		tgid = machine->pid;
+
 	if (tgid < 0)
 		goto out;
 
@@ -129,7 +133,11 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
 		goto out;
 	}
 
-	snprintf(filename, sizeof(filename), "/proc/%d/task", pid);
+	if (machine__is_default_guest(machine))
+		return 0;
+
+	snprintf(filename, sizeof(filename), "%s/proc/%d/task",
+		 machine->root_dir, pid);
 
 	tasks = opendir(filename);
 	if (tasks == NULL) {
@@ -178,7 +186,11 @@ static int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 	FILE *fp;
 	int rc = 0;
 
-	snprintf(filename, sizeof(filename), "/proc/%d/maps", pid);
+	if (machine__is_default_guest(machine))
+		return 0;
+
+	snprintf(filename, sizeof(filename), "%s/proc/%d/maps",
+		 machine->root_dir, pid);
 
 	fp = fopen(filename, "r");
 	if (fp == NULL) {
@@ -218,7 +230,10 @@ static int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		/*
 		 * Just like the kernel, see __perf_event_mmap in kernel/perf_event.c
 		 */
-		event->header.misc = PERF_RECORD_MISC_USER;
+		if (machine__is_host(machine))
+			event->header.misc = PERF_RECORD_MISC_USER;
+		else
+			event->header.misc = PERF_RECORD_MISC_GUEST_USER;
 
 		if (prot[2] != 'x') {
 			if (!mmap_data || prot[0] != 'r')
@@ -387,6 +402,7 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
 				   struct machine *machine, bool mmap_data)
 {
 	DIR *proc;
+	char proc_path[PATH_MAX];
 	struct dirent dirent, *next;
 	union perf_event *comm_event, *mmap_event;
 	int err = -1;
@@ -399,7 +415,12 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
 	if (mmap_event == NULL)
 		goto out_free_comm;
 
-	proc = opendir("/proc");
+	if (machine__is_default_guest(machine))
+		return 0;
+
+	snprintf(proc_path, sizeof(proc_path), "%s/proc", machine->root_dir);
+	proc = opendir(proc_path);
+
 	if (proc == NULL)
 		goto out_free_mmap;
 
-- 
1.8.2.1

[PATCH 2/4] perf tools: Add support for PERF_RECORD_MISC_GUEST_USER in thread__find_addr_map().

[Dongsheng Yang]

This patch remove a TODO in thread__find_addr_map() and add support of PERF_RECORD_MISC_GUEST_USER.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/event.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 6e36bbb..598b73e 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -679,15 +679,10 @@ void thread__find_addr_map(struct thread *thread,
 		al->level = 'g';
 		mg = &machine->kmaps;
 		load_map = true;
+	} else if (cpumode == PERF_RECORD_MISC_GUEST_USER && perf_guest) {
+		al->level = 'u';
 	} else {
-		/*
-		 * 'u' means guest os user space.
-		 * TODO: We don't support guest user space. Might support late.
-		 */
-		if (cpumode == PERF_RECORD_MISC_GUEST_USER && perf_guest)
-			al->level = 'u';
-		else
-			al->level = 'H';
+		al->level = 'H';
 		al->map = NULL;
 
 		if ((cpumode == PERF_RECORD_MISC_GUEST_USER ||
-- 
1.8.2.1

[PATCH 3/4] perf tools: Add support of user space symbols for guest in perf kvm top.

[Dongsheng Yang]

	# perf kvm --guestmount /tmp/guestmount/ top
	Samples: 1K of event 'cycles', Event count (approx.): 259112905
	 17.34%  libcrypto.so.1.0.1e  [u] 0x000000000007d971
	  5.60%  [guest.kernel]       [g] kallsyms_expand_symbol
	  5.44%  libcrypto.so.1.0.1e  [u] md5_block_asm_data_order
	  4.09%  [guest.kernel]       [g] number.isra.1
	  3.59%  [guest.kernel]       [g] vsnprintf
	  3.52%  sshd                 [u] 0x00000000000441c0
	  2.37%  [guest.kernel]       [g] format_decode
	  2.36%  [guest.kernel]       [g] memcpy
	  2.11%  [guest.kernel]       [g] strnlen

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-top.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 03d37a7..670bd0b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -821,11 +821,9 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx)
 			break;
 		case PERF_RECORD_MISC_GUEST_USER:
 			++top->guest_us_samples;
-			/*
-			 * TODO: we don't process guest user from host side
-			 * except simple counting.
-			 */
-			/* Fall thru */
+			machine = perf_session__find_machine(session,
+							     sample.pid);
+			break;
 		default:
 			goto next_event;
 		}
@@ -912,6 +910,7 @@ static int __cmd_top(struct perf_top *top)
 	struct perf_record_opts *opts = &top->record_opts;
 	pthread_t thread;
 	int ret;
+	struct rb_node *nd;
 
 	top->session = perf_session__new(NULL, false, NULL);
 	if (top->session == NULL)
@@ -931,6 +930,12 @@ static int __cmd_top(struct perf_top *top)
 
 	machine__synthesize_threads(&top->session->machines.host, &opts->target,
 				    top->evlist->threads, false);
+
+	for (nd = rb_first(&top->session->machines.guests); nd; nd = rb_next(nd)) {
+		struct machine *pos = rb_entry(nd, struct machine, rb_node);
+		machine__synthesize_threads(pos, &opts->target, top->evlist->threads, false);
+	}
+
 	ret = perf_top__start_counters(top);
 	if (ret)
 		goto out_delete;
-- 
1.8.2.1

[PATCH 4/4] perf tools: Add support of user space symbols for guest in perf kvm record.

[Dongsheng Yang]

	# perf kvm --guestmount /tmp/guestmount/ record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.828 MB perf.data.guest (~36163 samples) ]

	# perf kvm --guestmount /tmp/guestmount/ report
	Samples: 4K of event 'cycles', Event count (approx.): 2662750816
	  8.67%  [guest/9217]  dd                            [u] 0x0000000000004e90
	  6.62%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] fget_light
	  6.17%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] system_call
	  5.97%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __srcu_read_lock
	  5.53%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __srcu_read_unlock
	  5.47%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] __audit_syscall_exit
	  5.38%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] fsnotify
	  5.32%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] system_call_after_swapgs
	  4.45%  [guest/9217]  libc-2.17.so                  [u] __GI___libc_write
	  4.15%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] sys_write
	  3.97%  [guest/9217]  [guest.kernel.kallsyms.9217]  [g] vfs_read
	  3.78%  [guest/9217]  libc-2.17.so                  [u] __GI___libc_read

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-record.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index c1c1200..ac1e540 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -279,6 +279,9 @@ static void perf_event__synthesize_guest_os(struct machine *machine, void *data)
 {
 	int err;
 	struct perf_tool *tool = data;
+	struct perf_record *rec = container_of(tool, struct perf_record, tool);
+	struct perf_record_opts *opts = &rec->opts;
+	struct perf_evlist *evsel_list = rec->evlist;
 	/*
 	 *As for guest kernel when processing subcommand record&report,
 	 *we arrange module mmap prior to guest kernel mmap and trigger
@@ -305,6 +308,13 @@ static void perf_event__synthesize_guest_os(struct machine *machine, void *data)
 	if (err < 0)
 		pr_err("Couldn't record guest kernel [%d]'s reference"
 		       " relocation symbol.\n", machine->pid);
+
+	err = __machine__synthesize_threads(machine, tool, &opts->target, evsel_list->threads,
+					    process_synthesized_event, opts->sample_address);
+
+	if (err < 0)
+		pr_err("Couldn't record guest userspace [%d]'s reference"
+		       " relocation symbol.\n", machine->pid);
 }
 
 static struct perf_event_header finished_round_event = {
-- 
1.8.2.1