Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 23 Oct 2018 08:34:52 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     David Miller <davem@davemloft.net>
Cc:     dzickus@redhat.com, acme@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: perf overlapping maps...
Message-ID: <20181023063452.GB20075@krava>
References: <20181019.214401.2045294780943844999.davem@davemloft.net>
 <20181022140738.jvutwmstgm2f65et@redhat.com>
 <20181022161613.GF2945@krava>
 <20181022.105842.1364583912952511294.davem@davemloft.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181022.105842.1364583912952511294.davem@davemloft.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Oct 22, 2018 at 10:58:42AM -0700, David Miller wrote:
> From: Jiri Olsa <jolsa@redhat.com>
> Date: Mon, 22 Oct 2018 18:16:13 +0200
> 
> > I think the fix might actualy speed things up,
> > but yes, there could be other report regressions
> 
> I was about to say the same thing, it could actually speed things up.
> In the best case, less work is done (clone avoided, and overlapping
> maps don't have to be handled).  In the worst case, nothing changes.
> 
> Here is what I've been using, to give you an idea.  There may be some
> file offset fuzz in these patches.

I'm not sure about using the misc field bit defined/used by userland,
in case there's some new one comming in future for fork event..

but the only other way I can think of now is adding new 'user' event
for that, but that ended up as a bigger change (attached)

I think if we make some 'big enough' comment about the bit usage,
your change is better.. will you post or should I?

thanks,
jirka


---
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index bc646185f8d9..06e28cef5579 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -60,6 +60,7 @@ static const char *perf_event__names[] = {
 	[PERF_RECORD_EVENT_UPDATE]		= "EVENT_UPDATE",
 	[PERF_RECORD_TIME_CONV]			= "TIME_CONV",
 	[PERF_RECORD_HEADER_FEATURE]		= "FEATURE",
+	[PERF_RECORD_FORK_USER]			= "FORK_USER",
 };
 
 static const char *perf_ns__names[] = {
@@ -307,7 +308,7 @@ static int perf_event__synthesize_fork(struct perf_tool *tool,
 	}
 	event->fork.pid  = tgid;
 	event->fork.tid  = pid;
-	event->fork.header.type = PERF_RECORD_FORK;
+	event->fork.header.type = PERF_RECORD_FORK_USER;
 
 	event->fork.header.size = (sizeof(event->fork) + machine->id_hdr_size);
 
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index bfa60bcafbde..e7252ae61529 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -249,6 +249,7 @@ enum perf_user_event_type { /* above any possible kernel type */
 	PERF_RECORD_EVENT_UPDATE		= 78,
 	PERF_RECORD_TIME_CONV			= 79,
 	PERF_RECORD_HEADER_FEATURE		= 80,
+	PERF_RECORD_FORK_USER			= 81,
 	PERF_RECORD_HEADER_MAX
 };
 
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 111ae858cbcb..b7d8aec18707 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1702,6 +1702,7 @@ void machine__remove_thread(struct machine *machine, struct thread *th)
 int machine__process_fork_event(struct machine *machine, union perf_event *event,
 				struct perf_sample *sample)
 {
+	bool user = event->header.type == PERF_RECORD_FORK_USER;
 	struct thread *thread = machine__find_thread(machine,
 						     event->fork.pid,
 						     event->fork.tid);
@@ -1738,7 +1739,7 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
 					 event->fork.tid);
 
 	if (thread == NULL || parent == NULL ||
-	    thread__fork(thread, parent, sample->time) < 0) {
+	    thread__fork(thread, parent, sample->time, user) < 0) {
 		dump_printf("problem processing PERF_RECORD_FORK, skipping event.\n");
 		err = -1;
 	}
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index df5e12b22905..a219dd119d95 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -801,6 +801,7 @@ static perf_event__swap_op perf_event__swap_ops[] = {
 	[PERF_RECORD_STAT_ROUND]	  = perf_event__stat_round_swap,
 	[PERF_RECORD_EVENT_UPDATE]	  = perf_event__event_update_swap,
 	[PERF_RECORD_TIME_CONV]		  = perf_event__all64_swap,
+	[PERF_RECORD_FORK_USER]		  = perf_event__task_swap,
 	[PERF_RECORD_HEADER_MAX]	  = NULL,
 };
 
@@ -1269,6 +1270,7 @@ static int machines__deliver_event(struct machines *machines,
 	case PERF_RECORD_NAMESPACES:
 		return tool->namespaces(tool, event, sample, machine);
 	case PERF_RECORD_FORK:
+	case PERF_RECORD_FORK_USER:
 		return tool->fork(tool, event, sample, machine);
 	case PERF_RECORD_EXIT:
 		return tool->exit(tool, event, sample, machine);
@@ -1398,6 +1400,12 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 	}
 }
 
+static bool process_user(union perf_event *event)
+{
+	return event->header.type >= PERF_RECORD_USER_TYPE_START &&
+	       event->header.type != PERF_RECORD_FORK_USER;
+}
+
 int perf_session__deliver_synth_event(struct perf_session *session,
 				      union perf_event *event,
 				      struct perf_sample *sample)
@@ -1407,7 +1415,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
 
 	events_stats__inc(&evlist->stats, event->header.type);
 
-	if (event->header.type >= PERF_RECORD_USER_TYPE_START)
+	if (process_user(event))
 		return perf_session__process_user_event(session, event, 0);
 
 	return machines__deliver_event(&session->machines, evlist, event, sample, tool, 0);
@@ -1492,7 +1500,7 @@ static s64 perf_session__process_event(struct perf_session *session,
 
 	events_stats__inc(&evlist->stats, event->header.type);
 
-	if (event->header.type >= PERF_RECORD_USER_TYPE_START)
+	if (process_user(event))
 		return perf_session__process_user_event(session, event, file_offset);
 
 	if (tool->ordered_events) {
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 2048d393ece6..2782d2626f09 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -332,10 +332,6 @@ static int thread__prepare_access(struct thread *thread)
 static int thread__clone_map_groups(struct thread *thread,
 				    struct thread *parent)
 {
-	/* This is new thread, we share map groups for process. */
-	if (thread->pid_ == parent->pid_)
-		return thread__prepare_access(thread);
-
 	if (thread->mg == parent->mg) {
 		pr_debug("broken map groups on thread %d/%d parent %d/%d\n",
 			 thread->pid_, thread->tid, parent->pid_, parent->tid);
@@ -343,13 +339,11 @@ static int thread__clone_map_groups(struct thread *thread,
 	}
 
 	/* But this one is new process, copy maps. */
-	if (map_groups__clone(thread, parent->mg) < 0)
-		return -ENOMEM;
-
-	return 0;
+	return map_groups__clone(thread, parent->mg);
 }
 
-int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp)
+int thread__fork(struct thread *thread, struct thread *parent,
+		 u64 timestamp, bool user)
 {
 	if (parent->comm_set) {
 		const char *comm = thread__comm_str(parent);
@@ -362,6 +356,15 @@ int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp)
 	}
 
 	thread->ppid = parent->tid;
+
+	/* This is new thread, we share map groups for process. */
+	if (thread->pid_ == parent->pid_)
+		return thread__prepare_access(thread);
+
+	/* Do not clone maps for user space fork event. */
+	if (user)
+		return 0;
+
 	return thread__clone_map_groups(thread, parent);
 }
 
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 07606aa6998d..827727300100 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -87,7 +87,8 @@ struct comm *thread__comm(const struct thread *thread);
 struct comm *thread__exec_comm(const struct thread *thread);
 const char *thread__comm_str(const struct thread *thread);
 int thread__insert_map(struct thread *thread, struct map *map);
-int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp);
+int thread__fork(struct thread *thread, struct thread *parent,
+		 u64 timestamp, bool user);
 size_t thread__fprintf(struct thread *thread, FILE *fp);
 
 struct thread *thread__main_thread(struct machine *machine, struct thread *thread);