Hi Arnaldo, On Tue, Nov 12, 2013 at 08:57:00AM -0300, Arnaldo Carvalho de Melo wrote: > So this becomes the first part of this patch, split from yours and > massaged a bit so that by looking at the patch it becomes quickly clear > what it is doing, please let me now if I can keep this as-is (with your > authorship, etc).
Looks good to me. But I just have a nitpick, please see below. > > I'll test this all out after finishing the next part of the split up. > > commit 296f6ce34590099740bfe03ced37f6f53a0133f8 > Author: Namhyung Kim <namhy...@kernel.org> > Date: Tue Nov 12 08:51:45 2013 -0300 > > perf trace: Separate tp syscall field caching into init routine to be > reused > > We need to set this in evsels coming out of a perf.data file header, not > just for new ones created for live sessions. > > So separate the code that caches the syscall entry/exit tracepoint > format fields into a new function that will be used in the next > changeset. > > Signed-off-by: Namhyung Kim <namhy...@kernel.org> > Cc: Adrian Hunter <adrian.hun...@intel.com> > Cc: David Ahern <dsah...@gmail.com> > Cc: Frederic Weisbecker <fweis...@gmail.com> > Cc: Jiri Olsa <jo...@redhat.com> > Cc: Mike Galbraith <efa...@gmx.de> > Cc: Paul Mackerras <pau...@samba.org> > Cc: Peter Zijlstra <pet...@infradead.org> > Cc: Stephane Eranian <eran...@google.com> > Link: http://lkml.kernel.org/n/tip-iv4vbx2064hc2drv38egq...@git.kernel.org > Signed-off-by: Arnaldo Carvalho de Melo <a...@redhat.com> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c > index aeb6296a76bd..3fa1dce6d43e 100644 > --- a/tools/perf/builtin-trace.c > +++ b/tools/perf/builtin-trace.c > @@ -149,20 +149,32 @@ static void perf_evsel__delete_priv(struct perf_evsel > *evsel) > perf_evsel__delete(evsel); > } > > +static int perf_evsel__init_syscall_tp(struct perf_evsel *evsel, void > *handler) > +{ > + evsel->priv = malloc(sizeof(struct syscall_tp)); > + if (evsel->priv != NULL) { > + if (perf_evsel__init_sc_tp_uint_field(evsel, id)) > + goto out_delete; > + > + evsel->handler = handler; > + return 0; > + } > + > + return -ENOMEM; > + > +out_delete: > + free(evsel->priv); > + evsel->priv = NULL; Is this part needed? I can see that perf_evsel__delete_priv() can do it for you anyway. Yes I know it's needed for my later change, but I think we do it a bit differently. And again, is perf_evsel__delete_priv() needed? Isn't the ->priv is not used for anything else? Why not just letting perf_evsel__delete() handle this transparently? Looking at the source, evsel->priv is a member of union and the other member ->id_offset is used in when dealing with the perf file header and it doesn't allocate memory. Hmm, how about adding a new field like ->needs_free_priv then? Anyway, it should definitely be a different change, I just want to raise an issue after seeing it. Thanks, Namhyung > + return -ENOENT; > +} > + > static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, > void *handler) > { > struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction); > > if (evsel) { > - evsel->priv = malloc(sizeof(struct syscall_tp)); > - > - if (evsel->priv == NULL) > + if (perf_evsel__init_syscall_tp(evsel, handler)) > goto out_delete; > - > - if (perf_evsel__init_sc_tp_uint_field(evsel, id)) > - goto out_delete; > - > - evsel->handler = handler; > } > > return evsel; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
