Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1289967
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes |
| Date | 2015-12-11 23:30 +0100 |
| Message-ID | <qEEmm-21X-29@gated-at.bofh.it> (permalink) |
| References | <qDCwi-25G-3@gated-at.bofh.it> <qDNi2-Bz-17@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Em Wed, Dec 09, 2015 at 10:41:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> > General refcnt miscodings
> > =========================
> >
> > BTW, while applying this change, I've found that there are refcnt
> > coding mismatches in those code and most of the bugs come from those
> > mismatches.
> >
> > - The reference counter can be initialized by 0 or 1.
> > - If 0 is chosen, caller have to get it and free it if failed to
> > register appropriately.
> > - If 1 is chosen, caller doesn't need to get it, but when exits the
> > caller, it has to put it. (except for returning the object itself)
> > - The application should choose either one as its policy, to avoid
> > confusion.
> >
> > perf tools mixes it up (moreover, it initializes 2 in a case) and
> > caller usually forgets to put it (it is not surprising, because too
> > many "put" usually cause SEGV by accessing freed object.)
>
> Well, we should fix the bugs and document why some initialization is
> deemed better than a single initialization style.
> For instance, if we know that we will keep multiple references straight
> away, then we could init it with some value different that preferred,
> that I aggee, is 1, i.e. the usual way for some constructor is:
>
>
> struct foo *foo__new()
> {
> struct foo *f = malloc(sizeof (*f));
>
> if (f) {
> atomic_set(&f->refcnt, 1);
> }
>
> return f;
> }
>
> void *foo__delete(struct foo *f)
> {
> free(f);
> }
>
> void foo__put(struct foo *f)
> {
> if (f && atomic_dec_and_test(f->refcnt))
> foo__delete(f);
> }
>
>
> Then, when using if, and before adding it to any other tree, list, i.e.
> a container, we do:
>
> struct foo *f = foo__new();
>
> /*
> * assume f was allocated and then the function allocating it
> * failed, when it bails out it should just do:
> */
> out_error:
> foo__put(f);
>
> And that will make it hit zero, which will call foo__delete(), case
> closed.
>
> > As far as I can see, cgroup_sel, perf_mmap(this is initialized 0 or 2...),
> > thread, and comm_str are initialized by 0. Others are initialized by 1.
> >
> > So, I'd like to suggest that we choose one policy and cleanup the code.
> > I recommend to use init by 1 policy, because anyway caller has to get
>
> See above, if nothing else recommends using a different value, use 1.
So, I think I fixed the thread->refcnt case, please take a look at my
perf/core branch, more specifically this patch:
From a9a8442c64a86599600ecb2ae0f296b5c93e2a98 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Fri, 11 Dec 2015 19:11:23 -0300
Subject: [PATCH 1/1] perf thread: Fix reference count initial state
We should always return from thread__new(), the constructor, with the
object with a reference count of one, so that:
struct thread *thread = thread__new();
thread__put(thread);
Will call thread__delete().
If any reference is made to that 'thread' variable, it better use
thread__get(thread) to hold a reference.
We were returning with thread->refcnt set to zero, fix it and some cases
where thread__delete() was being called, which were not a problem
because just one reference was being used, now that we set it to 1, use
thread__put() instead.
Reported-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/n/tip-4b9mkuk66to4ecckpmpvqx6s@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/intel-pt.c | 4 ++--
tools/perf/util/machine.c | 19 ++++++++++++-------
tools/perf/util/thread.c | 10 ++++++++--
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 97f963a3dcb9..81a2eb77ba7f 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -1744,7 +1744,7 @@ static void intel_pt_free(struct perf_session *session)
auxtrace_heap__free(&pt->heap);
intel_pt_free_events(session);
session->auxtrace = NULL;
- thread__delete(pt->unknown_thread);
+ thread__put(pt->unknown_thread);
free(pt);
}
@@ -2153,7 +2153,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
return 0;
err_delete_thread:
- thread__delete(pt->unknown_thread);
+ thread__zput(pt->unknown_thread);
err_free_queues:
intel_pt_log_disable();
auxtrace_queues__free(&pt->queues);
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 1407d5107480..ad79297c76c8 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -352,13 +352,18 @@ static void machine__update_thread_pid(struct machine *machine,
}
th->mg = map_groups__get(leader->mg);
-
+out_put:
+ thread__put(leader);
return;
-
out_err:
pr_err("Failed to join map groups for %d:%d\n", th->pid_, th->tid);
+ goto out_put;
}
+/*
+ * Caller must eventually drop thread->refcnt returned with a successfull
+ * lookup/new thread inserted.
+ */
static struct thread *____machine__findnew_thread(struct machine *machine,
pid_t pid, pid_t tid,
bool create)
@@ -376,7 +381,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
if (th != NULL) {
if (th->tid == tid) {
machine__update_thread_pid(machine, th, pid);
- return th;
+ return thread__get(th);
}
machine->last_match = NULL;
@@ -389,7 +394,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
if (th->tid == tid) {
machine->last_match = th;
machine__update_thread_pid(machine, th, pid);
- return th;
+ return thread__get(th);
}
if (tid < th->tid)
@@ -417,7 +422,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine,
if (thread__init_map_groups(th, machine)) {
rb_erase_init(&th->rb_node, &machine->threads);
RB_CLEAR_NODE(&th->rb_node);
- thread__delete(th);
+ thread__put(th);
return NULL;
}
/*
@@ -441,7 +446,7 @@ struct thread *machine__findnew_thread(struct machine *machine, pid_t pid,
struct thread *th;
pthread_rwlock_wrlock(&machine->threads_lock);
- th = thread__get(__machine__findnew_thread(machine, pid, tid));
+ th = __machine__findnew_thread(machine, pid, tid);
pthread_rwlock_unlock(&machine->threads_lock);
return th;
}
@@ -451,7 +456,7 @@ struct thread *machine__find_thread(struct machine *machine, pid_t pid,
{
struct thread *th;
pthread_rwlock_rdlock(&machine->threads_lock);
- th = thread__get(____machine__findnew_thread(machine, pid, tid, false));
+ th = ____machine__findnew_thread(machine, pid, tid, false);
pthread_rwlock_unlock(&machine->threads_lock);
return th;
}
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 0a9ae8014729..dfd00c6dad6e 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -19,8 +19,10 @@ int thread__init_map_groups(struct thread *thread, struct machine *machine)
thread->mg = map_groups__new(machine);
} else {
leader = __machine__findnew_thread(machine, pid, pid);
- if (leader)
+ if (leader) {
thread->mg = map_groups__get(leader->mg);
+ thread__put(leader);
+ }
}
return thread->mg ? 0 : -1;
@@ -53,7 +55,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
goto err_thread;
list_add(&comm->list, &thread->comm_list);
- atomic_set(&thread->refcnt, 0);
+ atomic_set(&thread->refcnt, 1);
RB_CLEAR_NODE(&thread->rb_node);
}
@@ -95,6 +97,10 @@ struct thread *thread__get(struct thread *thread)
void thread__put(struct thread *thread)
{
if (thread && atomic_dec_and_test(&thread->refcnt)) {
+ /*
+ * Remove it from the dead_threads list, as last reference
+ * is gone.
+ */
list_del_init(&thread->node);
thread__delete(thread);
}
--
2.1.0
--
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/
Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread
[PATCH perf/core 00/22] perf refcnt debugger API and fixes Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
[PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
Re: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 15:20 +0100
RE: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 10:00 +0100
Re: [PATCH perf/core 14/22] perf: Fix dso__load_sym to put dso 'Arnaldo Carvalho de Melo' <acme@kernel.org> - 2015-12-10 20:30 +0100
[tip:perf/core] perf symbols: Fix dso__load_sym to put dso tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-12-14 09:20 +0100
[PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put used maps Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
Re: [PATCH perf/core 20/22] perf: Fix maps__fixup_overlappings to put used maps Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 16:20 +0100
[tip:perf/core] perf tools: Fix maps__fixup_overlappings to put used maps tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-12-10 09:20 +0100
[PATCH perf/core 04/22] perf refcnt: refcnt shows summary per object Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
[PATCH perf/core 22/22] perf: Fix write_numa_topology to put cpu_map instead of free Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
Re: [PATCH perf/core 22/22] perf: Fix write_numa_topology to put cpu_map instead of free Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 16:30 +0100
[tip:perf/core] perf tools: Fix write_numa_topology to put cpu_map instead of free tip-bot for Masami Hiramatsu <tipbot@zytor.com> - 2015-12-10 09:20 +0100
[PATCH perf/core 05/22] perf: make map to use refcnt Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
[PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> - 2015-12-09 03:30 +0100
Re: [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 15:40 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-09 14:50 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2015-12-10 04:40 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes Namhyung Kim <namhyung@kernel.org> - 2015-12-10 06:00 +0100
RE: [PATCH perf/core 00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 09:40 +0100
RE: [PATCH perf/core 00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> - 2015-12-10 12:10 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-10 14:00 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes 'Arnaldo Carvalho de Melo' <acme@kernel.org> - 2015-12-10 16:20 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 03:00 +0100
RE: [PATCH perf/core 00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> - 2015-12-11 03:10 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 03:30 +0100
RE: [PATCH perf/core 00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> - 2015-12-11 03:20 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 03:50 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes "Wangnan (F)" <wangnan0@huawei.com> - 2015-12-11 04:00 +0100
RE: [PATCH perf/core 00/22] perf refcnt debugger API and fixes 平松雅巳 / HIRAMATU，MASAMI <masami.hiramatsu.pt@hitachi.com> - 2015-12-11 05:00 +0100
Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-11 23:30 +0100
csiph-web