All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.