* [PATCH 1/1] perf,tools: open event on evsel cpus and threads @ 2015-08-21 6:23 kan.liang 2015-08-21 13:54 ` Jiri Olsa 2015-09-01 8:31 ` [tip:perf/urgent] perf evlist: Open " tip-bot for Kan Liang 0 siblings, 2 replies; 22+ messages in thread From: kan.liang @ 2015-08-21 6:23 UTC (permalink / raw) To: acme, jolsa; +Cc: linux-kernel, Kan Liang From: Kan Liang <kan.liang@intel.com> evsel may have different cpus and threads as evlist's. Use it's own cpus and threads, when open evsel in perf record. Signed-off-by: Kan Liang <kan.liang@intel.com> --- tools/perf/builtin-record.c | 2 +- tools/perf/util/evlist.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 25cf6b4..a0178bf 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -279,7 +279,7 @@ static int record__open(struct record *rec) evlist__for_each(evlist, pos) { try_again: - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { if (verbose) ui__warning("%s\n", msg); diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 373f65b..be6fde9 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e if (evsel->filter == NULL) continue; + /* + * filters only work for tracepoint event, which doesn't have cpu limit. + * So evlist and evsel should always be same. + */ err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); if (err) { *err_evsel = evsel; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads 2015-08-21 6:23 [PATCH 1/1] perf,tools: open event on evsel cpus and threads kan.liang @ 2015-08-21 13:54 ` Jiri Olsa 2015-08-31 20:31 ` Arnaldo Carvalho de Melo 2015-09-01 8:31 ` [tip:perf/urgent] perf evlist: Open " tip-bot for Kan Liang 1 sibling, 1 reply; 22+ messages in thread From: Jiri Olsa @ 2015-08-21 13:54 UTC (permalink / raw) To: kan.liang; +Cc: acme, jolsa, linux-kernel On Fri, Aug 21, 2015 at 02:23:14AM -0400, kan.liang@intel.com wrote: > From: Kan Liang <kan.liang@intel.com> > > evsel may have different cpus and threads as evlist's. > Use it's own cpus and threads, when open evsel in perf record. Acked-by: Jiri Olsa <jolsa@kernel.org> thanks, jirka > > Signed-off-by: Kan Liang <kan.liang@intel.com> > --- > tools/perf/builtin-record.c | 2 +- > tools/perf/util/evlist.c | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index 25cf6b4..a0178bf 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > evlist__for_each(evlist, pos) { > try_again: > - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { > + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { > if (verbose) > ui__warning("%s\n", msg); > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 373f65b..be6fde9 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e > if (evsel->filter == NULL) > continue; > > + /* > + * filters only work for tracepoint event, which doesn't have cpu limit. > + * So evlist and evsel should always be same. > + */ > err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); > if (err) { > *err_evsel = evsel; > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads 2015-08-21 13:54 ` Jiri Olsa @ 2015-08-31 20:31 ` Arnaldo Carvalho de Melo 2015-08-31 21:06 ` Liang, Kan 0 siblings, 1 reply; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-08-31 20:31 UTC (permalink / raw) To: Jiri Olsa; +Cc: kan.liang, jolsa, linux-kernel Em Fri, Aug 21, 2015 at 03:54:36PM +0200, Jiri Olsa escreveu: > On Fri, Aug 21, 2015 at 02:23:14AM -0400, kan.liang@intel.com wrote: > > From: Kan Liang <kan.liang@intel.com> > > > > evsel may have different cpus and threads as evlist's. > > Use it's own cpus and threads, when open evsel in perf record. > > Acked-by: Jiri Olsa <jolsa@kernel.org> Applying, I wonder if this isn't affecting other tools as well... And also perf_evlist__open(), that has to be fixed as well, right? - Arnaldo > thanks, > jirka > > > > > Signed-off-by: Kan Liang <kan.liang@intel.com> > > --- > > tools/perf/builtin-record.c | 2 +- > > tools/perf/util/evlist.c | 4 ++++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > > index 25cf6b4..a0178bf 100644 > > --- a/tools/perf/builtin-record.c > > +++ b/tools/perf/builtin-record.c > > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > > > evlist__for_each(evlist, pos) { > > try_again: > > - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { > > + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > > if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { > > if (verbose) > > ui__warning("%s\n", msg); > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > > index 373f65b..be6fde9 100644 > > --- a/tools/perf/util/evlist.c > > +++ b/tools/perf/util/evlist.c > > @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e > > if (evsel->filter == NULL) > > continue; > > > > + /* > > + * filters only work for tracepoint event, which doesn't have cpu limit. > > + * So evlist and evsel should always be same. > > + */ > > err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); > > if (err) { > > *err_evsel = evsel; > > -- > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 1/1] perf,tools: open event on evsel cpus and threads 2015-08-31 20:31 ` Arnaldo Carvalho de Melo @ 2015-08-31 21:06 ` Liang, Kan 2015-08-31 21:14 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 22+ messages in thread From: Liang, Kan @ 2015-08-31 21:06 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Jiri Olsa; +Cc: jolsa, linux-kernel > Em Fri, Aug 21, 2015 at 03:54:36PM +0200, Jiri Olsa escreveu: > > On Fri, Aug 21, 2015 at 02:23:14AM -0400, kan.liang@intel.com wrote: > > > From: Kan Liang <kan.liang@intel.com> > > > > > > evsel may have different cpus and threads as evlist's. > > > Use it's own cpus and threads, when open evsel in perf record. > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > Applying, I wonder if this isn't affecting other tools as well... And also No, it doesn't affect other tools. > perf_evlist__open(), that has to be fixed as well, right? > Umm... Right. We should fix it in perf_evlist__open as well. IIRC, Jirka once planned to send out a cleanup patch separately for this kind of issue (perf_evsel__enable also need to be fixed) on other tools. Thanks, Kan > - Arnaldo > > > thanks, > > jirka > > > > > > > > Signed-off-by: Kan Liang <kan.liang@intel.com> > > > --- > > > tools/perf/builtin-record.c | 2 +- > > > tools/perf/util/evlist.c | 4 ++++ > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/builtin-record.c > > > b/tools/perf/builtin-record.c index 25cf6b4..a0178bf 100644 > > > --- a/tools/perf/builtin-record.c > > > +++ b/tools/perf/builtin-record.c > > > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > > > > > evlist__for_each(evlist, pos) { > > > try_again: > > > - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < > 0) { > > > + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > > > if (perf_evsel__fallback(pos, errno, msg, > sizeof(msg))) { > > > if (verbose) > > > ui__warning("%s\n", msg); > > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > > > index 373f65b..be6fde9 100644 > > > --- a/tools/perf/util/evlist.c > > > +++ b/tools/perf/util/evlist.c > > > @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct > perf_evlist *evlist, struct perf_evsel **e > > > if (evsel->filter == NULL) > > > continue; > > > > > > + /* > > > + * filters only work for tracepoint event, which doesn't > have cpu limit. > > > + * So evlist and evsel should always be same. > > > + */ > > > err = perf_evsel__apply_filter(evsel, ncpus, nthreads, > evsel->filter); > > > if (err) { > > > *err_evsel = evsel; > > > -- > > > 1.8.3.1 > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads 2015-08-31 21:06 ` Liang, Kan @ 2015-08-31 21:14 ` Arnaldo Carvalho de Melo 2015-08-31 21:28 ` Liang, Kan 0 siblings, 1 reply; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-08-31 21:14 UTC (permalink / raw) To: Liang, Kan; +Cc: Jiri Olsa, jolsa, linux-kernel Em Mon, Aug 31, 2015 at 09:06:29PM +0000, Liang, Kan escreveu: > > > > Em Fri, Aug 21, 2015 at 03:54:36PM +0200, Jiri Olsa escreveu: > > > On Fri, Aug 21, 2015 at 02:23:14AM -0400, kan.liang@intel.com wrote: > > > > From: Kan Liang <kan.liang@intel.com> > > > > > > > > evsel may have different cpus and threads as evlist's. > > > > Use it's own cpus and threads, when open evsel in perf record. > > > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > Applying, I wonder if this isn't affecting other tools as well... And also > > No, it doesn't affect other tools. > > > perf_evlist__open(), that has to be fixed as well, right? > > > > Umm... Right. > We should fix it in perf_evlist__open as well. > IIRC, Jirka once planned to send out a cleanup patch separately for > this kind of issue (perf_evsel__enable also need to be fixed) on other > tools. So now that we have a backpointer, perhaps we should have a: struct thread_map *perf_evsel__threads(struct perf_evsel *evsel) { return evsel->threads ?: evsel->evlist ? evsel->evlist->threads : NULL; } Ditto for cpus and then change all occurrences of evsel->threads with perf_evsel__threads(), right? > Thanks, > Kan > > > - Arnaldo > > > > > thanks, > > > jirka > > > > > > > > > > > Signed-off-by: Kan Liang <kan.liang@intel.com> > > > > --- > > > > tools/perf/builtin-record.c | 2 +- > > > > tools/perf/util/evlist.c | 4 ++++ > > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/tools/perf/builtin-record.c > > > > b/tools/perf/builtin-record.c index 25cf6b4..a0178bf 100644 > > > > --- a/tools/perf/builtin-record.c > > > > +++ b/tools/perf/builtin-record.c > > > > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > > > > > > > evlist__for_each(evlist, pos) { > > > > try_again: > > > > - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < > > 0) { > > > > + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > > > > if (perf_evsel__fallback(pos, errno, msg, > > sizeof(msg))) { > > > > if (verbose) > > > > ui__warning("%s\n", msg); > > > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > > > > index 373f65b..be6fde9 100644 > > > > --- a/tools/perf/util/evlist.c > > > > +++ b/tools/perf/util/evlist.c > > > > @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct > > perf_evlist *evlist, struct perf_evsel **e > > > > if (evsel->filter == NULL) > > > > continue; > > > > > > > > + /* > > > > + * filters only work for tracepoint event, which doesn't > > have cpu limit. > > > > + * So evlist and evsel should always be same. > > > > + */ > > > > err = perf_evsel__apply_filter(evsel, ncpus, nthreads, > > evsel->filter); > > > > if (err) { > > > > *err_evsel = evsel; > > > > -- > > > > 1.8.3.1 > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 1/1] perf,tools: open event on evsel cpus and threads 2015-08-31 21:14 ` Arnaldo Carvalho de Melo @ 2015-08-31 21:28 ` Liang, Kan 2015-08-31 21:34 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 22+ messages in thread From: Liang, Kan @ 2015-08-31 21:28 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Jiri Olsa, jolsa, linux-kernel > > Em Mon, Aug 31, 2015 at 09:06:29PM +0000, Liang, Kan escreveu: > > > > > > > Em Fri, Aug 21, 2015 at 03:54:36PM +0200, Jiri Olsa escreveu: > > > > On Fri, Aug 21, 2015 at 02:23:14AM -0400, kan.liang@intel.com wrote: > > > > > From: Kan Liang <kan.liang@intel.com> > > > > > > > > > > evsel may have different cpus and threads as evlist's. > > > > > Use it's own cpus and threads, when open evsel in perf record. > > > > > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > > > Applying, I wonder if this isn't affecting other tools as well... > > > And also > > > > No, it doesn't affect other tools. > > > > > perf_evlist__open(), that has to be fixed as well, right? > > > > > > > Umm... Right. > > We should fix it in perf_evlist__open as well. > > IIRC, Jirka once planned to send out a cleanup patch separately for > > this kind of issue (perf_evsel__enable also need to be fixed) on other > > tools. > > So now that we have a backpointer, perhaps we should have a: > > struct thread_map *perf_evsel__threads(struct perf_evsel *evsel) { > return evsel->threads ?: evsel->evlist ? evsel->evlist->threads : > NULL; } > > Ditto for cpus and then change all occurrences of evsel->threads with > perf_evsel__threads(), right? No. We need to change all evlist->threads with evsel->threads. Ditto for cpus. The event should use its own thread/cpu list. > > > Thanks, > > Kan > > > > > - Arnaldo > > > > > > > thanks, > > > > jirka > > > > > > > > > > > > > > Signed-off-by: Kan Liang <kan.liang@intel.com> > > > > > --- > > > > > tools/perf/builtin-record.c | 2 +- > > > > > tools/perf/util/evlist.c | 4 ++++ > > > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/tools/perf/builtin-record.c > > > > > b/tools/perf/builtin-record.c index 25cf6b4..a0178bf 100644 > > > > > --- a/tools/perf/builtin-record.c > > > > > +++ b/tools/perf/builtin-record.c > > > > > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > > > > > > > > > evlist__for_each(evlist, pos) { > > > > > try_again: > > > > > - if (perf_evsel__open(pos, evlist->cpus, evlist- > >threads) < > > > 0) { > > > > > + if (perf_evsel__open(pos, pos->cpus, pos- > >threads) < 0) { > > > > > if (perf_evsel__fallback(pos, errno, msg, > > > sizeof(msg))) { > > > > > if (verbose) > > > > > ui__warning("%s\n", msg); > > > > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > > > > > index 373f65b..be6fde9 100644 > > > > > --- a/tools/perf/util/evlist.c > > > > > +++ b/tools/perf/util/evlist.c > > > > > @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct > > > perf_evlist *evlist, struct perf_evsel **e > > > > > if (evsel->filter == NULL) > > > > > continue; > > > > > > > > > > + /* > > > > > + * filters only work for tracepoint event, which > doesn't > > > have cpu limit. > > > > > + * So evlist and evsel should always be same. > > > > > + */ > > > > > err = perf_evsel__apply_filter(evsel, ncpus, > nthreads, > > > evsel->filter); > > > > > if (err) { > > > > > *err_evsel = evsel; > > > > > -- > > > > > 1.8.3.1 > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads 2015-08-31 21:28 ` Liang, Kan @ 2015-08-31 21:34 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-08-31 21:34 UTC (permalink / raw) To: Liang, Kan; +Cc: Jiri Olsa, jolsa, linux-kernel Em Mon, Aug 31, 2015 at 09:28:43PM +0000, Liang, Kan escreveu: > > > > > > > Em Mon, Aug 31, 2015 at 09:06:29PM +0000, Liang, Kan escreveu: > > > > > > > > > > Em Fri, Aug 21, 2015 at 03:54:36PM +0200, Jiri Olsa escreveu: > > > > > On Fri, Aug 21, 2015 at 02:23:14AM -0400, kan.liang@intel.com wrote: > > > > > > From: Kan Liang <kan.liang@intel.com> > > > > > > > > > > > > evsel may have different cpus and threads as evlist's. > > > > > > Use it's own cpus and threads, when open evsel in perf record. > > > > > > > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > > > > > Applying, I wonder if this isn't affecting other tools as well... > > > > And also > > > > > > No, it doesn't affect other tools. > > > > > > > perf_evlist__open(), that has to be fixed as well, right? > > > > > > > > > > Umm... Right. > > > We should fix it in perf_evlist__open as well. > > > IIRC, Jirka once planned to send out a cleanup patch separately for > > > this kind of issue (perf_evsel__enable also need to be fixed) on other > > > tools. > > > > So now that we have a backpointer, perhaps we should have a: > > > > struct thread_map *perf_evsel__threads(struct perf_evsel *evsel) { > > return evsel->threads ?: evsel->evlist ? evsel->evlist->threads : > > NULL; } > > > > Ditto for cpus and then change all occurrences of evsel->threads with > > perf_evsel__threads(), right? > > No. We need to change all evlist->threads with evsel->threads. > Ditto for cpus. > The event should use its own thread/cpu list. Which is equivalent, but IIRC doing it the way you do is better because we do that forwarding already, right? Lemme check... Yeah, it is done in perf_evlist__propagate_maps(), so, as long as that function was called, we can access evsel->{threads,cpus} and it will use evlist->{threads,cpus} if those entries were not set before perf_evlist__propagate_maps() was called. - Arnaldo > > > Thanks, > > > Kan > > > > > > > - Arnaldo > > > > > > > > > thanks, > > > > > jirka > > > > > > > > > > > > > > > > > Signed-off-by: Kan Liang <kan.liang@intel.com> > > > > > > --- > > > > > > tools/perf/builtin-record.c | 2 +- > > > > > > tools/perf/util/evlist.c | 4 ++++ > > > > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/tools/perf/builtin-record.c > > > > > > b/tools/perf/builtin-record.c index 25cf6b4..a0178bf 100644 > > > > > > --- a/tools/perf/builtin-record.c > > > > > > +++ b/tools/perf/builtin-record.c > > > > > > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > > > > > > > > > > > evlist__for_each(evlist, pos) { > > > > > > try_again: > > > > > > - if (perf_evsel__open(pos, evlist->cpus, evlist- > > >threads) < > > > > 0) { > > > > > > + if (perf_evsel__open(pos, pos->cpus, pos- > > >threads) < 0) { > > > > > > if (perf_evsel__fallback(pos, errno, msg, > > > > sizeof(msg))) { > > > > > > if (verbose) > > > > > > ui__warning("%s\n", msg); > > > > > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > > > > > > index 373f65b..be6fde9 100644 > > > > > > --- a/tools/perf/util/evlist.c > > > > > > +++ b/tools/perf/util/evlist.c > > > > > > @@ -1179,6 +1179,10 @@ int perf_evlist__apply_filters(struct > > > > perf_evlist *evlist, struct perf_evsel **e > > > > > > if (evsel->filter == NULL) > > > > > > continue; > > > > > > > > > > > > + /* > > > > > > + * filters only work for tracepoint event, which > > doesn't > > > > have cpu limit. > > > > > > + * So evlist and evsel should always be same. > > > > > > + */ > > > > > > err = perf_evsel__apply_filter(evsel, ncpus, > > nthreads, > > > > evsel->filter); > > > > > > if (err) { > > > > > > *err_evsel = evsel; > > > > > > -- > > > > > > 1.8.3.1 > > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-08-21 6:23 [PATCH 1/1] perf,tools: open event on evsel cpus and threads kan.liang 2015-08-21 13:54 ` Jiri Olsa @ 2015-09-01 8:31 ` tip-bot for Kan Liang 2015-09-03 13:34 ` Adrian Hunter 1 sibling, 1 reply; 22+ messages in thread From: tip-bot for Kan Liang @ 2015-09-01 8:31 UTC (permalink / raw) To: linux-tip-commits; +Cc: hpa, acme, tglx, jolsa, kan.liang, linux-kernel, mingo Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e Author: Kan Liang <kan.liang@intel.com> AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 perf evlist: Open event on evsel cpus and threads An evsel may have different cpus and threads than the evlist it is in. Use it's own cpus and threads, when opening the evsel in 'perf record'. Signed-off-by: Kan Liang <kan.liang@intel.com> Cc: Jiri Olsa <jolsa@kernel.org> Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- tools/perf/builtin-record.c | 2 +- tools/perf/util/evlist.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index a660022..1d14f38 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -279,7 +279,7 @@ static int record__open(struct record *rec) evlist__for_each(evlist, pos) { try_again: - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { if (verbose) ui__warning("%s\n", msg); diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 8d00039..d51a520 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -1181,6 +1181,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e if (evsel->filter == NULL) continue; + /* + * filters only work for tracepoint event, which doesn't have cpu limit. + * So evlist and evsel should always be same. + */ err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); if (err) { *err_evsel = evsel; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-01 8:31 ` [tip:perf/urgent] perf evlist: Open " tip-bot for Kan Liang @ 2015-09-03 13:34 ` Adrian Hunter 2015-09-03 15:27 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 22+ messages in thread From: Adrian Hunter @ 2015-09-03 13:34 UTC (permalink / raw) To: acme, kan.liang, mingo; +Cc: jolsa, linux-kernel On 01/09/15 11:31, tip-bot for Kan Liang wrote: > Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e > Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e > Author: Kan Liang <kan.liang@intel.com> > AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 > Committer: Arnaldo Carvalho de Melo <acme@redhat.com> > CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 > > perf evlist: Open event on evsel cpus and threads > > An evsel may have different cpus and threads than the evlist it is in. > > Use it's own cpus and threads, when opening the evsel in 'perf record'. > > Signed-off-by: Kan Liang <kan.liang@intel.com> > Cc: Jiri Olsa <jolsa@kernel.org> > Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> Just noticed this breaks Intel PT. Will have to investigate further. > --- > tools/perf/builtin-record.c | 2 +- > tools/perf/util/evlist.c | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index a660022..1d14f38 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > evlist__for_each(evlist, pos) { > try_again: > - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { > + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { > if (verbose) > ui__warning("%s\n", msg); > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 8d00039..d51a520 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -1181,6 +1181,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e > if (evsel->filter == NULL) > continue; > > + /* > + * filters only work for tracepoint event, which doesn't have cpu limit. > + * So evlist and evsel should always be same. > + */ > err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); > if (err) { > *err_evsel = evsel; > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 13:34 ` Adrian Hunter @ 2015-09-03 15:27 ` Arnaldo Carvalho de Melo 2015-09-03 16:23 ` Adrian Hunter 0 siblings, 1 reply; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-03 15:27 UTC (permalink / raw) To: Adrian Hunter; +Cc: kan.liang, mingo, jolsa, linux-kernel Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: > On 01/09/15 11:31, tip-bot for Kan Liang wrote: > > Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e > > Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e > > Author: Kan Liang <kan.liang@intel.com> > > AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 > > Committer: Arnaldo Carvalho de Melo <acme@redhat.com> > > CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 > > > > perf evlist: Open event on evsel cpus and threads > > > > An evsel may have different cpus and threads than the evlist it is in. > > > > Use it's own cpus and threads, when opening the evsel in 'perf record'. > > > > Signed-off-by: Kan Liang <kan.liang@intel.com> > > Cc: Jiri Olsa <jolsa@kernel.org> > > Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > Just noticed this breaks Intel PT. Will have to investigate further. What kind of breakage? It all should be equivalent to before, its just that it uses evsel->{threads,cpus} while before it was using evlist->{threads,cpus}, but that should point to the same thing if that perf_evlist__propagate_maps() method was called, so I assume this is some segfault? Something we could catch in a 'test' entry? Even if that required Intel PT hardware that would be something important to have, all this stuff is growing in complexity, we need those tests... - Arnaldo > > --- > > tools/perf/builtin-record.c | 2 +- > > tools/perf/util/evlist.c | 4 ++++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > > index a660022..1d14f38 100644 > > --- a/tools/perf/builtin-record.c > > +++ b/tools/perf/builtin-record.c > > @@ -279,7 +279,7 @@ static int record__open(struct record *rec) > > > > evlist__for_each(evlist, pos) { > > try_again: > > - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { > > + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > > if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { > > if (verbose) > > ui__warning("%s\n", msg); > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > > index 8d00039..d51a520 100644 > > --- a/tools/perf/util/evlist.c > > +++ b/tools/perf/util/evlist.c > > @@ -1181,6 +1181,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e > > if (evsel->filter == NULL) > > continue; > > > > + /* > > + * filters only work for tracepoint event, which doesn't have cpu limit. > > + * So evlist and evsel should always be same. > > + */ > > err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); > > if (err) { > > *err_evsel = evsel; > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 15:27 ` Arnaldo Carvalho de Melo @ 2015-09-03 16:23 ` Adrian Hunter 2015-09-03 16:41 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 22+ messages in thread From: Adrian Hunter @ 2015-09-03 16:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: kan.liang, mingo, jolsa, linux-kernel On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: > Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: >> On 01/09/15 11:31, tip-bot for Kan Liang wrote: >>> Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e >>> Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e >>> Author: Kan Liang <kan.liang@intel.com> >>> AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 >>> Committer: Arnaldo Carvalho de Melo <acme@redhat.com> >>> CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 >>> >>> perf evlist: Open event on evsel cpus and threads >>> >>> An evsel may have different cpus and threads than the evlist it is in. >>> >>> Use it's own cpus and threads, when opening the evsel in 'perf record'. >>> >>> Signed-off-by: Kan Liang <kan.liang@intel.com> >>> Cc: Jiri Olsa <jolsa@kernel.org> >>> Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com >>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> >> >> Just noticed this breaks Intel PT. Will have to investigate further. > > What kind of breakage? Can't open the sched_switch event > > It all should be equivalent to before, its just that it uses > evsel->{threads,cpus} while before it was using evlist->{threads,cpus}, > but that should point to the same thing if that > perf_evlist__propagate_maps() method was called, so I assume this is > some segfault? I think maybe it doesn't consider evsels added later > > Something we could catch in a 'test' entry? Even if that required Intel > PT hardware that would be something important to have, all this stuff is > growing in complexity, we need those tests... There is "Test tracking with sched_switch" but you need to expose it to the same issue i.e. diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index d51a520..6aeffdd 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -1433,7 +1433,7 @@ int perf_evlist__open(struct perf_evlist *evlist) perf_evlist__update_id_pos(evlist); evlist__for_each(evlist, evsel) { - err = perf_evsel__open(evsel, evlist->cpus, evlist->threads); + err = perf_evsel__open(evsel, evsel->cpus, evsel->threads); if (err < 0) goto out_err; } > > - Arnaldo > >>> --- >>> tools/perf/builtin-record.c | 2 +- >>> tools/perf/util/evlist.c | 4 ++++ >>> 2 files changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>> index a660022..1d14f38 100644 >>> --- a/tools/perf/builtin-record.c >>> +++ b/tools/perf/builtin-record.c >>> @@ -279,7 +279,7 @@ static int record__open(struct record *rec) >>> >>> evlist__for_each(evlist, pos) { >>> try_again: >>> - if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { >>> + if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { >>> if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { >>> if (verbose) >>> ui__warning("%s\n", msg); >>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >>> index 8d00039..d51a520 100644 >>> --- a/tools/perf/util/evlist.c >>> +++ b/tools/perf/util/evlist.c >>> @@ -1181,6 +1181,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e >>> if (evsel->filter == NULL) >>> continue; >>> >>> + /* >>> + * filters only work for tracepoint event, which doesn't have cpu limit. >>> + * So evlist and evsel should always be same. >>> + */ >>> err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); >>> if (err) { >>> *err_evsel = evsel; >>> ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 16:23 ` Adrian Hunter @ 2015-09-03 16:41 ` Arnaldo Carvalho de Melo 2015-09-03 18:19 ` Jiri Olsa 0 siblings, 1 reply; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-03 16:41 UTC (permalink / raw) To: Adrian Hunter; +Cc: kan.liang, mingo, jolsa, linux-kernel Em Thu, Sep 03, 2015 at 07:23:43PM +0300, Adrian Hunter escreveu: > On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: > >Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: > >>On 01/09/15 11:31, tip-bot for Kan Liang wrote: > >>>Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e > >>>Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e > >>>Author: Kan Liang <kan.liang@intel.com> > >>>AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 > >>>Committer: Arnaldo Carvalho de Melo <acme@redhat.com> > >>>CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 > >>> > >>>perf evlist: Open event on evsel cpus and threads > >>> > >>>An evsel may have different cpus and threads than the evlist it is in. > >>> > >>>Use it's own cpus and threads, when opening the evsel in 'perf record'. > >>> > >>>Signed-off-by: Kan Liang <kan.liang@intel.com> > >>>Cc: Jiri Olsa <jolsa@kernel.org> > >>>Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com > >>>Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > >> > >>Just noticed this breaks Intel PT. Will have to investigate further. > > > >What kind of breakage? > > Can't open the sched_switch event > > > > >It all should be equivalent to before, its just that it uses > >evsel->{threads,cpus} while before it was using evlist->{threads,cpus}, > >but that should point to the same thing if that > >perf_evlist__propagate_maps() method was called, so I assume this is > >some segfault? > > I think maybe it doesn't consider evsels added later A-ha, so when using perf_evlist__add() we need to do this, i.e. if evsel->{threads||cpus} is not set, fill it in with the evlist-> member? > > > >Something we could catch in a 'test' entry? Even if that required Intel > >PT hardware that would be something important to have, all this stuff is > >growing in complexity, we need those tests... > > There is "Test tracking with sched_switch" but you need to expose it > to the same issue i.e. Sure, Kan and Jiri were talking about the need to go doing these changes, Jiri? Kan? - Arnaldo > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index d51a520..6aeffdd 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -1433,7 +1433,7 @@ int perf_evlist__open(struct perf_evlist *evlist) > perf_evlist__update_id_pos(evlist); > > evlist__for_each(evlist, evsel) { > - err = perf_evsel__open(evsel, evlist->cpus, evlist->threads); > + err = perf_evsel__open(evsel, evsel->cpus, evsel->threads); > if (err < 0) > goto out_err; > } > > > > > > >- Arnaldo > > > >>>--- > >>> tools/perf/builtin-record.c | 2 +- > >>> tools/perf/util/evlist.c | 4 ++++ > >>> 2 files changed, 5 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > >>>index a660022..1d14f38 100644 > >>>--- a/tools/perf/builtin-record.c > >>>+++ b/tools/perf/builtin-record.c > >>>@@ -279,7 +279,7 @@ static int record__open(struct record *rec) > >>> > >>> evlist__for_each(evlist, pos) { > >>> try_again: > >>>- if (perf_evsel__open(pos, evlist->cpus, evlist->threads) < 0) { > >>>+ if (perf_evsel__open(pos, pos->cpus, pos->threads) < 0) { > >>> if (perf_evsel__fallback(pos, errno, msg, sizeof(msg))) { > >>> if (verbose) > >>> ui__warning("%s\n", msg); > >>>diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > >>>index 8d00039..d51a520 100644 > >>>--- a/tools/perf/util/evlist.c > >>>+++ b/tools/perf/util/evlist.c > >>>@@ -1181,6 +1181,10 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e > >>> if (evsel->filter == NULL) > >>> continue; > >>> > >>>+ /* > >>>+ * filters only work for tracepoint event, which doesn't have cpu limit. > >>>+ * So evlist and evsel should always be same. > >>>+ */ > >>> err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter); > >>> if (err) { > >>> *err_evsel = evsel; > >>> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 16:41 ` Arnaldo Carvalho de Melo @ 2015-09-03 18:19 ` Jiri Olsa 2015-09-03 18:38 ` Adrian Hunter 2015-09-03 20:41 ` Arnaldo Carvalho de Melo 0 siblings, 2 replies; 22+ messages in thread From: Jiri Olsa @ 2015-09-03 18:19 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Adrian Hunter, kan.liang, mingo, jolsa, linux-kernel On Thu, Sep 03, 2015 at 01:41:09PM -0300, Arnaldo Carvalho de Melo wrote: > Em Thu, Sep 03, 2015 at 07:23:43PM +0300, Adrian Hunter escreveu: > > On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: > > >Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: > > >>On 01/09/15 11:31, tip-bot for Kan Liang wrote: > > >>>Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e > > >>>Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e > > >>>Author: Kan Liang <kan.liang@intel.com> > > >>>AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 > > >>>Committer: Arnaldo Carvalho de Melo <acme@redhat.com> > > >>>CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 > > >>> > > >>>perf evlist: Open event on evsel cpus and threads > > >>> > > >>>An evsel may have different cpus and threads than the evlist it is in. > > >>> > > >>>Use it's own cpus and threads, when opening the evsel in 'perf record'. > > >>> > > >>>Signed-off-by: Kan Liang <kan.liang@intel.com> > > >>>Cc: Jiri Olsa <jolsa@kernel.org> > > >>>Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com > > >>>Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > >> > > >>Just noticed this breaks Intel PT. Will have to investigate further. > > > > > >What kind of breakage? > > > > Can't open the sched_switch event > > > > > > > >It all should be equivalent to before, its just that it uses > > >evsel->{threads,cpus} while before it was using evlist->{threads,cpus}, > > >but that should point to the same thing if that > > >perf_evlist__propagate_maps() method was called, so I assume this is > > >some segfault? > > > > I think maybe it doesn't consider evsels added later > > A-ha, so when using perf_evlist__add() we need to do this, i.e. if > evsel->{threads||cpus} is not set, fill it in with the evlist-> member? > > > > > > >Something we could catch in a 'test' entry? Even if that required Intel > > >PT hardware that would be something important to have, all this stuff is > > >growing in complexity, we need those tests... > > > > There is "Test tracking with sched_switch" but you need to expose it > > to the same issue i.e. > > Sure, Kan and Jiri were talking about the need to go doing these > changes, Jiri? Kan? > perf_evlist__propagate_maps is called from perf_evlist__create_maps, so if evsel is added later it will not be affected, perhaps we need something like below jirka --- diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 8d00039d6a20..dfdaf1aafd80 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -133,6 +133,9 @@ void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) if (!evlist->nr_entries++) perf_evlist__set_id_pos(evlist); + + entry->cpus = cpu_map__get(evlist->cpus); + entry->threads = thread_map__get(evlist->threads); } void perf_evlist__splice_list_tail(struct perf_evlist *evlist, ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 18:19 ` Jiri Olsa @ 2015-09-03 18:38 ` Adrian Hunter 2015-09-03 19:04 ` Jiri Olsa 2015-09-03 20:41 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 22+ messages in thread From: Adrian Hunter @ 2015-09-03 18:38 UTC (permalink / raw) To: Jiri Olsa, Arnaldo Carvalho de Melo; +Cc: kan.liang, mingo, jolsa, linux-kernel On 3/09/2015 9:19 p.m., Jiri Olsa wrote: > On Thu, Sep 03, 2015 at 01:41:09PM -0300, Arnaldo Carvalho de Melo wrote: >> Em Thu, Sep 03, 2015 at 07:23:43PM +0300, Adrian Hunter escreveu: >>> On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: >>>> Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: >>>>> On 01/09/15 11:31, tip-bot for Kan Liang wrote: >>>>>> Commit-ID: d988d5ee647861706bc7a391ddbc29429b50f00e >>>>>> Gitweb: http://git.kernel.org/tip/d988d5ee647861706bc7a391ddbc29429b50f00e >>>>>> Author: Kan Liang <kan.liang@intel.com> >>>>>> AuthorDate: Fri, 21 Aug 2015 02:23:14 -0400 >>>>>> Committer: Arnaldo Carvalho de Melo <acme@redhat.com> >>>>>> CommitDate: Mon, 31 Aug 2015 17:28:01 -0300 >>>>>> >>>>>> perf evlist: Open event on evsel cpus and threads >>>>>> >>>>>> An evsel may have different cpus and threads than the evlist it is in. >>>>>> >>>>>> Use it's own cpus and threads, when opening the evsel in 'perf record'. >>>>>> >>>>>> Signed-off-by: Kan Liang <kan.liang@intel.com> >>>>>> Cc: Jiri Olsa <jolsa@kernel.org> >>>>>> Link: http://lkml.kernel.org/r/1440138194-17001-1-git-send-email-kan.liang@intel.com >>>>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> >>>>> >>>>> Just noticed this breaks Intel PT. Will have to investigate further. >>>> >>>> What kind of breakage? >>> >>> Can't open the sched_switch event >>> >>>> >>>> It all should be equivalent to before, its just that it uses >>>> evsel->{threads,cpus} while before it was using evlist->{threads,cpus}, >>>> but that should point to the same thing if that >>>> perf_evlist__propagate_maps() method was called, so I assume this is >>>> some segfault? >>> >>> I think maybe it doesn't consider evsels added later >> >> A-ha, so when using perf_evlist__add() we need to do this, i.e. if >> evsel->{threads||cpus} is not set, fill it in with the evlist-> member? >> >>>> >>>> Something we could catch in a 'test' entry? Even if that required Intel >>>> PT hardware that would be something important to have, all this stuff is >>>> growing in complexity, we need those tests... >>> >>> There is "Test tracking with sched_switch" but you need to expose it >>> to the same issue i.e. >> >> Sure, Kan and Jiri were talking about the need to go doing these >> changes, Jiri? Kan? >> > > perf_evlist__propagate_maps is called from perf_evlist__create_maps, > so if evsel is added later it will not be affected, perhaps we need > something like below Yes but it would be nice to have a single function to do it that knows the rules for when the evsel->cpus should be retained. Say: int __perf_evlist__propagate_maps(struct perf_evlist *evlist, struct perf_evsel *evsel) then call it from perf_evlist__propagate_maps(), perf_evlist__add() and perf_evlist__splice_list_tail(). Probably need to add a member to perf_evlist to know when the evlist->cpus should replace the evsel->cpus > > jirka > > > --- > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 8d00039d6a20..dfdaf1aafd80 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -133,6 +133,9 @@ void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) > > if (!evlist->nr_entries++) > perf_evlist__set_id_pos(evlist); > + > + entry->cpus = cpu_map__get(evlist->cpus); > + entry->threads = thread_map__get(evlist->threads); > } > > void perf_evlist__splice_list_tail(struct perf_evlist *evlist, > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 18:38 ` Adrian Hunter @ 2015-09-03 19:04 ` Jiri Olsa 0 siblings, 0 replies; 22+ messages in thread From: Jiri Olsa @ 2015-09-03 19:04 UTC (permalink / raw) To: Adrian Hunter Cc: Arnaldo Carvalho de Melo, kan.liang, mingo, jolsa, linux-kernel On Thu, Sep 03, 2015 at 09:38:16PM +0300, Adrian Hunter wrote: SNIP > > > >perf_evlist__propagate_maps is called from perf_evlist__create_maps, > >so if evsel is added later it will not be affected, perhaps we need > >something like below > > Yes but it would be nice to have a single function to do it that knows > the rules for when the evsel->cpus should be retained. Say: > int __perf_evlist__propagate_maps(struct perf_evlist *evlist, struct perf_evsel *evsel) > then call it from perf_evlist__propagate_maps(), perf_evlist__add() and > perf_evlist__splice_list_tail(). Probably need to add a member to perf_evlist > to know when the evlist->cpus should replace the evsel->cpus ook, jirka ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 18:19 ` Jiri Olsa 2015-09-03 18:38 ` Adrian Hunter @ 2015-09-03 20:41 ` Arnaldo Carvalho de Melo 2015-09-04 7:05 ` Jiri Olsa 1 sibling, 1 reply; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-03 20:41 UTC (permalink / raw) To: Jiri Olsa; +Cc: Adrian Hunter, kan.liang, mingo, jolsa, linux-kernel Em Thu, Sep 03, 2015 at 08:19:39PM +0200, Jiri Olsa escreveu: > On Thu, Sep 03, 2015 at 01:41:09PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Thu, Sep 03, 2015 at 07:23:43PM +0300, Adrian Hunter escreveu: > > > On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: > > > >Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: > > > >>On 01/09/15 11:31, tip-bot for Kan Liang wrote: > > > >Something we could catch in a 'test' entry? Even if that required Intel > > > >PT hardware that would be something important to have, all this stuff is > > > >growing in complexity, we need those tests... > > > There is "Test tracking with sched_switch" but you need to expose it > > > to the same issue i.e. > > Sure, Kan and Jiri were talking about the need to go doing these > > changes, Jiri? Kan? > perf_evlist__propagate_maps is called from perf_evlist__create_maps, > so if evsel is added later it will not be affected, perhaps we need > something like below: > +++ b/tools/perf/util/evlist.c > @@ -133,6 +133,9 @@ void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) > > if (!evlist->nr_entries++) > perf_evlist__set_id_pos(evlist); > + > + entry->cpus = cpu_map__get(evlist->cpus); > + entry->threads = thread_map__get(evlist->threads); You can't simply do that, we need to do it only if those fields are not already set. - Arnaldo ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-03 20:41 ` Arnaldo Carvalho de Melo @ 2015-09-04 7:05 ` Jiri Olsa 2015-09-04 7:09 ` Adrian Hunter 0 siblings, 1 reply; 22+ messages in thread From: Jiri Olsa @ 2015-09-04 7:05 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Adrian Hunter, kan.liang, mingo, jolsa, linux-kernel On Thu, Sep 03, 2015 at 05:41:06PM -0300, Arnaldo Carvalho de Melo wrote: > Em Thu, Sep 03, 2015 at 08:19:39PM +0200, Jiri Olsa escreveu: > > On Thu, Sep 03, 2015 at 01:41:09PM -0300, Arnaldo Carvalho de Melo wrote: > > > Em Thu, Sep 03, 2015 at 07:23:43PM +0300, Adrian Hunter escreveu: > > > > On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: > > > > >Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: > > > > >>On 01/09/15 11:31, tip-bot for Kan Liang wrote: > > > > >Something we could catch in a 'test' entry? Even if that required Intel > > > > >PT hardware that would be something important to have, all this stuff is > > > > >growing in complexity, we need those tests... > > > > > There is "Test tracking with sched_switch" but you need to expose it > > > > to the same issue i.e. > > > > Sure, Kan and Jiri were talking about the need to go doing these > > > changes, Jiri? Kan? > > > perf_evlist__propagate_maps is called from perf_evlist__create_maps, > > so if evsel is added later it will not be affected, perhaps we need > > something like below: > > > +++ b/tools/perf/util/evlist.c > > @@ -133,6 +133,9 @@ void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) > > > > if (!evlist->nr_entries++) > > perf_evlist__set_id_pos(evlist); > > + > > + entry->cpus = cpu_map__get(evlist->cpus); > > + entry->threads = thread_map__get(evlist->threads); > > You can't simply do that, we need to do it only if those fields are not > already set. argh, right.. forgot about cpus setup.. anyway, Adrian already wanted to do single function for this and the propagate function jirka ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads 2015-09-04 7:05 ` Jiri Olsa @ 2015-09-04 7:09 ` Adrian Hunter 2015-09-04 12:15 ` [PATCH] perf tools: Fix gaps propagating maps Adrian Hunter 0 siblings, 1 reply; 22+ messages in thread From: Adrian Hunter @ 2015-09-04 7:09 UTC (permalink / raw) To: Jiri Olsa, Arnaldo Carvalho de Melo; +Cc: kan.liang, mingo, jolsa, linux-kernel On 04/09/15 10:05, Jiri Olsa wrote: > On Thu, Sep 03, 2015 at 05:41:06PM -0300, Arnaldo Carvalho de Melo wrote: >> Em Thu, Sep 03, 2015 at 08:19:39PM +0200, Jiri Olsa escreveu: >>> On Thu, Sep 03, 2015 at 01:41:09PM -0300, Arnaldo Carvalho de Melo wrote: >>>> Em Thu, Sep 03, 2015 at 07:23:43PM +0300, Adrian Hunter escreveu: >>>>> On 3/09/2015 6:27 p.m., Arnaldo Carvalho de Melo wrote: >>>>>> Em Thu, Sep 03, 2015 at 04:34:24PM +0300, Adrian Hunter escreveu: >>>>>>> On 01/09/15 11:31, tip-bot for Kan Liang wrote: >>>>>> Something we could catch in a 'test' entry? Even if that required Intel >>>>>> PT hardware that would be something important to have, all this stuff is >>>>>> growing in complexity, we need those tests... >> >>>>> There is "Test tracking with sched_switch" but you need to expose it >>>>> to the same issue i.e. >> >>>> Sure, Kan and Jiri were talking about the need to go doing these >>>> changes, Jiri? Kan? >> >>> perf_evlist__propagate_maps is called from perf_evlist__create_maps, >>> so if evsel is added later it will not be affected, perhaps we need >>> something like below: >> >>> +++ b/tools/perf/util/evlist.c >>> @@ -133,6 +133,9 @@ void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) >>> >>> if (!evlist->nr_entries++) >>> perf_evlist__set_id_pos(evlist); >>> + >>> + entry->cpus = cpu_map__get(evlist->cpus); >>> + entry->threads = thread_map__get(evlist->threads); >> >> You can't simply do that, we need to do it only if those fields are not >> already set. > > argh, right.. forgot about cpus setup.. anyway, Adrian already wanted > to do single function for this and the propagate function yeah, I started working on it this morning. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] perf tools: Fix gaps propagating maps 2015-09-04 7:09 ` Adrian Hunter @ 2015-09-04 12:15 ` Adrian Hunter 2015-09-04 13:28 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 22+ messages in thread From: Adrian Hunter @ 2015-09-04 12:15 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Jiri Olsa, jolsa, mingo, kan.liang, linux-kernel A perf_evsel is a selected event containing the perf_event_attr that is passed to perf_event_open(). A perf_evlist is a collection of perf_evsel's. A perf_evlist also has lists of cpus and threads (pids) on which to open the event. These lists are called 'maps' and this patch is about how those 'maps' are propagated from the perf_evlist to the perf_evsels. Originally perf_evsels did not have their own thread 'maps', and their cpu 'maps' were only used for checking. Then threads were added by: 578e91ec04d0 ("perf evlist: Propagate thread maps through the evlist") And then 'perf record' was changed to open events using the 'maps' from perf_evsel not perf_evlist anymore, by: d988d5ee6478 ("perf evlist: Open event on evsel cpus and threads") That exposed situations where the 'maps' were not getting propagated correctly, such as when perf_evsel are added to the perf_evlist later. This patch ensures 'maps' get propagated in that case, and also if 'maps' are subsequently changed or set to NULL by perf_evlist__set_maps() and perf_evlist__create_syswide_maps(). Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- tools/perf/util/evlist.c | 129 ++++++++++++++++++++++++++++++++--------------- tools/perf/util/evlist.h | 1 + 2 files changed, 88 insertions(+), 42 deletions(-) diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index d51a5200c8af..00128cf7655b 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -124,8 +124,49 @@ void perf_evlist__delete(struct perf_evlist *evlist) free(evlist); } +static void __perf_evlist__propagate_maps(struct perf_evlist *evlist, + struct perf_evsel *evsel, + bool propagate) +{ + if (propagate) { + /* + * We already have cpus for evsel (via PMU sysfs) so + * keep it, if there's no target cpu list defined. + */ + if ((!evsel->cpus || evlist->has_user_cpus) && + evsel->cpus != evlist->cpus) { + cpu_map__put(evsel->cpus); + evsel->cpus = cpu_map__get(evlist->cpus); + } + + if (!evsel->threads) + evsel->threads = thread_map__get(evlist->threads); + } else { + if (evsel->cpus == evlist->cpus) { + cpu_map__put(evsel->cpus); + evsel->cpus = NULL; + } + + if (evsel->threads == evlist->threads) { + thread_map__put(evsel->threads); + evsel->threads = NULL; + } + } +} + +static void perf_evlist__propagate_maps(struct perf_evlist *evlist, + bool propagate) +{ + struct perf_evsel *evsel; + + evlist__for_each(evlist, evsel) + __perf_evlist__propagate_maps(evlist, evsel, propagate); +} + void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) { + __perf_evlist__propagate_maps(evlist, entry, true); + entry->evlist = evlist; list_add_tail(&entry->node, &evlist->entries); entry->idx = evlist->nr_entries; @@ -140,6 +181,10 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist, int nr_entries) { bool set_id_pos = !evlist->nr_entries; + struct perf_evsel *evsel; + + __evlist__for_each(list, evsel) + __perf_evlist__propagate_maps(evlist, evsel, true); list_splice_tail(list, &evlist->entries); evlist->nr_entries += nr_entries; @@ -1103,34 +1148,11 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages, return perf_evlist__mmap_ex(evlist, pages, overwrite, 0, false); } -static int perf_evlist__propagate_maps(struct perf_evlist *evlist, - bool has_user_cpus) -{ - struct perf_evsel *evsel; - - evlist__for_each(evlist, evsel) { - /* - * We already have cpus for evsel (via PMU sysfs) so - * keep it, if there's no target cpu list defined. - */ - if (evsel->cpus && has_user_cpus) - cpu_map__put(evsel->cpus); - - if (!evsel->cpus || has_user_cpus) - evsel->cpus = cpu_map__get(evlist->cpus); - - evsel->threads = thread_map__get(evlist->threads); - - if ((evlist->cpus && !evsel->cpus) || - (evlist->threads && !evsel->threads)) - return -ENOMEM; - } - - return 0; -} - int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target) { + if (evlist->threads || evlist->cpus) + return -1; + evlist->threads = thread_map__new_str(target->pid, target->tid, target->uid); @@ -1145,7 +1167,11 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target) if (evlist->cpus == NULL) goto out_delete_threads; - return perf_evlist__propagate_maps(evlist, !!target->cpu_list); + evlist->has_user_cpus = !!target->cpu_list; + + perf_evlist__propagate_maps(evlist, true); + + return 0; out_delete_threads: thread_map__put(evlist->threads); @@ -1157,17 +1183,32 @@ int perf_evlist__set_maps(struct perf_evlist *evlist, struct cpu_map *cpus, struct thread_map *threads) { - if (evlist->cpus) - cpu_map__put(evlist->cpus); + /* + * First 'un-propagate' the current maps which allows the propagation to + * work correctly even when changing the maps or setting them to NULL. + */ + perf_evlist__propagate_maps(evlist, false); - evlist->cpus = cpus; + /* + * Allow for the possibility that one or another of the maps isn't being + * changed i.e. don't put it. Note we are assuming the maps that are + * being applied are brand new and evlist is taking ownership of the + * original reference count of 1. If that is not the case it is up to + * the caller to increase the reference count. + */ + if (cpus != evlist->cpus) { + cpu_map__put(evlist->cpus); + evlist->cpus = cpus; + } - if (evlist->threads) + if (threads != evlist->threads) { thread_map__put(evlist->threads); + evlist->threads = threads; + } - evlist->threads = threads; + perf_evlist__propagate_maps(evlist, true); - return perf_evlist__propagate_maps(evlist, false); + return 0; } int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel) @@ -1387,6 +1428,8 @@ void perf_evlist__close(struct perf_evlist *evlist) static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist) { + struct cpu_map *cpus; + struct thread_map *threads; int err = -ENOMEM; /* @@ -1398,20 +1441,22 @@ static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist) * error, and we may not want to do that fallback to a * default cpu identity map :-\ */ - evlist->cpus = cpu_map__new(NULL); - if (evlist->cpus == NULL) + cpus = cpu_map__new(NULL); + if (!cpus) goto out; - evlist->threads = thread_map__new_dummy(); - if (evlist->threads == NULL) - goto out_free_cpus; + threads = thread_map__new_dummy(); + if (!threads) + goto out_put; - err = 0; + err = perf_evlist__set_maps(evlist, cpus, threads); + if (err) + goto out_put; out: return err; -out_free_cpus: - cpu_map__put(evlist->cpus); - evlist->cpus = NULL; +out_put: + cpu_map__put(cpus); + thread_map__put(threads); goto out; } diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h index b39a6198f4ac..2dd5715dfef6 100644 --- a/tools/perf/util/evlist.h +++ b/tools/perf/util/evlist.h @@ -42,6 +42,7 @@ struct perf_evlist { int nr_mmaps; bool overwrite; bool enabled; + bool has_user_cpus; size_t mmap_len; int id_pos; int is_pos; -- 1.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] perf tools: Fix gaps propagating maps 2015-09-04 12:15 ` [PATCH] perf tools: Fix gaps propagating maps Adrian Hunter @ 2015-09-04 13:28 ` Arnaldo Carvalho de Melo 2015-09-04 13:42 ` Adrian Hunter 0 siblings, 1 reply; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-04 13:28 UTC (permalink / raw) To: Adrian Hunter; +Cc: Jiri Olsa, jolsa, mingo, kan.liang, linux-kernel Em Fri, Sep 04, 2015 at 03:15:54PM +0300, Adrian Hunter escreveu: > A perf_evsel is a selected event containing the perf_event_attr > that is passed to perf_event_open(). A perf_evlist is a collection > of perf_evsel's. A perf_evlist also has lists of cpus and threads > (pids) on which to open the event. These lists are called 'maps' > and this patch is about how those 'maps' are propagated from the >> perf_evlist to the perf_evsels. Can't this be broken up in multiple patches, for instance this: int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target) { + if (evlist->threads || evlist->cpus) + return -1; + Looks like a fix that could be separated. Also FOO__propagate(.., false) to do the opposite of propagate seems confusing, how about FOO__unpropagate() if that verb exists? :-) Also, when unpropagating, you do: if (evsel->cpus == evlist->cpus) { cpu_map__put(evsel->cpus); evsel->cpus = NULL; } What if the PMU code _set_ it to the same cpus as in evlist->cpus, but now we're unpropagating to set to another CPU, in this case we will be changing the PMU setting with a new one. I.e. when a PMU sets it it should be sticky, no? I.e. we would have to know, in the evsel, if evsel->cpus was set by the PMU or any other future entity expecting this behaviour, so that we don't touch it, i.e. testing (evsel->cpus != evlist->cpus) when unpropagating doesn't seem to cut, right? - Arnaldo > Originally perf_evsels did not have their own thread 'maps', and > their cpu 'maps' were only used for checking. Then threads were > added by: > > 578e91ec04d0 ("perf evlist: Propagate thread maps through the evlist") > > And then 'perf record' was changed to open events using the 'maps' > from perf_evsel not perf_evlist anymore, by: > > d988d5ee6478 ("perf evlist: Open event on evsel cpus and threads") > > That exposed situations where the 'maps' were not getting propagated > correctly, such as when perf_evsel are added to the perf_evlist later. > This patch ensures 'maps' get propagated in that case, and also if 'maps' > are subsequently changed or set to NULL by perf_evlist__set_maps() > and perf_evlist__create_syswide_maps(). > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > tools/perf/util/evlist.c | 129 ++++++++++++++++++++++++++++++++--------------- > tools/perf/util/evlist.h | 1 + > 2 files changed, 88 insertions(+), 42 deletions(-) > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index d51a5200c8af..00128cf7655b 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -124,8 +124,49 @@ void perf_evlist__delete(struct perf_evlist *evlist) > free(evlist); > } > > +static void __perf_evlist__propagate_maps(struct perf_evlist *evlist, > + struct perf_evsel *evsel, > + bool propagate) > +{ > + if (propagate) { > + /* > + * We already have cpus for evsel (via PMU sysfs) so > + * keep it, if there's no target cpu list defined. > + */ > + if ((!evsel->cpus || evlist->has_user_cpus) && > + evsel->cpus != evlist->cpus) { > + cpu_map__put(evsel->cpus); > + evsel->cpus = cpu_map__get(evlist->cpus); > + } > + > + if (!evsel->threads) > + evsel->threads = thread_map__get(evlist->threads); > + } else { > + if (evsel->cpus == evlist->cpus) { > + cpu_map__put(evsel->cpus); > + evsel->cpus = NULL; > + } > + > + if (evsel->threads == evlist->threads) { > + thread_map__put(evsel->threads); > + evsel->threads = NULL; > + } > + } > +} > + > +static void perf_evlist__propagate_maps(struct perf_evlist *evlist, > + bool propagate) > +{ > + struct perf_evsel *evsel; > + > + evlist__for_each(evlist, evsel) > + __perf_evlist__propagate_maps(evlist, evsel, propagate); > +} > + > void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry) > { > + __perf_evlist__propagate_maps(evlist, entry, true); > + > entry->evlist = evlist; > list_add_tail(&entry->node, &evlist->entries); > entry->idx = evlist->nr_entries; > @@ -140,6 +181,10 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist, > int nr_entries) > { > bool set_id_pos = !evlist->nr_entries; > + struct perf_evsel *evsel; > + > + __evlist__for_each(list, evsel) > + __perf_evlist__propagate_maps(evlist, evsel, true); > > list_splice_tail(list, &evlist->entries); > evlist->nr_entries += nr_entries; > @@ -1103,34 +1148,11 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages, > return perf_evlist__mmap_ex(evlist, pages, overwrite, 0, false); > } > > -static int perf_evlist__propagate_maps(struct perf_evlist *evlist, > - bool has_user_cpus) > -{ > - struct perf_evsel *evsel; > - > - evlist__for_each(evlist, evsel) { > - /* > - * We already have cpus for evsel (via PMU sysfs) so > - * keep it, if there's no target cpu list defined. > - */ > - if (evsel->cpus && has_user_cpus) > - cpu_map__put(evsel->cpus); > - > - if (!evsel->cpus || has_user_cpus) > - evsel->cpus = cpu_map__get(evlist->cpus); > - > - evsel->threads = thread_map__get(evlist->threads); > - > - if ((evlist->cpus && !evsel->cpus) || > - (evlist->threads && !evsel->threads)) > - return -ENOMEM; > - } > - > - return 0; > -} > - > int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target) > { > + if (evlist->threads || evlist->cpus) > + return -1; > + > evlist->threads = thread_map__new_str(target->pid, target->tid, > target->uid); > > @@ -1145,7 +1167,11 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target) > if (evlist->cpus == NULL) > goto out_delete_threads; > > - return perf_evlist__propagate_maps(evlist, !!target->cpu_list); > + evlist->has_user_cpus = !!target->cpu_list; > + > + perf_evlist__propagate_maps(evlist, true); > + > + return 0; > > out_delete_threads: > thread_map__put(evlist->threads); > @@ -1157,17 +1183,32 @@ int perf_evlist__set_maps(struct perf_evlist *evlist, > struct cpu_map *cpus, > struct thread_map *threads) > { > - if (evlist->cpus) > - cpu_map__put(evlist->cpus); > + /* > + * First 'un-propagate' the current maps which allows the propagation to > + * work correctly even when changing the maps or setting them to NULL. > + */ > + perf_evlist__propagate_maps(evlist, false); > > - evlist->cpus = cpus; > + /* > + * Allow for the possibility that one or another of the maps isn't being > + * changed i.e. don't put it. Note we are assuming the maps that are > + * being applied are brand new and evlist is taking ownership of the > + * original reference count of 1. If that is not the case it is up to > + * the caller to increase the reference count. > + */ > + if (cpus != evlist->cpus) { > + cpu_map__put(evlist->cpus); > + evlist->cpus = cpus; > + } > > - if (evlist->threads) > + if (threads != evlist->threads) { > thread_map__put(evlist->threads); > + evlist->threads = threads; > + } > > - evlist->threads = threads; > + perf_evlist__propagate_maps(evlist, true); > > - return perf_evlist__propagate_maps(evlist, false); > + return 0; > } > > int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel) > @@ -1387,6 +1428,8 @@ void perf_evlist__close(struct perf_evlist *evlist) > > static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist) > { > + struct cpu_map *cpus; > + struct thread_map *threads; > int err = -ENOMEM; > > /* > @@ -1398,20 +1441,22 @@ static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist) > * error, and we may not want to do that fallback to a > * default cpu identity map :-\ > */ > - evlist->cpus = cpu_map__new(NULL); > - if (evlist->cpus == NULL) > + cpus = cpu_map__new(NULL); > + if (!cpus) > goto out; > > - evlist->threads = thread_map__new_dummy(); > - if (evlist->threads == NULL) > - goto out_free_cpus; > + threads = thread_map__new_dummy(); > + if (!threads) > + goto out_put; > > - err = 0; > + err = perf_evlist__set_maps(evlist, cpus, threads); > + if (err) > + goto out_put; > out: > return err; > -out_free_cpus: > - cpu_map__put(evlist->cpus); > - evlist->cpus = NULL; > +out_put: > + cpu_map__put(cpus); > + thread_map__put(threads); > goto out; > } > > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h > index b39a6198f4ac..2dd5715dfef6 100644 > --- a/tools/perf/util/evlist.h > +++ b/tools/perf/util/evlist.h > @@ -42,6 +42,7 @@ struct perf_evlist { > int nr_mmaps; > bool overwrite; > bool enabled; > + bool has_user_cpus; > size_t mmap_len; > int id_pos; > int is_pos; > -- > 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] perf tools: Fix gaps propagating maps 2015-09-04 13:28 ` Arnaldo Carvalho de Melo @ 2015-09-04 13:42 ` Adrian Hunter 2015-09-04 14:48 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 22+ messages in thread From: Adrian Hunter @ 2015-09-04 13:42 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Jiri Olsa, jolsa, mingo, kan.liang, linux-kernel On 04/09/15 16:28, Arnaldo Carvalho de Melo wrote: > Em Fri, Sep 04, 2015 at 03:15:54PM +0300, Adrian Hunter escreveu: >> A perf_evsel is a selected event containing the perf_event_attr >> that is passed to perf_event_open(). A perf_evlist is a collection >> of perf_evsel's. A perf_evlist also has lists of cpus and threads >> (pids) on which to open the event. These lists are called 'maps' >> and this patch is about how those 'maps' are propagated from the >>> perf_evlist to the perf_evsels. > > Can't this be broken up in multiple patches, for instance this: Ok, might not be until next week though. > > int perf_evlist__create_maps(struct perf_evlist *evlist, struct > target *target) > { > + if (evlist->threads || evlist->cpus) > + return -1; > + Or you could just drop that chunk. > > Looks like a fix that could be separated. Also FOO__propagate(.., false) > to do the opposite of propagate seems confusing, how about > FOO__unpropagate() if that verb exists? :-) Ok > > > Also, when unpropagating, you do: > > if (evsel->cpus == evlist->cpus) { > cpu_map__put(evsel->cpus); > evsel->cpus = NULL; > } > > What if the PMU code _set_ it to the same cpus as in evlist->cpus, but > now we're unpropagating to set to another CPU, in this case we will be > changing the PMU setting with a new one. I.e. when a PMU sets it it > should be sticky, no? We are comparing the pointer, so that won't happen unless the PMU actually does evsel->cpus = evlist->cpus which seems unlikely. > > I.e. we would have to know, in the evsel, if evsel->cpus was set by the > PMU or any other future entity expecting this behaviour, so that we > don't touch it, i.e. testing (evsel->cpus != evlist->cpus) when > unpropagating doesn't seem to cut, right? I think the pointer comparison covers that. i.e. the pointers won't be the same even if the cpus are. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] perf tools: Fix gaps propagating maps 2015-09-04 13:42 ` Adrian Hunter @ 2015-09-04 14:48 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 22+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-04 14:48 UTC (permalink / raw) To: Adrian Hunter; +Cc: Jiri Olsa, jolsa, mingo, kan.liang, linux-kernel, acme Em Fri, Sep 04, 2015 at 04:42:30PM +0300, Adrian Hunter escreveu: > On 04/09/15 16:28, Arnaldo Carvalho de Melo wrote: > > Em Fri, Sep 04, 2015 at 03:15:54PM +0300, Adrian Hunter escreveu: > >> A perf_evsel is a selected event containing the perf_event_attr > >> that is passed to perf_event_open(). A perf_evlist is a collection > >> of perf_evsel's. A perf_evlist also has lists of cpus and threads > >> (pids) on which to open the event. These lists are called 'maps' > >> and this patch is about how those 'maps' are propagated from the > >>> perf_evlist to the perf_evsels. > > > > Can't this be broken up in multiple patches, for instance this: > > Ok, might not be until next week though. Take your time, it seems that so far the only problem is with Intel PT, with adding that sched_switch after the propagation was done, no? And with kernel with PERF_RECORD_SWITCH, that I am pushing that other patch from you, to the support using it replacing sched:sched_Switch in the tooling side reduces the impact of this problem, right? > > int perf_evlist__create_maps(struct perf_evlist *evlist, struct > > target *target) > > { > > + if (evlist->threads || evlist->cpus) > > + return -1; > > + > Or you could just drop that chunk. > > Looks like a fix that could be separated. Also FOO__propagate(.., false) > > to do the opposite of propagate seems confusing, how about > > FOO__unpropagate() if that verb exists? :-) > Ok > > Also, when unpropagating, you do: > > > > if (evsel->cpus == evlist->cpus) { > > cpu_map__put(evsel->cpus); > > evsel->cpus = NULL; > > } > > > > What if the PMU code _set_ it to the same cpus as in evlist->cpus, but > > now we're unpropagating to set to another CPU, in this case we will be > > changing the PMU setting with a new one. I.e. when a PMU sets it it > > should be sticky, no? > > We are comparing the pointer, so that won't happen unless the PMU actually > does evsel->cpus = evlist->cpus which seems unlikely. > > I.e. we would have to know, in the evsel, if evsel->cpus was set by the > > PMU or any other future entity expecting this behaviour, so that we > > don't touch it, i.e. testing (evsel->cpus != evlist->cpus) when > > unpropagating doesn't seem to cut, right? > > I think the pointer comparison covers that. i.e. the pointers won't be the > same even if the cpus are. It makes it more unlikely, yes. I think that to be safe we should have a evsel->sticky_{cpu,threads}, well, at least the evsel->sticky_cpus seems needed due to the PMU usecase. - Arnaldo ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2015-09-04 14:48 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-21 6:23 [PATCH 1/1] perf,tools: open event on evsel cpus and threads kan.liang 2015-08-21 13:54 ` Jiri Olsa 2015-08-31 20:31 ` Arnaldo Carvalho de Melo 2015-08-31 21:06 ` Liang, Kan 2015-08-31 21:14 ` Arnaldo Carvalho de Melo 2015-08-31 21:28 ` Liang, Kan 2015-08-31 21:34 ` Arnaldo Carvalho de Melo 2015-09-01 8:31 ` [tip:perf/urgent] perf evlist: Open " tip-bot for Kan Liang 2015-09-03 13:34 ` Adrian Hunter 2015-09-03 15:27 ` Arnaldo Carvalho de Melo 2015-09-03 16:23 ` Adrian Hunter 2015-09-03 16:41 ` Arnaldo Carvalho de Melo 2015-09-03 18:19 ` Jiri Olsa 2015-09-03 18:38 ` Adrian Hunter 2015-09-03 19:04 ` Jiri Olsa 2015-09-03 20:41 ` Arnaldo Carvalho de Melo 2015-09-04 7:05 ` Jiri Olsa 2015-09-04 7:09 ` Adrian Hunter 2015-09-04 12:15 ` [PATCH] perf tools: Fix gaps propagating maps Adrian Hunter 2015-09-04 13:28 ` Arnaldo Carvalho de Melo 2015-09-04 13:42 ` Adrian Hunter 2015-09-04 14:48 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.