linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-16 15:43 [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread Jin Yao
@ 2018-01-16 12:51 ` Jiri Olsa
  2018-01-16 13:06   ` Jin, Yao
  2018-03-06  6:42 ` [tip:perf/core] " tip-bot for Jin Yao
  1 sibling, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2018-01-16 12:51 UTC (permalink / raw)
  To: Jin Yao
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

On Tue, Jan 16, 2018 at 11:43:08PM +0800, Jin Yao wrote:
> If we execute 'perf stat --per-thread' with non-root account
> (even set kernel.perf_event_paranoid = -1 yet), it reports the error:
> 
> jinyao@skl:~$ perf stat --per-thread
> Error:
> You may not have permission to collect system-wide stats.
> 
> Consider tweaking /proc/sys/kernel/perf_event_paranoid,
> which controls use of the performance events system by
> unprivileged users (without CAP_SYS_ADMIN).
> 
> The current value is 2:
> 
>   -1: Allow use of (almost) all events by all users
>       Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> >= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
>       Disallow raw tracepoint access by users without CAP_SYS_ADMIN
> >= 1: Disallow CPU event access by users without CAP_SYS_ADMIN
> >= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN
> 
> To make this setting permanent, edit /etc/sysctl.conf too, e.g.:
> 
>         kernel.perf_event_paranoid = -1
> 
> Perhaps the ptrace rule doesn't allow to trace some processes. But anyway
> the global --per-thread mode had better ignore such errors and continue
> working on other threads.
> 
> This patch will record the index of error thread in perf_evsel__open()
> and remove this thread before retrying.
> 
> For example (run with non-root, kernel.perf_event_paranoid isn't set):
> 
> jinyao@skl:~$ perf stat --per-thread
> ^C
>  Performance counter stats for 'system wide':
> 
>           vmstat-3458               6.171984      cpu-clock:u (msec)        #    0.000 CPUs utilized
>             perf-3670               0.515599      cpu-clock:u (msec)        #    0.000 CPUs utilized
>           vmstat-3458              1,163,643      cycles:u                  #    0.189 GHz
>             perf-3670                 40,881      cycles:u                  #    0.079 GHz
>           vmstat-3458              1,410,238      instructions:u            #    1.21  insn per cycle
>             perf-3670                  3,536      instructions:u            #    0.09  insn per cycle
>           vmstat-3458                288,937      branches:u                #   46.814 M/sec
>             perf-3670                    936      branches:u                #    1.815 M/sec
>           vmstat-3458                 15,195      branch-misses:u           #    5.26% of all branches
>             perf-3670                     76      branch-misses:u           #    8.12% of all branches
> 
>       12.651675247 seconds time elapsed

could we use the existing code like in attached patch?

we might also swap following warning for some generic one:
  WARNING: Ignored open failure for pid 28392

jirka


---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 98bf9d32f222..f9d4d3ad6fc5 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -276,6 +276,9 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
 			attr->enable_on_exec = 1;
 	}
 
+	if (stat_config.aggr_mode == AGGR_THREAD)
+		evsel->ignore_missing_thread = true;
+
 	if (target__has_cpu(&target) && !target__has_per_thread(&target))
 		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 8f971a2301d1..509ee175bc97 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1656,7 +1656,7 @@ static bool ignore_missing_thread(struct perf_evsel *evsel,
 		return false;
 
 	/* The -ESRCH is perf event syscall errno for pid's not found. */
-	if (err != -ESRCH)
+	if (err != -ESRCH && err != -EACCES)
 		return false;
 
 	/* If there's only one thread, let it fail. */

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-16 12:51 ` Jiri Olsa
@ 2018-01-16 13:06   ` Jin, Yao
  2018-01-16 13:17     ` Jiri Olsa
  0 siblings, 1 reply; 10+ messages in thread
From: Jin, Yao @ 2018-01-16 13:06 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

Just tested. But looks it's not OK for '--per-thread' case.

jinyao@skl:~/$ ./perf stat --per-thread
WARNING: Ignored open failure for pid 1
WARNING: Ignored open failure for pid 2
WARNING: Ignored open failure for pid 4
WARNING: Ignored open failure for pid 6
WARNING: Ignored open failure for pid 7
WARNING: Ignored open failure for pid 8
WARNING: Ignored open failure for pid 9
WARNING: Ignored open failure for pid 10
......
WARNING: Ignored open failure for pid 11783
WARNING: Ignored open failure for pid 12275
WARNING: Ignored open failure for pid 31733
WARNING: Ignored open failure for pid 31737
Error:
You may not have permission to collect system-wide stats.

Consider tweaking /proc/sys/kernel/perf_event_paranoid,
which controls use of the performance events system by
unprivileged users (without CAP_SYS_ADMIN).

The current value is 2:

   -1: Allow use of (almost) all events by all users
       Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
 >= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
       Disallow raw tracepoint access by users without CAP_SYS_ADMIN
 >= 1: Disallow CPU event access by users without CAP_SYS_ADMIN
 >= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN

To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

         kernel.perf_event_paranoid = -1

Thanks
Jin Yao

On 1/16/2018 8:51 PM, Jiri Olsa wrote:
> On Tue, Jan 16, 2018 at 11:43:08PM +0800, Jin Yao wrote:
>> If we execute 'perf stat --per-thread' with non-root account
>> (even set kernel.perf_event_paranoid = -1 yet), it reports the error:
>>
>> jinyao@skl:~$ perf stat --per-thread
>> Error:
>> You may not have permission to collect system-wide stats.
>>
>> Consider tweaking /proc/sys/kernel/perf_event_paranoid,
>> which controls use of the performance events system by
>> unprivileged users (without CAP_SYS_ADMIN).
>>
>> The current value is 2:
>>
>>    -1: Allow use of (almost) all events by all users
>>        Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>>> = 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
>>        Disallow raw tracepoint access by users without CAP_SYS_ADMIN
>>> = 1: Disallow CPU event access by users without CAP_SYS_ADMIN
>>> = 2: Disallow kernel profiling by users without CAP_SYS_ADMIN
>>
>> To make this setting permanent, edit /etc/sysctl.conf too, e.g.:
>>
>>          kernel.perf_event_paranoid = -1
>>
>> Perhaps the ptrace rule doesn't allow to trace some processes. But anyway
>> the global --per-thread mode had better ignore such errors and continue
>> working on other threads.
>>
>> This patch will record the index of error thread in perf_evsel__open()
>> and remove this thread before retrying.
>>
>> For example (run with non-root, kernel.perf_event_paranoid isn't set):
>>
>> jinyao@skl:~$ perf stat --per-thread
>> ^C
>>   Performance counter stats for 'system wide':
>>
>>            vmstat-3458               6.171984      cpu-clock:u (msec)        #    0.000 CPUs utilized
>>              perf-3670               0.515599      cpu-clock:u (msec)        #    0.000 CPUs utilized
>>            vmstat-3458              1,163,643      cycles:u                  #    0.189 GHz
>>              perf-3670                 40,881      cycles:u                  #    0.079 GHz
>>            vmstat-3458              1,410,238      instructions:u            #    1.21  insn per cycle
>>              perf-3670                  3,536      instructions:u            #    0.09  insn per cycle
>>            vmstat-3458                288,937      branches:u                #   46.814 M/sec
>>              perf-3670                    936      branches:u                #    1.815 M/sec
>>            vmstat-3458                 15,195      branch-misses:u           #    5.26% of all branches
>>              perf-3670                     76      branch-misses:u           #    8.12% of all branches
>>
>>        12.651675247 seconds time elapsed
> 
> could we use the existing code like in attached patch?
> 
> we might also swap following warning for some generic one:
>    WARNING: Ignored open failure for pid 28392
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 98bf9d32f222..f9d4d3ad6fc5 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -276,6 +276,9 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
>   			attr->enable_on_exec = 1;
>   	}
>   
> +	if (stat_config.aggr_mode == AGGR_THREAD)
> +		evsel->ignore_missing_thread = true;
> +
>   	if (target__has_cpu(&target) && !target__has_per_thread(&target))
>   		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
>   
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 8f971a2301d1..509ee175bc97 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1656,7 +1656,7 @@ static bool ignore_missing_thread(struct perf_evsel *evsel,
>   		return false;
>   
>   	/* The -ESRCH is perf event syscall errno for pid's not found. */
> -	if (err != -ESRCH)
> +	if (err != -ESRCH && err != -EACCES)
>   		return false;
>   
>   	/* If there's only one thread, let it fail. */
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-16 13:06   ` Jin, Yao
@ 2018-01-16 13:17     ` Jiri Olsa
  2018-01-16 14:10       ` Jin, Yao
  2018-01-22  5:10       ` Jin, Yao
  0 siblings, 2 replies; 10+ messages in thread
From: Jiri Olsa @ 2018-01-16 13:17 UTC (permalink / raw)
  To: Jin, Yao
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

On Tue, Jan 16, 2018 at 09:06:09PM +0800, Jin, Yao wrote:
> Just tested. But looks it's not OK for '--per-thread' case.

yea, I haven't tested much.. might need soem tweaking,
but my point was that it could be doable on one place
instead of introducing another if possible

jirka

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-16 13:17     ` Jiri Olsa
@ 2018-01-16 14:10       ` Jin, Yao
  2018-01-22  5:10       ` Jin, Yao
  1 sibling, 0 replies; 10+ messages in thread
From: Jin, Yao @ 2018-01-16 14:10 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin



On 1/16/2018 9:17 PM, Jiri Olsa wrote:
> On Tue, Jan 16, 2018 at 09:06:09PM +0800, Jin, Yao wrote:
>> Just tested. But looks it's not OK for '--per-thread' case.
> 
> yea, I haven't tested much.. might need soem tweaking,
> but my point was that it could be doable on one place
> instead of introducing another if possible
> 
> jirka
> 

Yes, I understand. It'd better we can put the code to more common places.

Actually I used the similar patch as yours at first. While I found it 
didn't work for the case of system-wide '--per-thread' then I developed 
current one.

Thanks
Jin Yao

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
@ 2018-01-16 15:43 Jin Yao
  2018-01-16 12:51 ` Jiri Olsa
  2018-03-06  6:42 ` [tip:perf/core] " tip-bot for Jin Yao
  0 siblings, 2 replies; 10+ messages in thread
From: Jin Yao @ 2018-01-16 15:43 UTC (permalink / raw)
  To: acme, jolsa, peterz, mingo, alexander.shishkin
  Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao

If we execute 'perf stat --per-thread' with non-root account
(even set kernel.perf_event_paranoid = -1 yet), it reports the error:

jinyao@skl:~$ perf stat --per-thread
Error:
You may not have permission to collect system-wide stats.

Consider tweaking /proc/sys/kernel/perf_event_paranoid,
which controls use of the performance events system by
unprivileged users (without CAP_SYS_ADMIN).

The current value is 2:

  -1: Allow use of (almost) all events by all users
      Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
      Disallow raw tracepoint access by users without CAP_SYS_ADMIN
>= 1: Disallow CPU event access by users without CAP_SYS_ADMIN
>= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN

To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

        kernel.perf_event_paranoid = -1

Perhaps the ptrace rule doesn't allow to trace some processes. But anyway
the global --per-thread mode had better ignore such errors and continue
working on other threads.

This patch will record the index of error thread in perf_evsel__open()
and remove this thread before retrying.

For example (run with non-root, kernel.perf_event_paranoid isn't set):

jinyao@skl:~$ perf stat --per-thread
^C
 Performance counter stats for 'system wide':

          vmstat-3458               6.171984      cpu-clock:u (msec)        #    0.000 CPUs utilized
            perf-3670               0.515599      cpu-clock:u (msec)        #    0.000 CPUs utilized
          vmstat-3458              1,163,643      cycles:u                  #    0.189 GHz
            perf-3670                 40,881      cycles:u                  #    0.079 GHz
          vmstat-3458              1,410,238      instructions:u            #    1.21  insn per cycle
            perf-3670                  3,536      instructions:u            #    0.09  insn per cycle
          vmstat-3458                288,937      branches:u                #   46.814 M/sec
            perf-3670                    936      branches:u                #    1.815 M/sec
          vmstat-3458                 15,195      branch-misses:u           #    5.26% of all branches
            perf-3670                     76      branch-misses:u           #    8.12% of all branches

      12.651675247 seconds time elapsed

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-stat.c    | 14 +++++++++++++-
 tools/perf/util/evsel.c      |  3 +++
 tools/perf/util/thread_map.c |  1 +
 tools/perf/util/thread_map.h |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 98bf9d3..bcdb47c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -632,7 +632,19 @@ static int __run_perf_stat(int argc, const char **argv)
                                 if (verbose > 0)
                                         ui__warning("%s\n", msg);
                                 goto try_again;
-                        }
+			} else if (target__has_per_thread(&target) &&
+				   evsel_list->threads &&
+				   evsel_list->threads->err_thread != -1) {
+				/*
+				 * For global --per-thread case, skip current
+				 * error thread.
+				 */
+				if (!thread_map__remove(evsel_list->threads,
+							evsel_list->threads->err_thread)) {
+					evsel_list->threads->err_thread = -1;
+					goto try_again;
+				}
+			}
 
 			perf_evsel__open_strerror(counter, &target,
 						  errno, msg, sizeof(msg));
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a4d256e..12f8733 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1899,6 +1899,9 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 		goto fallback_missing_features;
 	}
 out_close:
+	if (err)
+		threads->err_thread = thread;
+
 	do {
 		while (--thread >= 0) {
 			close(FD(evsel, cpu, thread));
diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index 3e1038f..870cb0c 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -32,6 +32,7 @@ static void thread_map__reset(struct thread_map *map, int start, int nr)
 	size_t size = (nr - start) * sizeof(map->map[0]);
 
 	memset(&map->map[start], 0, size);
+	map->err_thread = -1;
 }
 
 static struct thread_map *thread_map__realloc(struct thread_map *map, int nr)
diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
index 0a806b9..ac6baf1 100644
--- a/tools/perf/util/thread_map.h
+++ b/tools/perf/util/thread_map.h
@@ -14,6 +14,7 @@ struct thread_map_data {
 struct thread_map {
 	refcount_t refcnt;
 	int nr;
+	int err_thread;
 	struct thread_map_data map[];
 };
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-16 13:17     ` Jiri Olsa
  2018-01-16 14:10       ` Jin, Yao
@ 2018-01-22  5:10       ` Jin, Yao
  2018-01-23 14:19         ` Jiri Olsa
  1 sibling, 1 reply; 10+ messages in thread
From: Jin, Yao @ 2018-01-22  5:10 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin



On 1/16/2018 9:17 PM, Jiri Olsa wrote:
> On Tue, Jan 16, 2018 at 09:06:09PM +0800, Jin, Yao wrote:
>> Just tested. But looks it's not OK for '--per-thread' case.
> 
> yea, I haven't tested much.. might need soem tweaking,
> but my point was that it could be doable on one place
> instead of introducing another if possible
> 
> jirka
> 

Hi Jiri,

I ever considered to move the operation of removing error thread to 
perf_evsel__fallback(). The perf_evsel__fallback() is common code and 
it's shared by perf report, perf stat and perf top.

While finally I think it'd better let the caller decide to remove error 
thread and try again, or just return the warning message. 
perf_evsel__fallback() probably doesn't know what the caller want to do.

That's my current thinking. Maybe there will be a better fix...

Thanks
Jin Yao

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-22  5:10       ` Jin, Yao
@ 2018-01-23 14:19         ` Jiri Olsa
  2018-02-27  0:53           ` Jin, Yao
  0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2018-01-23 14:19 UTC (permalink / raw)
  To: Jin, Yao
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

On Mon, Jan 22, 2018 at 01:10:31PM +0800, Jin, Yao wrote:
> 
> 
> On 1/16/2018 9:17 PM, Jiri Olsa wrote:
> > On Tue, Jan 16, 2018 at 09:06:09PM +0800, Jin, Yao wrote:
> > > Just tested. But looks it's not OK for '--per-thread' case.
> > 
> > yea, I haven't tested much.. might need soem tweaking,
> > but my point was that it could be doable on one place
> > instead of introducing another if possible
> > 
> > jirka
> > 
> 
> Hi Jiri,
> 
> I ever considered to move the operation of removing error thread to
> perf_evsel__fallback(). The perf_evsel__fallback() is common code and it's
> shared by perf report, perf stat and perf top.
> 
> While finally I think it'd better let the caller decide to remove error
> thread and try again, or just return the warning message.
> perf_evsel__fallback() probably doesn't know what the caller want to do.
> 
> That's my current thinking. Maybe there will be a better fix...

ok, can't think of better fix atm.. looks good ;-)

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-23 14:19         ` Jiri Olsa
@ 2018-02-27  0:53           ` Jin, Yao
  2018-02-27 14:37             ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 10+ messages in thread
From: Jin, Yao @ 2018-02-27  0:53 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin



On 1/23/2018 10:19 PM, Jiri Olsa wrote:
> On Mon, Jan 22, 2018 at 01:10:31PM +0800, Jin, Yao wrote:
>>
>>
>> On 1/16/2018 9:17 PM, Jiri Olsa wrote:
>>> On Tue, Jan 16, 2018 at 09:06:09PM +0800, Jin, Yao wrote:
>>>> Just tested. But looks it's not OK for '--per-thread' case.
>>>
>>> yea, I haven't tested much.. might need soem tweaking,
>>> but my point was that it could be doable on one place
>>> instead of introducing another if possible
>>>
>>> jirka
>>>
>>
>> Hi Jiri,
>>
>> I ever considered to move the operation of removing error thread to
>> perf_evsel__fallback(). The perf_evsel__fallback() is common code and it's
>> shared by perf report, perf stat and perf top.
>>
>> While finally I think it'd better let the caller decide to remove error
>> thread and try again, or just return the warning message.
>> perf_evsel__fallback() probably doesn't know what the caller want to do.
>>
>> That's my current thinking. Maybe there will be a better fix...
> 
> ok, can't think of better fix atm.. looks good ;-)
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> thanks,
> jirka
> 

Hi Arnaldo,

Could this fix be accepted?

Thanks
Jin Yao

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-02-27  0:53           ` Jin, Yao
@ 2018-02-27 14:37             ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-02-27 14:37 UTC (permalink / raw)
  To: Jin, Yao
  Cc: Jiri Olsa, jolsa, peterz, mingo, alexander.shishkin,
	Linux-kernel, ak, kan.liang, yao.jin

Em Tue, Feb 27, 2018 at 08:53:13AM +0800, Jin, Yao escreveu:
> 
> 
> On 1/23/2018 10:19 PM, Jiri Olsa wrote:
> > On Mon, Jan 22, 2018 at 01:10:31PM +0800, Jin, Yao wrote:
> > > 
> > > 
> > > On 1/16/2018 9:17 PM, Jiri Olsa wrote:
> > > > On Tue, Jan 16, 2018 at 09:06:09PM +0800, Jin, Yao wrote:
> > > > > Just tested. But looks it's not OK for '--per-thread' case.
> > > > 
> > > > yea, I haven't tested much.. might need soem tweaking,
> > > > but my point was that it could be doable on one place
> > > > instead of introducing another if possible
> > > > 
> > > > jirka
> > > > 
> > > 
> > > Hi Jiri,
> > > 
> > > I ever considered to move the operation of removing error thread to
> > > perf_evsel__fallback(). The perf_evsel__fallback() is common code and it's
> > > shared by perf report, perf stat and perf top.
> > > 
> > > While finally I think it'd better let the caller decide to remove error
> > > thread and try again, or just return the warning message.
> > > perf_evsel__fallback() probably doesn't know what the caller want to do.
> > > 
> > > That's my current thinking. Maybe there will be a better fix...
> > 
> > ok, can't think of better fix atm.. looks good ;-)
> > 
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > 
> > thanks,
> > jirka
> > 
> 
> Hi Arnaldo,
> 
> Could this fix be accepted?

yeah, its just strange that that err_thread is not used, applied.

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip:perf/core] perf stat: Ignore error thread when enabling system-wide --per-thread
  2018-01-16 15:43 [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread Jin Yao
  2018-01-16 12:51 ` Jiri Olsa
@ 2018-03-06  6:42 ` tip-bot for Jin Yao
  1 sibling, 0 replies; 10+ messages in thread
From: tip-bot for Jin Yao @ 2018-03-06  6:42 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: alexander.shishkin, acme, linux-kernel, jolsa, mingo, yao.jin,
	ak, kan.liang, peterz, hpa, tglx

Commit-ID:  ab6c79b819f5a50cf41a11ebec17bef63b530333
Gitweb:     https://git.kernel.org/tip/ab6c79b819f5a50cf41a11ebec17bef63b530333
Author:     Jin Yao <yao.jin@linux.intel.com>
AuthorDate: Tue, 16 Jan 2018 23:43:08 +0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 27 Feb 2018 11:29:21 -0300

perf stat: Ignore error thread when enabling system-wide --per-thread

If we execute 'perf stat --per-thread' with non-root account (even set
kernel.perf_event_paranoid = -1 yet), it reports the error:

  jinyao@skl:~$ perf stat --per-thread
  Error:
  You may not have permission to collect system-wide stats.

  Consider tweaking /proc/sys/kernel/perf_event_paranoid,
  which controls use of the performance events system by
  unprivileged users (without CAP_SYS_ADMIN).

  The current value is 2:

    -1: Allow use of (almost) all events by all users
        Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
  >= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
        Disallow raw tracepoint access by users without CAP_SYS_ADMIN
  >= 1: Disallow CPU event access by users without CAP_SYS_ADMIN
  >= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN

  To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

          kernel.perf_event_paranoid = -1

Perhaps the ptrace rule doesn't allow to trace some processes. But anyway
the global --per-thread mode had better ignore such errors and continue
working on other threads.

This patch will record the index of error thread in perf_evsel__open()
and remove this thread before retrying.

For example (run with non-root, kernel.perf_event_paranoid isn't set):

  jinyao@skl:~$ perf stat --per-thread
  ^C
   Performance counter stats for 'system wide':

         vmstat-3458    6.171984   cpu-clock:u (msec) #  0.000 CPUs utilized
           perf-3670    0.515599   cpu-clock:u (msec) #  0.000 CPUs utilized
         vmstat-3458   1,163,643   cycles:u           #  0.189 GHz
           perf-3670      40,881   cycles:u           #  0.079 GHz
         vmstat-3458   1,410,238   instructions:u     #  1.21  insn per cycle
           perf-3670       3,536   instructions:u     #  0.09  insn per cycle
         vmstat-3458     288,937   branches:u         # 46.814 M/sec
           perf-3670         936   branches:u         #  1.815 M/sec
         vmstat-3458      15,195   branch-misses:u    #  5.26% of all branches
           perf-3670          76   branch-misses:u    #  8.12% of all branches

        12.651675247 seconds time elapsed

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1516117388-10120-1-git-send-email-yao.jin@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c    | 14 +++++++++++++-
 tools/perf/util/evsel.c      |  3 +++
 tools/perf/util/thread_map.c |  1 +
 tools/perf/util/thread_map.h |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index fadcff52cd09..6214d2b220b2 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -637,7 +637,19 @@ try_again:
                                 if (verbose > 0)
                                         ui__warning("%s\n", msg);
                                 goto try_again;
-                        }
+			} else if (target__has_per_thread(&target) &&
+				   evsel_list->threads &&
+				   evsel_list->threads->err_thread != -1) {
+				/*
+				 * For global --per-thread case, skip current
+				 * error thread.
+				 */
+				if (!thread_map__remove(evsel_list->threads,
+							evsel_list->threads->err_thread)) {
+					evsel_list->threads->err_thread = -1;
+					goto try_again;
+				}
+			}
 
 			perf_evsel__open_strerror(counter, &target,
 						  errno, msg, sizeof(msg));
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ef351688b797..b56e1c2ddaee 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1915,6 +1915,9 @@ try_fallback:
 		goto fallback_missing_features;
 	}
 out_close:
+	if (err)
+		threads->err_thread = thread;
+
 	do {
 		while (--thread >= 0) {
 			close(FD(evsel, cpu, thread));
diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index 729dad8f412d..5d467d8ae9ab 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -32,6 +32,7 @@ static void thread_map__reset(struct thread_map *map, int start, int nr)
 	size_t size = (nr - start) * sizeof(map->map[0]);
 
 	memset(&map->map[start], 0, size);
+	map->err_thread = -1;
 }
 
 static struct thread_map *thread_map__realloc(struct thread_map *map, int nr)
diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h
index 5ec91cfd1869..2f689c90a8c6 100644
--- a/tools/perf/util/thread_map.h
+++ b/tools/perf/util/thread_map.h
@@ -14,6 +14,7 @@ struct thread_map_data {
 struct thread_map {
 	refcount_t refcnt;
 	int nr;
+	int err_thread;
 	struct thread_map_data map[];
 };
 

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-03-06  6:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-16 15:43 [PATCH] perf stat: Ignore error thread when enabling system-wide --per-thread Jin Yao
2018-01-16 12:51 ` Jiri Olsa
2018-01-16 13:06   ` Jin, Yao
2018-01-16 13:17     ` Jiri Olsa
2018-01-16 14:10       ` Jin, Yao
2018-01-22  5:10       ` Jin, Yao
2018-01-23 14:19         ` Jiri Olsa
2018-02-27  0:53           ` Jin, Yao
2018-02-27 14:37             ` Arnaldo Carvalho de Melo
2018-03-06  6:42 ` [tip:perf/core] " tip-bot for Jin Yao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).