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

Groups > linux.kernel > #1574620 > unrolled thread

[RFC PATCH] perf/stat: Add --disable-hwdt

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-06 13:20 +0100
    Re: [RFC PATCH] perf/stat: Add --disable-hwdt Ingo Molnar <mingo@kernel.org> - 2017-02-06 13:30 +0100
      Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-06 13:50 +0100
        Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-06 13:50 +0100
          Re: [RFC PATCH] perf/stat: Add --disable-hwdt Robert Richter <rric@kernel.org> - 2017-02-06 14:20 +0100
            Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-06 14:30 +0100
              Re: [RFC PATCH] perf/stat: Add --disable-hwdt Ingo Molnar <mingo@kernel.org> - 2017-02-07 08:30 +0100
                Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-07 12:00 +0100
                  Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-07 16:10 +0100
                    Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-11 18:10 +0100
                      Re: [RFC PATCH] perf/stat: Add --disable-hwdt Ingo Molnar <mingo@kernel.org> - 2017-02-11 19:00 +0100
                        Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-11 19:40 +0100
                          Re: [RFC PATCH] perf/stat: Add --disable-hwdt Ingo Molnar <mingo@kernel.org> - 2017-02-11 21:50 +0100
            Re: [RFC PATCH] perf/stat: Add --disable-hwdt Vince Weaver <vince@deater.net> - 2017-02-06 15:30 +0100
              Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-06 18:10 +0100
          Re: [RFC PATCH] perf/stat: Add --disable-hwdt Borislav Petkov <bp@alien8.de> - 2017-02-07 02:10 +0100
            [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers Borislav Petkov <bp@alien8.de> - 2017-02-07 02:10 +0100
              Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-07 02:50 +0100
                Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers Borislav Petkov <bp@alien8.de> - 2017-02-07 11:40 +0100
                  Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers Borislav Petkov <bp@alien8.de> - 2017-02-07 16:10 +0100
                    Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-07 16:40 +0100
                  Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-07 16:10 +0100
            [PATCH 2/2] perf stat: Disable HW watchdog around a perf stat session Borislav Petkov <bp@alien8.de> - 2017-02-07 02:20 +0100
              Re: [PATCH 2/2] perf stat: Disable HW watchdog around a perf stat  session Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-07 02:50 +0100
        Re: [RFC PATCH] perf/stat: Add --disable-hwdt Ingo Molnar <mingo@kernel.org> - 2017-02-06 13:50 +0100

Page 1 of 2  [1] 2  Next page →

#1574620 — [RFC PATCH] perf/stat: Add --disable-hwdt

Hi guys,

so I've been tracing recently on an AMD F15h which has those funky counter
constraints and am seeing this:

# ./perf stat sleep 1

 Performance counter stats for 'sleep 1':

          0.749208      task-clock (msec)         #    0.001 CPUs utilized          
                 1      context-switches          #    0.001 M/sec                  
                 0      cpu-migrations            #    0.000 K/sec                  
                54      page-faults               #    0.072 M/sec                  
         1,122,815      cycles                    #    1.499 GHz                    
           286,740      stalled-cycles-frontend   #   25.54% frontend cycles idle   
     <not counted>      stalled-cycles-backend                                        (0.00%)
     ^^^^^^^^^^^^
     <not counted>      instructions                                                  (0.00%)
     ^^^^^^^^^^^^
     <not counted>      branches                                                      (0.00%)
     <not counted>      branch-misses                                                 (0.00%)

       1.001550070 seconds time elapsed


The problem is that the HW watchdog thing is already taking up a
counter so when perf stat uses the default counters and when we reach
stalled-cycles-backend, we run out of counters for the remaining events.

So how about something like this:

# ./perf stat --disable-hwdt sleep 1

 Performance counter stats for 'sleep 1':

          0.782552      task-clock (msec)         #    0.001 CPUs utilized          
                 1      context-switches          #    0.001 M/sec                  
                 0      cpu-migrations            #    0.000 K/sec                  
                55      page-faults               #    0.070 M/sec                  
         1,163,246      cycles                    #    1.486 GHz                    
           293,598      stalled-cycles-frontend   #   25.24% frontend cycles idle   
           400,017      stalled-cycles-backend    #   34.39% backend cycles idle    
           676,505      instructions              #    0.58  insn per cycle         
                                                  #    0.59  stalled cycles per insn
           133,822      branches                  #  171.007 M/sec                  
             7,319      branch-misses             #    5.47% of all branches        

       1.001660058 seconds time elapsed

We did explore other opportunities on IRC like sharing counters or
making the HW WDT thing a 'soft' counter but all those are nasty and
probably not really worth the trouble of touching perf core just so that
this works.

Besides, future generations don't have those constraints anymore so it
is only F15h.

Below is a silly patch as a syntactic sugar helper for perf stat. This
is just an RFC anyway, I'll do it properly with fopen() if you're ok
with the approach.

---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..7b466dde8012 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -146,6 +146,7 @@ static aggr_get_id_t		aggr_get_id;
 static bool			append_file;
 static const char		*output_name;
 static int			output_fd;
+static bool			disable_hwdt;
 
 struct perf_stat {
 	bool			 record;
@@ -1575,6 +1576,10 @@ static void sig_atexit(void)
 
 	sigprocmask(SIG_SETMASK, &oset, NULL);
 
+	if (disable_hwdt)
+		if (system("echo 1 > /proc/sys/kernel/nmi_watchdog"))
+			return;
+
 	if (signr == -1)
 		return;
 
@@ -1659,6 +1664,8 @@ static const struct option stat_options[] = {
 			"Only print computed metrics. No raw values", enable_metric_only),
 	OPT_BOOLEAN(0, "topdown", &topdown_run,
 			"measure topdown level 1 statistics"),
+	OPT_BOOLEAN(0, "disable-hwdt", &disable_hwdt,
+			"Disable HW watchdog to free-up a counter"),
 	OPT_END()
 };
 
@@ -2523,6 +2530,11 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (perf_stat_init_aggr_mode())
 		goto out;
 
+	if (disable_hwdt) {
+		if (system("echo 0 > /proc/sys/kernel/nmi_watchdog"))
+			goto out;
+	}
+
 	/*
 	 * We dont want to block the signals - that would cause
 	 * child tasks to inherit that and Ctrl-C would not work.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [next] | [standalone]

#1574630

* Borislav Petkov <bp@alien8.de> wrote:

> Hi guys,
> 
> so I've been tracing recently on an AMD F15h which has those funky counter
> constraints and am seeing this:
> 
> # ./perf stat sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           0.749208      task-clock (msec)         #    0.001 CPUs utilized          
>                  1      context-switches          #    0.001 M/sec                  
>                  0      cpu-migrations            #    0.000 K/sec                  
>                 54      page-faults               #    0.072 M/sec                  
>          1,122,815      cycles                    #    1.499 GHz                    
>            286,740      stalled-cycles-frontend   #   25.54% frontend cycles idle   
>      <not counted>      stalled-cycles-backend                                        (0.00%)
>      ^^^^^^^^^^^^
>      <not counted>      instructions                                                  (0.00%)
>      ^^^^^^^^^^^^
>      <not counted>      branches                                                      (0.00%)
>      <not counted>      branch-misses                                                 (0.00%)
> 
>        1.001550070 seconds time elapsed
> 
> 
> The problem is that the HW watchdog thing is already taking up a
> counter so when perf stat uses the default counters and when we reach
> stalled-cycles-backend, we run out of counters for the remaining events.
> 
> So how about something like this:
> 
> # ./perf stat --disable-hwdt sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           0.782552      task-clock (msec)         #    0.001 CPUs utilized          
>                  1      context-switches          #    0.001 M/sec                  
>                  0      cpu-migrations            #    0.000 K/sec                  
>                 55      page-faults               #    0.070 M/sec                  
>          1,163,246      cycles                    #    1.486 GHz                    
>            293,598      stalled-cycles-frontend   #   25.24% frontend cycles idle   
>            400,017      stalled-cycles-backend    #   34.39% backend cycles idle    
>            676,505      instructions              #    0.58  insn per cycle         
>                                                   #    0.59  stalled cycles per insn
>            133,822      branches                  #  171.007 M/sec                  
>              7,319      branch-misses             #    5.47% of all branches        
> 
>        1.001660058 seconds time elapsed
> 
> We did explore other opportunities on IRC like sharing counters or
> making the HW WDT thing a 'soft' counter but all those are nasty and
> probably not really worth the trouble of touching perf core just so that
> this works.
> 
> Besides, future generations don't have those constraints anymore so it
> is only F15h.
> 
> Below is a silly patch as a syntactic sugar helper for perf stat. This
> is just an RFC anyway, I'll do it properly with fopen() if you're ok
> with the approach.

Looks sensible, and I'd in fact make this the new default behavior (if root runs 
perf stat) - i.e. add a flag to re-enable it, for the rare case where we want to 
debug a hard deadlock while running perf stat ...

Thanks,

	Ingo

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

#1574636

On Mon, Feb 06, 2017 at 01:22:31PM +0100, Ingo Molnar wrote:
> Looks sensible, and I'd in fact make this the new default behavior (if root runs 
> perf stat) - i.e. add a flag to re-enable it, for the rare case where we want to 
> debug a hard deadlock while running perf stat ...

I'd probably only need to save the previous state, in case it was
disabled for whatever reason.

So actually, I'll make it so that we restore the previous state at exit.
Whatever it was. But while perf stat runs as root, we will disable it by
default.

/me scratches head a bit...

Oh ok, even better - no need for a cmdline option then.

Or does it *ever* make sense to have watchdog running *while* perf stat
runs too?

Because if it doesn't, turning it off by default *if* it was enabled

	[ should be, we do enable HW WDT by default ]

and restoring it should be the proper thing to do. Without cmdline
option.

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1574637

On Mon, Feb 06, 2017 at 01:44:48PM +0100, Ingo Molnar wrote:
> Yeah, if for some whatever reason perf locks up while running perf stat as root, 
> so I'd keep the option, as a general principle. :-/

--dont-disable-hwdt it is.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1574670

On 06.02.17 13:49:37, Borislav Petkov wrote:
> On Mon, Feb 06, 2017 at 01:44:48PM +0100, Ingo Molnar wrote:
> > Yeah, if for some whatever reason perf locks up while running perf stat as root, 
> > so I'd keep the option, as a general principle. :-/
> 
> --dont-disable-hwdt it is.

Isn't there the danger the previous watchdog state is never restored
if for some reason perf got killed? So maybe have some other task
running that restores it once perf is gone.

-Robert

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

#1574675

On Mon, Feb 06, 2017 at 02:18:32PM +0100, Robert Richter wrote:
> Isn't there the danger the previous watchdog state is never restored
> if for some reason perf got killed? So maybe have some other task
> running that restores it once perf is gone.

Currently, I'm restoring it in the atexit() sighandler. Isn't that
always called?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1575429

* Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Feb 06, 2017 at 02:18:32PM +0100, Robert Richter wrote:
>
> > Isn't there the danger the previous watchdog state is never restored if for 
> > some reason perf got killed? So maybe have some other task running that 
> > restores it once perf is gone.
> 
> Currently, I'm restoring it in the atexit() sighandler. Isn't that always 
> called?

Normally yes, but it's not guaranteed as atexit() is all user-space, SIGKILL (or 
OOM) or a straight exit (or a crash in the exit handler itself) will cause it to 
not run.

But there's only so much we can do about that, the /proc/sys API is fundamentally 
lossy in that regard. We'd have to add much more involved kernel support to 
guarantee that the watchdog state is restored.

A way to do it would be create a new /proc/sys/kernel/watchdog_disable_file that 
disables that watchdog while it's _open_. When a task exits and the kernel 
automatically closes the file, the watchdog is re-enabled again. (Or the process 
itself can close the file too.)

This method would also nest properly and would handle multi-processes races 
correctly: for example if a script runs perf as root, and root uses 'perf top', 
the two should not race and the hardware watchdog should not end up being 
disabled...

Thanks,

	Ingo

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

#1575558

On Tue, Feb 07, 2017 at 08:25:12AM +0100, Ingo Molnar wrote:
> But there's only so much we can do about that, the /proc/sys API is fundamentally 
> lossy in that regard. We'd have to add much more involved kernel support to 
> guarantee that the watchdog state is restored.

So I think doing all this is meh but I guess I probably should do it
just so that we're thorough.

> A way to do it would be create a new /proc/sys/kernel/watchdog_disable_file that 
> disables that watchdog while it's _open_. When a task exits and the kernel 
> automatically closes the file, the watchdog is re-enabled again. (Or the process 
> itself can close the file too.)
> 
> This method would also nest properly and would handle multi-processes races 
> correctly: for example if a script runs perf as root, and root uses 'perf top', 
> the two should not race and the hardware watchdog should not end up being 
> disabled...

Hmm, so I don't like the aspect of adding a /proc file just for that.

Can we do something with sys_perf_event_open(..., flags) instead and
pass in a new flag that says:

PERF_FLAG_SHOO_COUNTER_USERS

or so which would go and turn off HW WDT (and possibly future things
using counters) while we're running a session?

The name should be generic enough so that we can use it for future
temporary disabling of things while a perf session runs.

Then on perf's exit path - I see there are a bunch of _destroy() things
being called when events are disappearing - we'd reenable stuff again
based on that flag.

This way we're clean in userspace and have the maximum control over
everything since we're in the kernel.

From a quick staring, there's PERF_FLAG_FD_CLOEXEC which is a good
example for something like that. It doesn't do what I'd like to do but I
think I should model this in a similar fashion.

Thoughts?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1575790

Btw,

I'm wondering if, alternatively, we add this to the manpage of perf stat
and be done with it:

"Remember to turn off HW watchdog temporarily as it is taking up one hw
counter:

$ echo  0 > /proc/sys/kernel/nmi_watchdog
 ... perf workload
$ echo  1 > /proc/sys/kernel/nmi_watchdog

in case some of the specified events don't get counted."

This solves the problem rather simply :-)))

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1579032

Ok,

turns out perf-list(1) already talks about it in the "EVENT GROUPS"
section. How about this then:

---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..2d18283574db 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -146,6 +146,7 @@ static aggr_get_id_t		aggr_get_id;
 static bool			append_file;
 static const char		*output_name;
 static int			output_fd;
+static int			print_free_counters_hint;
 
 struct perf_stat {
 	bool			 record;
@@ -1109,6 +1110,9 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 			counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,
 			csv_sep);
 
+		if (counter->supported)
+			print_free_counters_hint = 1;
+
 		fprintf(stat_config.output, "%-*s%s",
 			csv_output ? 0 : unit_width,
 			counter->unit, csv_sep);
@@ -1476,7 +1480,13 @@ static void print_footer(void)
 		print_noise_pct(stddev_stats(&walltime_nsecs_stats),
 				avg_stats(&walltime_nsecs_stats));
 	}
+
 	fprintf(output, "\n\n");
+
+	if (print_free_counters_hint)
+		fprintf(output,
+"Some events couldn't be scheduled. Consider freeing some counters by disabling the NMI watchdog temporarily, \n"
+"for example. See perf-list(1) manpage.\n");
 }
 
 static void print_counters(struct timespec *ts, int argc, const char **argv)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1579055

* Borislav Petkov <bp@alien8.de> wrote:

> Ok,
> 
> turns out perf-list(1) already talks about it in the "EVENT GROUPS"
> section. How about this then:
> 
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index a02f2e965628..2d18283574db 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -146,6 +146,7 @@ static aggr_get_id_t		aggr_get_id;
>  static bool			append_file;
>  static const char		*output_name;
>  static int			output_fd;
> +static int			print_free_counters_hint;
>  
>  struct perf_stat {
>  	bool			 record;
> @@ -1109,6 +1110,9 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
>  			counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,
>  			csv_sep);
>  
> +		if (counter->supported)
> +			print_free_counters_hint = 1;
> +
>  		fprintf(stat_config.output, "%-*s%s",
>  			csv_output ? 0 : unit_width,
>  			counter->unit, csv_sep);
> @@ -1476,7 +1480,13 @@ static void print_footer(void)
>  		print_noise_pct(stddev_stats(&walltime_nsecs_stats),
>  				avg_stats(&walltime_nsecs_stats));
>  	}
> +
>  	fprintf(output, "\n\n");
> +
> +	if (print_free_counters_hint)
> +		fprintf(output,
> +"Some events couldn't be scheduled. Consider freeing some counters by disabling the NMI watchdog temporarily, \n"
> +"for example. See perf-list(1) manpage.\n");

So I checked the perf-list manpage and it didn't tell me much about how to disable 
the NMI watchdog.

How about a more proactive hint, something like:

	To disable the NMI watchdog permanently, do:

		sudo echo kernel.nmi_watchdog=0 >> /etc/sysctl.conf
		sudo sysctl -p

? (untested!)

Thanks,

	Ingo

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

#1579067

On Sat, Feb 11, 2017 at 06:59:10PM +0100, Ingo Molnar wrote:
> So I checked the perf-list manpage and it didn't tell me much about how to disable 
> the NMI watchdog.

Oh, it is buried there:

"
EVENT GROUPS
------------

...


Globally pinned events can limit the number of counters available for
other groups. On x86 systems, the NMI watchdog pins a counter by default.
The nmi watchdog can be disabled as root with

	echo 0 > /proc/sys/kernel/nmi_watchdog"

> How about a more proactive hint, something like:
> 
> 	To disable the NMI watchdog permanently, do:

Why permanently? We want it to run and be disabled around the
measurement only. Anyway, here's something more to the point:

---
From: Borislav Petkov <bp@suse.de>
Date: Tue, 7 Feb 2017 01:40:05 +0100
Subject: [PATCH -v2] perf stat: Issue a HW watchdog disable hint

When using perf stat on an AMD F15h system with the default hw events
attributes, some of the events don't get counted:

 Performance counter stats for 'sleep 1':

          0.749208      task-clock (msec)         #    0.001 CPUs utilized
                 1      context-switches          #    0.001 M/sec
                 0      cpu-migrations            #    0.000 K/sec
                54      page-faults               #    0.072 M/sec
         1,122,815      cycles                    #    1.499 GHz
           286,740      stalled-cycles-frontend   #   25.54% frontend cycles idle
     <not counted>      stalled-cycles-backend                                        (0.00%)
     ^^^^^^^^^^^^
     <not counted>      instructions                                                  (0.00%)
     ^^^^^^^^^^^^
     <not counted>      branches                                                      (0.00%)
     <not counted>      branch-misses                                                 (0.00%)

       1.001550070 seconds time elapsed

The reason is that we have the HW watchdog consuming one PMU counter
and when perf tries to schedule 6 events on 6 counters and some of
those counters are constrained to only a specific subset of PMCs by the
hardware, the event scheduling fails.

So issue a hint to disable the HW watchdog around a perf stat session.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 tools/perf/builtin-stat.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..a2763243a03d 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -146,6 +146,7 @@ static aggr_get_id_t		aggr_get_id;
 static bool			append_file;
 static const char		*output_name;
 static int			output_fd;
+static int			print_free_counters_hint;
 
 struct perf_stat {
 	bool			 record;
@@ -1109,6 +1110,9 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 			counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,
 			csv_sep);
 
+		if (counter->supported)
+			print_free_counters_hint = 1;
+
 		fprintf(stat_config.output, "%-*s%s",
 			csv_output ? 0 : unit_width,
 			counter->unit, csv_sep);
@@ -1477,6 +1481,13 @@ static void print_footer(void)
 				avg_stats(&walltime_nsecs_stats));
 	}
 	fprintf(output, "\n\n");
+
+	if (print_free_counters_hint)
+		fprintf(output,
+"Some events weren't counted. Try disabling the NMI watchdog:\n"
+"	echo 0 > /proc/sys/kernel/nmi_watchdog\n"
+"	perf stat ...\n"
+"	echo 1 > /proc/sys/kernel/nmi_watchdog\n");
 }
 
 static void print_counters(struct timespec *ts, int argc, const char **argv)
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1579114

* Borislav Petkov <bp@alien8.de> wrote:

> On Sat, Feb 11, 2017 at 06:59:10PM +0100, Ingo Molnar wrote:
> > So I checked the perf-list manpage and it didn't tell me much about how to disable 
> > the NMI watchdog.
> 
> Oh, it is buried there:
> 
> "
> EVENT GROUPS
> ------------
> 
> ...
> 
> 
> Globally pinned events can limit the number of counters available for
> other groups. On x86 systems, the NMI watchdog pins a counter by default.
> The nmi watchdog can be disabled as root with
> 
> 	echo 0 > /proc/sys/kernel/nmi_watchdog"
> 
> > How about a more proactive hint, something like:
> > 
> > 	To disable the NMI watchdog permanently, do:
> 
> Why permanently? We want it to run and be disabled around the
> measurement only. Anyway, here's something more to the point:

> +	if (print_free_counters_hint)
> +		fprintf(output,
> +"Some events weren't counted. Try disabling the NMI watchdog:\n"
> +"	echo 0 > /proc/sys/kernel/nmi_watchdog\n"
> +"	perf stat ...\n"
> +"	echo 1 > /proc/sys/kernel/nmi_watchdog\n");
>  }

Ok, looks good to me!

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

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

#1574821

On Mon, 6 Feb 2017, Robert Richter wrote:

> On 06.02.17 13:49:37, Borislav Petkov wrote:
> > On Mon, Feb 06, 2017 at 01:44:48PM +0100, Ingo Molnar wrote:
> > > Yeah, if for some whatever reason perf locks up while running perf stat as root, 
> > > so I'd keep the option, as a general principle. :-/
> > 
> > --dont-disable-hwdt it is.
> 
> Isn't there the danger the previous watchdog state is never restored
> if for some reason perf got killed? So maybe have some other task
> running that restores it once perf is gone.

minor issue, but is it possibly to do anything about dmesg spam?  From 
what I recall every time you enable and disable the watchdog the kernel 
prints a message.  Makes for messy logs, especially when you run the 
perf_fuzzer as root.

Vince

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

#1574995

On Mon, Feb 06, 2017 at 09:23:33AM -0500, Vince Weaver wrote:
> minor issue, but is it possibly to do anything about dmesg spam?  From 
> what I recall every time you enable and disable the watchdog the kernel 
> prints a message.  Makes for messy logs, especially when you run the 
> perf_fuzzer as root.

You mean this:

[81304.460656] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.

We could turn it onto a

pr_info_once().

I mean, the feedback that the thing has been enabled is the 1 in
/proc/sys/kernel/nmi_watchdog anyway...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1575302

Ok,

here's something (as replies to this message) which looks much more
decent.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1575303 — [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers

From: Borislav Petkov <bp@suse.de>

Add helper functions to be able to read/write ints into proc files.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 tools/lib/api/fs/fs.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 tools/lib/api/fs/fs.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
index 4b6bfc43cccf..8e4b9fe18b75 100644
--- a/tools/lib/api/fs/fs.c
+++ b/tools/lib/api/fs/fs.c
@@ -314,6 +314,22 @@ int filename__read_int(const char *filename, int *value)
 	return err;
 }
 
+int filename__write_int(const char *filename, int value)
+{
+	char line[64];
+	int fd = open(filename, O_WRONLY), err = -1;
+
+	if (fd < 0)
+		return -1;
+
+	snprintf(line, sizeof(int), "%d", value);
+
+	err = write(fd, line, strnlen(line, 64));
+
+	close(fd);
+	return err;
+}
+
 /*
  * Parses @value out of @filename with strtoull.
  * By using 0 for base, the strtoull detects the
@@ -400,6 +416,32 @@ int procfs__read_str(const char *entry, char **buf, size_t *sizep)
 	return filename__read_str(path, buf, sizep);
 }
 
+int procfs__read_int(const char *entry, int *value)
+{
+	char path[PATH_MAX];
+	const char *procfs = procfs__mountpoint();
+
+	if (!procfs)
+		return -1;
+
+	snprintf(path, sizeof(path), "%s/%s", procfs, entry);
+
+	return filename__read_int(path, value);
+}
+
+int procfs__write_int(const char *entry, int value)
+{
+	char path[PATH_MAX];
+	const char *procfs = procfs__mountpoint();
+
+	if (!procfs)
+		return -1;
+
+	snprintf(path, sizeof(path), "%s/%s", procfs, entry);
+
+	return filename__write_int(path, value);
+}
+
 int sysfs__read_ull(const char *entry, unsigned long long *value)
 {
 	char path[PATH_MAX];
diff --git a/tools/lib/api/fs/fs.h b/tools/lib/api/fs/fs.h
index 6b332dc74498..095f25d4c70f 100644
--- a/tools/lib/api/fs/fs.h
+++ b/tools/lib/api/fs/fs.h
@@ -28,9 +28,12 @@ FS(bpf_fs)
 
 
 int filename__read_int(const char *filename, int *value);
+int filename__write_int(const char *filename, int value);
 int filename__read_ull(const char *filename, unsigned long long *value);
 int filename__read_str(const char *filename, char **buf, size_t *sizep);
 
+int procfs__read_int(const char *entry, int *value);
+int procfs__write_int(const char *entry, int value);
 int procfs__read_str(const char *entry, char **buf, size_t *sizep);
 
 int sysctl__read_int(const char *sysctl, int *value);
-- 
2.11.0


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1575317 — Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers

Em Tue, Feb 07, 2017 at 02:09:17AM +0100, Borislav Petkov escreveu:
> From: Borislav Petkov <bp@suse.de>
> 
> Add helper functions to be able to read/write ints into proc files.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  tools/lib/api/fs/fs.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tools/lib/api/fs/fs.h |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
> index 4b6bfc43cccf..8e4b9fe18b75 100644
> --- a/tools/lib/api/fs/fs.c
> +++ b/tools/lib/api/fs/fs.c
> @@ -314,6 +314,22 @@ int filename__read_int(const char *filename, int *value)
>  	return err;
>  }
>  
> +int filename__write_int(const char *filename, int value)
> +{
> +	char line[64];
> +	int fd = open(filename, O_WRONLY), err = -1;
> +
> +	if (fd < 0)
> +		return -1;
> +
> +	snprintf(line, sizeof(int), "%d", value);
> +
> +	err = write(fd, line, strnlen(line, 64));
> +
> +	close(fd);
> +	return err;
> +}
> +
>  /*
>   * Parses @value out of @filename with strtoull.
>   * By using 0 for base, the strtoull detects the
> @@ -400,6 +416,32 @@ int procfs__read_str(const char *entry, char **buf, size_t *sizep)
>  	return filename__read_str(path, buf, sizep);
>  }
>  
> +int procfs__read_int(const char *entry, int *value)
> +{
> +	char path[PATH_MAX];
> +	const char *procfs = procfs__mountpoint();
> +
> +	if (!procfs)
> +		return -1;
> +
> +	snprintf(path, sizeof(path), "%s/%s", procfs, entry);
> +
> +	return filename__read_int(path, value);
> +}
> +
> +int procfs__write_int(const char *entry, int value)
> +{
> +	char path[PATH_MAX];
> +	const char *procfs = procfs__mountpoint();
> +
> +	if (!procfs)
> +		return -1;
> +
> +	snprintf(path, sizeof(path), "%s/%s", procfs, entry);
> +
> +	return filename__write_int(path, value);
> +}
> +
>  int sysfs__read_ull(const char *entry, unsigned long long *value)
>  {
>  	char path[PATH_MAX];
> diff --git a/tools/lib/api/fs/fs.h b/tools/lib/api/fs/fs.h
> index 6b332dc74498..095f25d4c70f 100644
> --- a/tools/lib/api/fs/fs.h
> +++ b/tools/lib/api/fs/fs.h
> @@ -28,9 +28,12 @@ FS(bpf_fs)
>  
>  
>  int filename__read_int(const char *filename, int *value);
> +int filename__write_int(const char *filename, int value);
>  int filename__read_ull(const char *filename, unsigned long long *value);
>  int filename__read_str(const char *filename, char **buf, size_t *sizep);
>  
> +int procfs__read_int(const char *entry, int *value);
> +int procfs__write_int(const char *entry, int value);
>  int procfs__read_str(const char *entry, char **buf, size_t *sizep);
>  
>  int sysctl__read_int(const char *sysctl, int *value);

Isn't sysctl__read_int() what you want?

See next patch...

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

#1575552 — Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers

On Mon, Feb 06, 2017 at 10:43:56PM -0300, Arnaldo Carvalho de Melo wrote:
> >  int sysctl__read_int(const char *sysctl, int *value);
> 
> Isn't sysctl__read_int() what you want?

Right, so looking at this: don't you think that having both sysctl__*
and procfs__* is a little redundant?

The sysctl* things are doing the accesses over proc so shouldn't it all
be procfs__* interfaces and no sysctl__* ones at all

or

at least the sysctl__* ones should call the procfs__* ones?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1575784 — Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers

On Tue, Feb 07, 2017 at 12:00:45PM -0300, Arnaldo Carvalho de Melo wrote:
> And forget that under the hood this is in something called "procfs".

... unless one day you want to set a file in /proc which is not under
the sysctl hierarchy :-)

I mean, I don't care what I do - you're the maintainer - I'm just giving
you 2 cents :-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web