perf dsos: Tidy reference counting and locking

Move more functionality into dsos.c generally from machine.c, renaming
functions to match their new usage.

The find function is made to always "get" before returning a dso.

Reduce the scope of locks in vdso to match the locking paradigm.

Signed-off-by: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Anne Macedo <retpolanne@posteo.net>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Ben Gainey <ben.gainey@arm.com>
Cc: Changbin Du <changbin.du@huawei.com>
Cc: Chengen Du <chengen.du@canonical.com>
Cc: Colin Ian King <colin.i.king@gmail.com>
Cc: Ilkka Koskinen <ilkka@os.amperecomputing.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Clark <james.clark@arm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: K Prateek Nayak <kprateek.nayak@amd.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Leo Yan <leo.yan@linux.dev>
Cc: Li Dong <lidong@vivo.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paran Lee <p4ranlee@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: Song Liu <song@kernel.org>
Cc: Sun Haiyong <sunhaiyong@loongson.cn>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Cc: Yang Jihong <yangjihong1@huawei.com>
Cc: Yanteng Si <siyanteng@loongson.cn>
Cc: Yicong Yang <yangyicong@hisilicon.com>
Cc: zhaimingbing <zhaimingbing@cmss.chinamobile.com>
Link: https://lore.kernel.org/r/20240410064214.2755936-3-irogers@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index e65ef6762bedf2ae02f53e40c687ab3ed6b5b3c3..d269e09005a726b6134252cacdf69ec6a3066ae1 100644 (file)
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -181,7 +181,7 @@ struct dso *__dsos__findnew_link_by_longname_id(struct rb_root *root, struct dso
                         * at the end of the list of duplicates.
                         */
                        if (!dso || (dso == this))
-                               return this;    /* Find matching dso */
+                               return dso__get(this);  /* Find matching dso */
                        /*
                         * The core kernel DSOs may have duplicated long name.
                         * In this case, the short name should be different.
@@ -253,15 +253,20 @@ static struct dso *__dsos__find_id(struct dsos *dsos, const char *name, struct d
        if (cmp_short) {
                list_for_each_entry(pos, &dsos->head, node)
                        if (__dso__cmp_short_name(name, id, pos) == 0)
-                               return pos;
+                               return dso__get(pos);
                return NULL;
        }
        return __dsos__findnew_by_longname_id(&dsos->root, name, id);
 }
 
-struct dso *__dsos__find(struct dsos *dsos, const char *name, bool cmp_short)
+struct dso *dsos__find(struct dsos *dsos, const char *name, bool cmp_short)
 {
-       return __dsos__find_id(dsos, name, NULL, cmp_short);
+       struct dso *res;
+
+       down_read(&dsos->lock);
+       res = __dsos__find_id(dsos, name, NULL, cmp_short);
+       up_read(&dsos->lock);
+       return res;
 }
 
 static void dso__set_basename(struct dso *dso)
@@ -303,8 +308,6 @@ static struct dso *__dsos__addnew_id(struct dsos *dsos, const char *name, struct
        if (dso != NULL) {
                __dsos__add(dsos, dso);
                dso__set_basename(dso);
-               /* Put dso here because __dsos_add already got it */
-               dso__put(dso);
        }
        return dso;
 }
@@ -328,7 +331,7 @@ struct dso *dsos__findnew_id(struct dsos *dsos, const char *name, struct dso_id
 {
        struct dso *dso;
        down_write(&dsos->lock);
-       dso = dso__get(__dsos__findnew_id(dsos, name, id));
+       dso = __dsos__findnew_id(dsos, name, id);
        up_write(&dsos->lock);
        return dso;
 }
@@ -374,3 +377,59 @@ int __dsos__hit_all(struct dsos *dsos)
 
        return 0;
 }
+
+struct dso *dsos__findnew_module_dso(struct dsos *dsos,
+                                    struct machine *machine,
+                                    struct kmod_path *m,
+                                    const char *filename)
+{
+       struct dso *dso;
+
+       down_write(&dsos->lock);
+
+       dso = __dsos__find_id(dsos, m->name, NULL, /*cmp_short=*/true);
+       if (!dso) {
+               dso = __dsos__addnew(dsos, m->name);
+               if (dso == NULL)
+                       goto out_unlock;
+
+               dso__set_module_info(dso, m, machine);
+               dso__set_long_name(dso, strdup(filename), true);
+               dso->kernel = DSO_SPACE__KERNEL;
+       }
+
+out_unlock:
+       up_write(&dsos->lock);
+       return dso;
+}
+
+struct dso *dsos__find_kernel_dso(struct dsos *dsos)
+{
+       struct dso *dso, *res = NULL;
+
+       down_read(&dsos->lock);
+       list_for_each_entry(dso, &dsos->head, node) {
+               /*
+                * The cpumode passed to is_kernel_module is not the cpumode of
+                * *this* event. If we insist on passing correct cpumode to
+                * is_kernel_module, we should record the cpumode when we adding
+                * this dso to the linked list.
+                *
+                * However we don't really need passing correct cpumode.  We
+                * know the correct cpumode must be kernel mode (if not, we
+                * should not link it onto kernel_dsos list).
+                *
+                * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN.
+                * is_kernel_module() treats it as a kernel cpumode.
+                */
+               if (!dso->kernel ||
+                   is_kernel_module(dso->long_name,
+                                    PERF_RECORD_MISC_CPUMODE_UNKNOWN))
+                       continue;
+
+               res = dso__get(dso);
+               break;
+       }
+       up_read(&dsos->lock);
+       return res;
+}

diff --git a/tools/perf/util/dsos.h b/tools/perf/util/dsos.h
index 1c81ddf07f8f301fb73ade991e28a7c1cb605471..a7c7f723c5fff0115e54522c05eebf79c9547000 100644 (file)
--- a/tools/perf/util/dsos.h
+++ b/tools/perf/util/dsos.h
@@ -10,6 +10,8 @@
 
 struct dso;
 struct dso_id;
+struct kmod_path;
+struct machine;
 
 /*
  * DSOs are put into both a list for fast iteration and rbtree for fast
@@ -33,7 +35,7 @@ void dsos__exit(struct dsos *dsos);
 void __dsos__add(struct dsos *dsos, struct dso *dso);
 void dsos__add(struct dsos *dsos, struct dso *dso);
 struct dso *__dsos__addnew(struct dsos *dsos, const char *name);
-struct dso *__dsos__find(struct dsos *dsos, const char *name, bool cmp_short);
+struct dso *dsos__find(struct dsos *dsos, const char *name, bool cmp_short);
 
 struct dso *dsos__findnew_id(struct dsos *dsos, const char *name, struct dso_id *id);
  
@@ -48,4 +50,9 @@ size_t __dsos__fprintf(struct dsos *dsos, FILE *fp);
 
 int __dsos__hit_all(struct dsos *dsos);
 
+struct dso *dsos__findnew_module_dso(struct dsos *dsos, struct machine *machine,
+                                    struct kmod_path *m, const char *filename);
+
+struct dso *dsos__find_kernel_dso(struct dsos *dsos);
+
 #endif /* __PERF_DSOS */

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 848a94b8602b22e6488c028d7464356517bca9e2..e2c800a1449b0a95a3387269d2d479b679de9bc9 100644 (file)
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -646,31 +646,6 @@ int machine__process_lost_samples_event(struct machine *machine __maybe_unused,
        return 0;
 }
 
-static struct dso *machine__findnew_module_dso(struct machine *machine,
-                                              struct kmod_path *m,
-                                              const char *filename)
-{
-       struct dso *dso;
-
-       down_write(&machine->dsos.lock);
-
-       dso = __dsos__find(&machine->dsos, m->name, true);
-       if (!dso) {
-               dso = __dsos__addnew(&machine->dsos, m->name);
-               if (dso == NULL)
-                       goto out_unlock;
-
-               dso__set_module_info(dso, m, machine);
-               dso__set_long_name(dso, strdup(filename), true);
-               dso->kernel = DSO_SPACE__KERNEL;
-       }
-
-       dso__get(dso);
-out_unlock:
-       up_write(&machine->dsos.lock);
-       return dso;
-}
-
 int machine__process_aux_event(struct machine *machine __maybe_unused,
                               union perf_event *event)
 {
@@ -854,7 +829,7 @@ static struct map *machine__addnew_module_map(struct machine *machine, u64 start
        if (kmod_path__parse_name(&m, filename))
                return NULL;
 
-       dso = machine__findnew_module_dso(machine, &m, filename);
+       dso = dsos__findnew_module_dso(&machine->dsos, machine, &m, filename);
        if (dso == NULL)
                goto out;
 
@@ -1663,40 +1638,7 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
                 * Should be there already, from the build-id table in
                 * the header.
                 */
-               struct dso *kernel = NULL;
-               struct dso *dso;
-
-               down_read(&machine->dsos.lock);
-
-               list_for_each_entry(dso, &machine->dsos.head, node) {
-
-                       /*
-                        * The cpumode passed to is_kernel_module is not the
-                        * cpumode of *this* event. If we insist on passing
-                        * correct cpumode to is_kernel_module, we should
-                        * record the cpumode when we adding this dso to the
-                        * linked list.
-                        *
-                        * However we don't really need passing correct
-                        * cpumode.  We know the correct cpumode must be kernel
-                        * mode (if not, we should not link it onto kernel_dsos
-                        * list).
-                        *
-                        * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN.
-                        * is_kernel_module() treats it as a kernel cpumode.
-                        */
-
-                       if (!dso->kernel ||
-                           is_kernel_module(dso->long_name,
-                                            PERF_RECORD_MISC_CPUMODE_UNKNOWN))
-                               continue;
-
-
-                       kernel = dso__get(dso);
-                       break;
-               }
-
-               up_read(&machine->dsos.lock);
+               struct dso *kernel = dsos__find_kernel_dso(&machine->dsos);
 
                if (kernel == NULL)
                        kernel = machine__findnew_dso(machine, machine->mmap_name);

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a5d57c201a30c9633b028c6fc8ca6c97ea9785e8..cca871959f87abd9a0321dd5c4a44d6882ac32c3 100644 (file)
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -196,9 +196,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
                         * reading the header will have the build ID set and all future mmaps will
                         * have it missing.
                         */
-                       down_read(&machine->dsos.lock);
-                       header_bid_dso = __dsos__find(&machine->dsos, filename, false);
-                       up_read(&machine->dsos.lock);
+                       header_bid_dso = dsos__find(&machine->dsos, filename, false);
                        if (header_bid_dso && header_bid_dso->header_build_id) {
                                dso__set_build_id(dso, &header_bid_dso->bid);
                                dso->header_build_id = 1;

diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
index df8963796187dc69515114aff048a7a757299a7d..35532dcbff743d2960702f6c08d5c9ee8540edd6 100644 (file)
--- a/tools/perf/util/vdso.c
+++ b/tools/perf/util/vdso.c
@@ -133,8 +133,6 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
        if (dso != NULL) {
                __dsos__add(&machine->dsos, dso);
                dso__set_long_name(dso, long_name, false);
-               /* Put dso here because __dsos_add already got it */
-               dso__put(dso);
        }
 
        return dso;
@@ -252,17 +250,15 @@ static struct dso *__machine__findnew_compat(struct machine *machine,
        const char *file_name;
        struct dso *dso;
 
-       dso = __dsos__find(&machine->dsos, vdso_file->dso_name, true);
+       dso = dsos__find(&machine->dsos, vdso_file->dso_name, true);
        if (dso)
-               goto out;
+               return dso;
 
        file_name = vdso__get_compat_file(vdso_file);
        if (!file_name)
-               goto out;
+               return NULL;
 
-       dso = __machine__addnew_vdso(machine, vdso_file->dso_name, file_name);
-out:
-       return dso;
+       return __machine__addnew_vdso(machine, vdso_file->dso_name, file_name);
 }
 
 static int __machine__findnew_vdso_compat(struct machine *machine,
@@ -308,21 +304,21 @@ static struct dso *machine__find_vdso(struct machine *machine,
        dso_type = machine__thread_dso_type(machine, thread);
        switch (dso_type) {
        case DSO__TYPE_32BIT:
-               dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
+               dso = dsos__find(&machine->dsos, DSO__NAME_VDSO32, true);
                if (!dso) {
-                       dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO,
-                                          true);
+                       dso = dsos__find(&machine->dsos, DSO__NAME_VDSO,
+                                        true);
                        if (dso && dso_type != dso__type(dso, machine))
                                dso = NULL;
                }
                break;
        case DSO__TYPE_X32BIT:
-               dso = __dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
+               dso = dsos__find(&machine->dsos, DSO__NAME_VDSOX32, true);
                break;
        case DSO__TYPE_64BIT:
        case DSO__TYPE_UNKNOWN:
        default:
-               dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+               dso = dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
                break;
        }
 
@@ -334,37 +330,33 @@ struct dso *machine__findnew_vdso(struct machine *machine,
 {
        struct vdso_info *vdso_info;
        struct dso *dso = NULL;
+       char *file;
 
-       down_write(&machine->dsos.lock);
        if (!machine->vdso_info)
                machine->vdso_info = vdso_info__new();
 
        vdso_info = machine->vdso_info;
        if (!vdso_info)
-               goto out_unlock;
+               return NULL;
 
        dso = machine__find_vdso(machine, thread);
        if (dso)
-               goto out_unlock;
+               return dso;
 
 #if BITS_PER_LONG == 64
        if (__machine__findnew_vdso_compat(machine, thread, vdso_info, &dso))
-               goto out_unlock;
+               return dso;
 #endif
 
-       dso = __dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
-       if (!dso) {
-               char *file;
+       dso = dsos__find(&machine->dsos, DSO__NAME_VDSO, true);
+       if (dso)
+               return dso;
 
-               file = get_file(&vdso_info->vdso);
-               if (file)
-                       dso = __machine__addnew_vdso(machine, DSO__NAME_VDSO, file);
-       }
+       file = get_file(&vdso_info->vdso);
+       if (!file)
+               return NULL;
 
-out_unlock:
-       dso__get(dso);
-       up_write(&machine->dsos.lock);
-       return dso;
+       return __machine__addnew_vdso(machine, DSO__NAME_VDSO, file);
 }
 
 bool dso__is_vdso(struct dso *dso)