[PATCH 1/1] perf,tools: open event on evsel cpus and threads

[PATCH 1/1] perf,tools: open event on evsel cpus and threads

[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

Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads

[Jiri Olsa]

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
> 

Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads

[Arnaldo Carvalho de Melo]

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
> > 

RE: [PATCH 1/1] perf,tools: open event on evsel cpus and threads

[Liang, Kan]

> 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
> > >

Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads

[Arnaldo Carvalho de Melo]

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
> > > >

RE: [PATCH 1/1] perf,tools: open event on evsel cpus and threads

[Liang, Kan]

> 
> 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
> > > > >

Re: [PATCH 1/1] perf,tools: open event on evsel cpus and threads

[Arnaldo Carvalho de Melo]

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
> > > > > >

[tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[tip-bot for Kan Liang]

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;

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Adrian Hunter]

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;
> 

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Arnaldo Carvalho de Melo]

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;
> > 

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Adrian Hunter]

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;
>>>

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Arnaldo Carvalho de Melo]

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;
> >>>

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Jiri Olsa]

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,

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Adrian Hunter]

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,
>

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Jiri Olsa]

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

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Arnaldo Carvalho de Melo]

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

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Jiri Olsa]

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

Re: [tip:perf/urgent] perf evlist: Open event on evsel cpus and threads

[Adrian Hunter]

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.

[PATCH] perf tools: Fix gaps propagating maps

[Adrian Hunter]

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

Re: [PATCH] perf tools: Fix gaps propagating maps

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf tools: Fix gaps propagating maps

[Adrian Hunter]

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.

Re: [PATCH] perf tools: Fix gaps propagating maps

[Arnaldo Carvalho de Melo]

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