Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1210303 > unrolled thread

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

Started byJiri Olsa <jolsa@redhat.com>
First post2015-08-20 11:00 +0200
Last post2015-08-21 15:30 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 01/10] perf,tools: open event on evsel cpus and  threads Jiri Olsa <jolsa@redhat.com> - 2015-08-20 11:00 +0200
    RE: [PATCH RFC 01/10] perf,tools: open event on evsel cpus and  threads "Liang, Kan" <kan.liang@intel.com> - 2015-08-20 19:30 +0200
      Re: [PATCH RFC 01/10] perf,tools: open event on evsel cpus and  threads Jiri Olsa <jolsa@redhat.com> - 2015-08-21 09:50 +0200
        RE: [PATCH RFC 01/10] perf,tools: open event on evsel cpus and  threads "Liang, Kan" <kan.liang@intel.com> - 2015-08-21 15:30 +0200

#1210303 — Re: [PATCH RFC 01/10] perf,tools: open event on evsel cpus and threads

FromJiri Olsa <jolsa@redhat.com>
Date2015-08-20 11:00 +0200
SubjectRe: [PATCH RFC 01/10] perf,tools: open event on evsel cpus and threads
Message-ID<pZtRv-8m5-3@gated-at.bofh.it>
On Tue, Aug 18, 2015 at 05:25:37AM -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.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  tools/perf/builtin-record.c | 2 +-
>  1 file changed, 1 insertion(+), 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);
> -- 
> 1.8.3.1
> 

dont we need then handle filters the same way?
like in attached change? totally untested..

jirka


---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 7aa039bd379a..f5cdf678d504 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -244,12 +244,9 @@ static void handle_initial_delay(void)
 	struct perf_evsel *counter;
 
 	if (initial_delay) {
-		const int ncpus = cpu_map__nr(evsel_list->cpus),
-			nthreads = thread_map__nr(evsel_list->threads);
-
 		usleep(initial_delay * 1000);
 		evlist__for_each(evsel_list, counter)
-			perf_evsel__enable(counter, ncpus, nthreads);
+			perf_evsel__enable(counter);
 	}
 }
 
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 373f65b02545..0d8428458ae1 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1172,14 +1172,12 @@ int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **e
 {
 	struct perf_evsel *evsel;
 	int err = 0;
-	const int ncpus = cpu_map__nr(evlist->cpus),
-		  nthreads = thread_map__nr(evlist->threads);
 
 	evlist__for_each(evlist, evsel) {
 		if (evsel->filter == NULL)
 			continue;
 
-		err = perf_evsel__apply_filter(evsel, ncpus, nthreads, evsel->filter);
+		err = perf_evsel__apply_filter(evsel, evsel->filter);
 		if (err) {
 			*err_evsel = evsel;
 			break;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index b096ef7a240c..e1e70c55c68d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -898,9 +898,10 @@ static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthread
 	return evsel->fd != NULL ? 0 : -ENOMEM;
 }
 
-static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ncpus, int nthreads,
-			  int ioc,  void *arg)
+static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ioc,  void *arg)
 {
+	int ncpus = cpu_map__nr(evsel->cpus),
+	    nthreads = thread_map__nr(evsel->threads);
 	int cpu, thread;
 
 	if (evsel->system_wide)
@@ -919,11 +920,9 @@ static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ncpus, int nthrea
 	return 0;
 }
 
-int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
-			     const char *filter)
+int perf_evsel__apply_filter(struct perf_evsel *evsel, const char *filter)
 {
-	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
-				     PERF_EVENT_IOC_SET_FILTER,
+	return perf_evsel__run_ioctl(evsel, PERF_EVENT_IOC_SET_FILTER,
 				     (void *)filter);
 }
 
@@ -957,11 +956,9 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 	return -1;
 }
 
-int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads)
+int perf_evsel__enable(struct perf_evsel *evsel)
 {
-	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
-				     PERF_EVENT_IOC_ENABLE,
-				     0);
+	return perf_evsel__run_ioctl(evsel, PERF_EVENT_IOC_ENABLE, 0);
 }
 
 int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 93ac6b128149..bb5b9c1d482b 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -214,9 +214,8 @@ void perf_evsel__set_sample_id(struct perf_evsel *evsel,
 int perf_evsel__set_filter(struct perf_evsel *evsel, const char *filter);
 int perf_evsel__append_filter(struct perf_evsel *evsel,
 			      const char *op, const char *filter);
-int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
-			     const char *filter);
-int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads);
+int perf_evsel__apply_filter(struct perf_evsel *evsel, const char *filter);
+int perf_evsel__enable(struct perf_evsel *evsel);
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
 			     struct cpu_map *cpus);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1210641

From"Liang, Kan" <kan.liang@intel.com>
Date2015-08-20 19:30 +0200
Message-ID<pZBP5-37g-13@gated-at.bofh.it>
In reply to#1210303
> 
> On Tue, Aug 18, 2015 at 05:25:37AM -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.
> >
> > Signed-off-by: Kan Liang <kan.liang@intel.com>
> > ---
> >  tools/perf/builtin-record.c | 2 +-
> >  1 file changed, 1 insertion(+), 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);
> > --
> > 1.8.3.1
> >
> 
> dont we need then handle filters the same way?
> like in attached change? totally untested..

Filters look only work for tracepoint event, which doesn't have cpu limit.
So evlist and evsel should always be same.
I think we don't need to change it.

> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 7aa039bd379a..f5cdf678d504 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -244,12 +244,9 @@ static void handle_initial_delay(void)
>  	struct perf_evsel *counter;
> 
>  	if (initial_delay) {
> -		const int ncpus = cpu_map__nr(evsel_list->cpus),
> -			nthreads = thread_map__nr(evsel_list->threads);
> -
>  		usleep(initial_delay * 1000);
>  		evlist__for_each(evsel_list, counter)
> -			perf_evsel__enable(counter, ncpus, nthreads);
> +			perf_evsel__enable(counter);
>  	}
>  }
>

Agree, we need to use evsel's cpu and threads here.
What about the code as below? It should be simpler.
+			perf_evsel__enable(counter, cpu_map__nr(counter->cpus), thread_map__nr(counter->threads));
 
 
Thanks,
Kan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1210972

FromJiri Olsa <jolsa@redhat.com>
Date2015-08-21 09:50 +0200
Message-ID<pZPfj-5Az-5@gated-at.bofh.it>
In reply to#1210641
On Thu, Aug 20, 2015 at 05:24:32PM +0000, Liang, Kan wrote:
> > 
> > On Tue, Aug 18, 2015 at 05:25:37AM -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.
> > >
> > > Signed-off-by: Kan Liang <kan.liang@intel.com>
> > > ---
> > >  tools/perf/builtin-record.c | 2 +-
> > >  1 file changed, 1 insertion(+), 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);
> > > --
> > > 1.8.3.1
> > >
> > 
> > dont we need then handle filters the same way?
> > like in attached change? totally untested..
> 
> Filters look only work for tracepoint event, which doesn't have cpu limit.
> So evlist and evsel should always be same.
> I think we don't need to change it.

right.. at least please make a comment about that

> 
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 7aa039bd379a..f5cdf678d504 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -244,12 +244,9 @@ static void handle_initial_delay(void)
> >  	struct perf_evsel *counter;
> > 
> >  	if (initial_delay) {
> > -		const int ncpus = cpu_map__nr(evsel_list->cpus),
> > -			nthreads = thread_map__nr(evsel_list->threads);
> > -
> >  		usleep(initial_delay * 1000);
> >  		evlist__for_each(evsel_list, counter)
> > -			perf_evsel__enable(counter, ncpus, nthreads);
> > +			perf_evsel__enable(counter);
> >  	}
> >  }
> >
> 
> Agree, we need to use evsel's cpu and threads here.
> What about the code as below? It should be simpler.
> +			perf_evsel__enable(counter, cpu_map__nr(counter->cpus), thread_map__nr(counter->threads));
>  

ok, maybe I'll submit that patch as a cleanup,
it seems more sane to use evsel cpus and threads
now that we always have it there

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1211159

From"Liang, Kan" <kan.liang@intel.com>
Date2015-08-21 15:30 +0200
Message-ID<pZUym-4R5-7@gated-at.bofh.it>
In reply to#1210972
> 
> On Thu, Aug 20, 2015 at 05:24:32PM +0000, Liang, Kan wrote:
> > >
> > > On Tue, Aug 18, 2015 at 05:25:37AM -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.
> > > >
> > > > Signed-off-by: Kan Liang <kan.liang@intel.com>
> > > > ---
> > > >  tools/perf/builtin-record.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 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);
> > > > --
> > > > 1.8.3.1
> > > >
> > >
> > > dont we need then handle filters the same way?
> > > like in attached change? totally untested..
> >
> > Filters look only work for tracepoint event, which doesn't have cpu limit.
> > So evlist and evsel should always be same.
> > I think we don't need to change it.
> 
> right.. at least please make a comment about that
>
OK. I will do that.

> >
> > >
> > > jirka
> > >
> > >
> > > ---
> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > index 7aa039bd379a..f5cdf678d504 100644
> > > --- a/tools/perf/builtin-stat.c
> > > +++ b/tools/perf/builtin-stat.c
> > > @@ -244,12 +244,9 @@ static void handle_initial_delay(void)
> > >  	struct perf_evsel *counter;
> > >
> > >  	if (initial_delay) {
> > > -		const int ncpus = cpu_map__nr(evsel_list->cpus),
> > > -			nthreads = thread_map__nr(evsel_list->threads);
> > > -
> > >  		usleep(initial_delay * 1000);
> > >  		evlist__for_each(evsel_list, counter)
> > > -			perf_evsel__enable(counter, ncpus, nthreads);
> > > +			perf_evsel__enable(counter);
> > >  	}
> > >  }
> > >
> >
> > Agree, we need to use evsel's cpu and threads here.
> > What about the code as below? It should be simpler.
> > +			perf_evsel__enable(counter,
> cpu_map__nr(counter->cpus),
> > +thread_map__nr(counter->threads));
> >
> 
> ok, maybe I'll submit that patch as a cleanup, it seems more sane to use
> evsel cpus and threads now that we always have it there
> 

I guess I will send out the builtin-record change and comments as a patch.
So you can either merge the cleanup code to that patch, or send out the
cleanup patch separately.
Either is fine for me.

Thanks,
Kan


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web