perf kvm: Get rid of unused guest_kallsyms

PATCH In review Gui Jianfeng 4 participants 11 messages Jun 25, 2010 – Aug 4, 2010 perf events

lore

Share & tools

lore

Gui Jianfeng

author

2010-06-25 02:17 UTC #0 lore raw

PATCH 1/3 perf kvm: Get rid of unused guest_kallsyms

guest_kallsyms is redundant here, remove it.

Signed-off-by Gui Jianfeng <[email protected]>

---
 tools/perf/builtin-record.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

tools/perf/builtin-record.c +0 -9

index dc3435e..abaf0f8 100644
@@ -439,8 +439,6 @@ static void atexit_header(void)
 static void event__synthesize_guest_os(struct machine *machine, void *data)
 {
 	int err;
-	char *guest_kallsyms;
-	char path[PATH_MAX];
 	struct perf_session *psession = data;
 
 	if (machine__is_host(machine))

@@ -460,13 +458,6 @@ static void event__synthesize_guest_os(struct machine *machine, void *data)
 		pr_err("Couldn't record guest kernel [%d]'s reference"
 		       " relocation symbol.\n", machine->pid);
 
-	if (machine__is_default_guest(machine))
-		guest_kallsyms = (char *) symbol_conf.default_guest_kallsyms;
-	else {
-		sprintf(path, "%s/proc/kallsyms", machine->root_dir);
-		guest_kallsyms = path;
-	}
-
 	/*
 	 * We use _stext for guest kernel because guest kernel's /proc/kallsyms
 	 * have no _text sometimes.

Signature · 1 line

1.6.5.2

Arnaldo Carvalho de Melo

2010-06-25 13:14 UTC #2 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

Em Fri, Jun 25, 2010 at 10:16:59AM +0800, Gui Jianfeng escreveu:

Quoted 1 line

group_fd related code is dead here, remove them.

I agree it is dead at the moment, but I think Peter (or was it Ingo at the time?) had a reason for it to live, Peter, care to take a look?

- Arnaldo

Quoted 57 lines

Signed-off-by Gui Jianfeng <[email protected]>

---
 tools/perf/builtin-record.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

tools/perf/builtin-record.c +1 ~~-12~~

index abaf0f8..84d58b7 100644
@@ -45,7 +45,6 @@ static int			freq				=   1000;
 static int			output;
 static int			pipe_output			=      0;
 static const char		*output_name			= "perf.data";
-static int			group				=      0;
 static int			realtime_prio			=      0;
 static bool			raw_samples			=  false;
 static bool			system_wide			=  false;

@@ -203,8 +202,6 @@ static void sig_atexit(void)
 	kill(getpid(), signr);
 }
 
-static int group_fd;
-
 static struct perf_header_attr *get_header_attr(struct perf_event_attr *a, int nr)
 {
 	struct perf_header_attr *h_attr;

@@ -291,7 +288,7 @@ static void create_counter(int counter, int cpu)
 	for (thread_index = 0; thread_index < thread_num; thread_index++) {
 try_again:
 		fd[nr_cpu][counter][thread_index] = sys_perf_event_open(attr,
-				all_tids[thread_index], cpu, group_fd, 0);
+				all_tids[thread_index], cpu, -1, 0);
 
 		if (fd[nr_cpu][counter][thread_index] < 0) {
 			int err = errno;

@@ -359,12 +356,6 @@ try_again:
 		assert(fd[nr_cpu][counter][thread_index] >= 0);
 		fcntl(fd[nr_cpu][counter][thread_index], F_SETFL, O_NONBLOCK);
 
-		/*
-		 * First counter acts as the group leader:
-		 */
-		if (group && group_fd == -1)
-			group_fd = fd[nr_cpu][counter][thread_index];
-
 		if (counter || thread_index) {
 			ret = ioctl(fd[nr_cpu][counter][thread_index],
 					PERF_EVENT_IOC_SET_OUTPUT,

@@ -406,7 +397,6 @@ static void open_counters(int cpu)
 {
 	int counter;
 
-	group_fd = -1;
 	for (counter = 0; counter < nr_counters; counter++)
 		create_counter(counter, cpu);
 
-- 1.6.5.2

Peter Zijlstra

maintainer

2010-06-25 14:49 UTC #3 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

Quoted 5 lines · Arnaldo Carvalho de Melo

Em Fri, Jun 25, 2010 at 10:16:59AM +0800, Gui Jianfeng escreveu:
Quoted 1 line
group_fd related code is dead here, remove them.
I agree it is dead at the moment, but I think Peter (or was it Ingo at the time?) had a reason for it to live, Peter, care to take a look?

Ah, yes, it would be nice for SH (which doesn't have a PMI) to couple a software timer with their hardware counter to get samples.

Never got around to actually implementing that though, maybe Paul has a minion interested in making that work?

Quoted 57 lines

Signed-off-by Gui Jianfeng <[email protected]>

---
 tools/perf/builtin-record.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

tools/perf/builtin-record.c +1 ~~-12~~

index abaf0f8..84d58b7 100644
@@ -45,7 +45,6 @@ static int			freq				=   1000;
 static int			output;
 static int			pipe_output			=      0;
 static const char		*output_name			= "perf.data";
-static int			group				=      0;
 static int			realtime_prio			=      0;
 static bool			raw_samples			=  false;
 static bool			system_wide			=  false;

@@ -203,8 +202,6 @@ static void sig_atexit(void)
 	kill(getpid(), signr);
 }
 
-static int group_fd;
-
 static struct perf_header_attr *get_header_attr(struct perf_event_attr *a, int nr)
 {
 	struct perf_header_attr *h_attr;

@@ -291,7 +288,7 @@ static void create_counter(int counter, int cpu)
 	for (thread_index = 0; thread_index < thread_num; thread_index++) {
 try_again:
 		fd[nr_cpu][counter][thread_index] = sys_perf_event_open(attr,
-				all_tids[thread_index], cpu, group_fd, 0);
+				all_tids[thread_index], cpu, -1, 0);
 
 		if (fd[nr_cpu][counter][thread_index] < 0) {
 			int err = errno;

@@ -359,12 +356,6 @@ try_again:
 		assert(fd[nr_cpu][counter][thread_index] >= 0);
 		fcntl(fd[nr_cpu][counter][thread_index], F_SETFL, O_NONBLOCK);
 
-		/*
-		 * First counter acts as the group leader:
-		 */
-		if (group && group_fd == -1)
-			group_fd = fd[nr_cpu][counter][thread_index];
-
 		if (counter || thread_index) {
 			ret = ioctl(fd[nr_cpu][counter][thread_index],
 					PERF_EVENT_IOC_SET_OUTPUT,

@@ -406,7 +397,6 @@ static void open_counters(int cpu)
 {
 	int counter;
 
-	group_fd = -1;
 	for (counter = 0; counter < nr_counters; counter++)
 		create_counter(counter, cpu);
 
-- 1.6.5.2

Matt Fleming

2010-06-26 15:14 UTC #4 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

Quoted 6 lines · Peter Zijlstra

Ah, yes, it would be nice for SH (which doesn't have a PMI) to couple a software timer with their hardware counter to get samples.
Never got around to actually implementing that though, maybe Paul has a minion interested in making that work?

I'll take a look at it.

Arnaldo Carvalho de Melo

2010-06-26 18:12 UTC #5 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

Em Sat, Jun 26, 2010 at 04:14:46PM +0100, Matt Fleming escreveu:

Quoted 8 lines · Peter Zijlstra

Quoted 5 lines
Ah, yes, it would be nice for SH (which doesn't have a PMI) to couple a software timer with their hardware counter to get samples.
Never got around to actually implementing that though, maybe Paul has a minion interested in making that work?
I'll take a look at it.

Thanks!

- Arnaldo

Matt Fleming

2010-07-18 20:06 UTC #6 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

In reply to Matt Fleming

Quoted 9 lines · Matt Fleming

Quoted 6 lines · Peter Zijlstra
Ah, yes, it would be nice for SH (which doesn't have a PMI) to couple a software timer with their hardware counter to get samples.
Never got around to actually implementing that though, maybe Paul has a minion interested in making that work?
I'll take a look at it.

How does the 'group_fd' parameter relate to the lack of PMI? Is the idea to have one hrtimer that, when it fires, we sample all the counters? So the first counter to be created is the group leader, which starts the hrtimer, and all other counters are linked to this one? I had a go at using a hrtimer per counter (minus any weighting of samples) and it worked OK and seemed sensible given that we may want to sample counters at different frequencies.

Is this what you had in mind with the 'group_fd' paramter, Peter? That there'd be only one hrtimer?

Peter Zijlstra

maintainer

2010-08-04 10:56 UTC #7 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

Quoted 19 lines · Matt Fleming

Quoted 9 lines · Matt Fleming
Quoted 6 lines · Peter Zijlstra
Ah, yes, it would be nice for SH (which doesn't have a PMI) to couple a software timer with their hardware counter to get samples.
Never got around to actually implementing that though, maybe Paul has a minion interested in making that work?
I'll take a look at it.
How does the 'group_fd' parameter relate to the lack of PMI? Is the idea to have one hrtimer that, when it fires, we sample all the counters? So the first counter to be created is the group leader, which starts the hrtimer, and all other counters are linked to this one? I had a go at using a hrtimer per counter (minus any weighting of samples) and it worked OK and seemed sensible given that we may want to sample counters at different frequencies.

Quoted 2 lines

Is this what you had in mind with the 'group_fd' paramter, Peter? That there'd be only one hrtimer?

Right. So sys_perf_event_open() creates a stand alone event, but if you supply the group_fd param it will attach the newly created one to the leader indicated by group_fd.

Groups have the properly that they will always be scheduled together, and read/sample can access/provide data of all of them.

So you can have a sampling leader report the counts of its siblings.

You can make multiple groups, like {hrtimer, cycles} and {hrtimer, instructions} and {hrtimer, dcache-miss} and perf will schedule the stuff. If you'd try to do {hrtimer, cycles, insns, dcache-miss} but only have 2 hardware counters the group creation should fail because the implementation should dis-allow creating groups that cannot be scheduled

Matt Fleming

2010-08-04 11:25 UTC #8 lore raw

Re: [PATCH 2/3] perf: Remove dead code in buildin-record.c

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Matt Fleming
How does the 'group_fd' parameter relate to the lack of PMI? Is the idea to have one hrtimer that, when it fires, we sample all the counters? So the first counter to be created is the group leader, which starts the hrtimer, and all other counters are linked to this one? I had a go at using a hrtimer per counter (minus any weighting of samples) and it worked OK and seemed sensible given that we may want to sample counters at different frequencies.
Is this what you had in mind with the 'group_fd' paramter, Peter? That there'd be only one hrtimer?

Quoted 6 lines

Right. So sys_perf_event_open() creates a stand alone event, but if you supply the group_fd param it will attach the newly created one to the leader indicated by group_fd.
Groups have the properly that they will always be scheduled together, and read/sample can access/provide data of all of them.

Ah, I think I see. The benefit of scheduling these events together is that we can then sample the counters at roughly the same time? As opposed to the case where we have multiple hrtimers firing and the counter values will be from vastly different times? Yeah, that makes sense now.

Quoted 8 lines

So you can have a sampling leader report the counts of its siblings.
You can make multiple groups, like {hrtimer, cycles} and {hrtimer, instructions} and {hrtimer, dcache-miss} and perf will schedule the stuff. If you'd try to do {hrtimer, cycles, insns, dcache-miss} but only have 2 hardware counters the group creation should fail because the implementation should dis-allow creating groups that cannot be scheduled

Right, so the hrtimer is an event all by itself: the parent event?

Gui Jianfeng

author

2010-06-25 02:20 UTC #9 lore raw

PATCH 3/3 perf: Fix the bug which fails to put memory mapping to record file

In reply to Gui Jianfeng

If we execute perf record without -t or -p option, it fails to load process memory map to record file. Here fix it.

Signed-off-by Gui Jianfeng <[email protected]>

---
 tools/perf/builtin-record.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

tools/perf/builtin-record.c +13 -3

index 84d58b7..f834d21 100644
@@ -685,9 +685,19 @@ static int __cmd_record(int argc, const char **argv)
 	if (perf_guest)
 		perf_session__process_machines(session, event__synthesize_guest_os);
 
-	if (!system_wide && profile_cpu == -1)
-		event__synthesize_thread(target_tid, process_synthesized_event,
-					 session);
+	if (!system_wide && profile_cpu == -1) {
+		int ret;
+
+		if (target_tid != -1) {
+			ret = event__synthesize_thread(target_tid,
+						 process_synthesized_event,
+						 session);
+		} else {
+			ret = event__synthesize_thread(all_tids[0],
+						 process_synthesized_event,
+						 session);
+		}
+	}
 	else
 		event__synthesize_threads(process_synthesized_event, session);

-- 1.6.5.2

tip-bot for Gui Jianfeng

author

2010-06-25 15:29 UTC #10 lore raw

In reply to Gui Jianfeng

Commit-ID:  830f4c803196eec181e209110885c4ac130f3805
Gitweb:     http://git.kernel.org/tip/830f4c803196eec181e209110885c4ac130f3805
Author:     Gui Jianfeng <[email protected]>
AuthorDate: Fri, 25 Jun 2010 10:15:28 +0800
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 25 Jun 2010 07:28:21 -0300

perf kvm: Get rid of unused guest_kallsyms

guest_kallsyms is redundant here, remove it.

Cc: Ingo Molnar <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Yanmin Zhang <[email protected]>
LKML-Reference: <[email protected]>

Signed-off-by Gui Jianfeng <[email protected]>

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-record.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

tools/perf/builtin-record.c +0 -9

index 86b1c3b..0df6408 100644
@@ -445,8 +445,6 @@ static void atexit_header(void)
 static void event__synthesize_guest_os(struct machine *machine, void *data)
 {
 	int err;
-	char *guest_kallsyms;
-	char path[PATH_MAX];
 	struct perf_session *psession = data;
 
 	if (machine__is_host(machine))

@@ -466,13 +464,6 @@ static void event__synthesize_guest_os(struct machine *machine, void *data)
 		pr_err("Couldn't record guest kernel [%d]'s reference"
 		       " relocation symbol.\n", machine->pid);
 
-	if (machine__is_default_guest(machine))
-		guest_kallsyms = (char *) symbol_conf.default_guest_kallsyms;
-	else {
-		sprintf(path, "%s/proc/kallsyms", machine->root_dir);
-		guest_kallsyms = path;
-	}
-
 	/*
 	 * We use _stext for guest kernel because guest kernel's /proc/kallsyms
 	 * have no _text sometimes.