| Message ID | 20220823220922.256001-8-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Mutex wrapper, locking and memory leak fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-PR | fail | merge-conflict |
On 24/08/22 01:09, Ian Rogers wrote: > Switch to the use of mutex wrappers that provide better error checking > for synth_lock. It would be better to distinguish patches that make drop-in replacements from patches like this that change logic. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/builtin-record.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 4713f0f3a6cf..02eb85677e99 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -21,6 +21,7 @@ > #include "util/evsel.h" > #include "util/debug.h" > #include "util/mmap.h" > +#include "util/mutex.h" > #include "util/target.h" > #include "util/session.h" > #include "util/tool.h" > @@ -608,17 +609,18 @@ static int process_synthesized_event(struct perf_tool *tool, > return record__write(rec, NULL, event, event->header.size); > } > > +static struct mutex synth_lock; > + > static int process_locked_synthesized_event(struct perf_tool *tool, > union perf_event *event, > struct perf_sample *sample __maybe_unused, > struct machine *machine __maybe_unused) > { > - static pthread_mutex_t synth_lock = PTHREAD_MUTEX_INITIALIZER; > int ret; > > - pthread_mutex_lock(&synth_lock); > + mutex_lock(&synth_lock); > ret = process_synthesized_event(tool, event, sample, machine); > - pthread_mutex_unlock(&synth_lock); > + mutex_unlock(&synth_lock); > return ret; > } > > @@ -1917,6 +1919,7 @@ static int record__synthesize(struct record *rec, bool tail) > } > > if (rec->opts.nr_threads_synthesize > 1) { > + mutex_init(&synth_lock, /*pshared=*/false); It would be better to have mutex_init() and mutex_init_shared() since /*pshared=*/true is rarely used. > perf_set_multithreaded(); > f = process_locked_synthesized_event; > } > @@ -1930,8 +1933,10 @@ static int record__synthesize(struct record *rec, bool tail) > rec->opts.nr_threads_synthesize); > } > > - if (rec->opts.nr_threads_synthesize > 1) > + if (rec->opts.nr_threads_synthesize > 1) { > perf_set_singlethreaded(); > + mutex_destroy(&synth_lock); > + } > > out: > return err;
On Wed, Aug 24, 2022 at 3:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 24/08/22 01:09, Ian Rogers wrote: > > Switch to the use of mutex wrappers that provide better error checking > > for synth_lock. > > It would be better to distinguish patches that make drop-in > replacements from patches like this that change logic. The only change here is PTHREAD_MUTEX_INITIALIZER to mutex_init because PTHREAD_MUTEX_INITIALIZER doesn't have error checking. The two are morally equivalent and so no logic change is intended - although one may inadvertently happen by the moving initialization from compile time to runtime. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/builtin-record.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > > index 4713f0f3a6cf..02eb85677e99 100644 > > --- a/tools/perf/builtin-record.c > > +++ b/tools/perf/builtin-record.c > > @@ -21,6 +21,7 @@ > > #include "util/evsel.h" > > #include "util/debug.h" > > #include "util/mmap.h" > > +#include "util/mutex.h" > > #include "util/target.h" > > #include "util/session.h" > > #include "util/tool.h" > > @@ -608,17 +609,18 @@ static int process_synthesized_event(struct perf_tool *tool, > > return record__write(rec, NULL, event, event->header.size); > > } > > > > +static struct mutex synth_lock; > > + > > static int process_locked_synthesized_event(struct perf_tool *tool, > > union perf_event *event, > > struct perf_sample *sample __maybe_unused, > > struct machine *machine __maybe_unused) > > { > > - static pthread_mutex_t synth_lock = PTHREAD_MUTEX_INITIALIZER; > > int ret; > > > > - pthread_mutex_lock(&synth_lock); > > + mutex_lock(&synth_lock); > > ret = process_synthesized_event(tool, event, sample, machine); > > - pthread_mutex_unlock(&synth_lock); > > + mutex_unlock(&synth_lock); > > return ret; > > } > > > > @@ -1917,6 +1919,7 @@ static int record__synthesize(struct record *rec, bool tail) > > } > > > > if (rec->opts.nr_threads_synthesize > 1) { > > + mutex_init(&synth_lock, /*pshared=*/false); > > It would be better to have mutex_init() and mutex_init_shared() > since /*pshared=*/true is rarely used. Will change in v3. Thanks, Ian > > perf_set_multithreaded(); > > f = process_locked_synthesized_event; > > } > > @@ -1930,8 +1933,10 @@ static int record__synthesize(struct record *rec, bool tail) > > rec->opts.nr_threads_synthesize); > > } > > > > - if (rec->opts.nr_threads_synthesize > 1) > > + if (rec->opts.nr_threads_synthesize > 1) { > > perf_set_singlethreaded(); > > + mutex_destroy(&synth_lock); > > + } > > > > out: > > return err; >
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 4713f0f3a6cf..02eb85677e99 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -21,6 +21,7 @@ #include "util/evsel.h" #include "util/debug.h" #include "util/mmap.h" +#include "util/mutex.h" #include "util/target.h" #include "util/session.h" #include "util/tool.h" @@ -608,17 +609,18 @@ static int process_synthesized_event(struct perf_tool *tool, return record__write(rec, NULL, event, event->header.size); } +static struct mutex synth_lock; + static int process_locked_synthesized_event(struct perf_tool *tool, union perf_event *event, struct perf_sample *sample __maybe_unused, struct machine *machine __maybe_unused) { - static pthread_mutex_t synth_lock = PTHREAD_MUTEX_INITIALIZER; int ret; - pthread_mutex_lock(&synth_lock); + mutex_lock(&synth_lock); ret = process_synthesized_event(tool, event, sample, machine); - pthread_mutex_unlock(&synth_lock); + mutex_unlock(&synth_lock); return ret; } @@ -1917,6 +1919,7 @@ static int record__synthesize(struct record *rec, bool tail) } if (rec->opts.nr_threads_synthesize > 1) { + mutex_init(&synth_lock, /*pshared=*/false); perf_set_multithreaded(); f = process_locked_synthesized_event; } @@ -1930,8 +1933,10 @@ static int record__synthesize(struct record *rec, bool tail) rec->opts.nr_threads_synthesize); } - if (rec->opts.nr_threads_synthesize > 1) + if (rec->opts.nr_threads_synthesize > 1) { perf_set_singlethreaded(); + mutex_destroy(&synth_lock); + } out: return err;
Switch to the use of mutex wrappers that provide better error checking for synth_lock. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/builtin-record.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)