Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1574620 > unrolled thread
| Started by | Borislav Petkov <bp@alien8.de> |
|---|---|
| First post | 2017-02-06 13:20 +0100 |
| Last post | 2017-02-06 13:50 +0100 |
| Articles | 5 on this page of 25 — 5 participants |
Back to article view | Back to linux.kernel
[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 2 of 2 — ← Prev page 1 [2]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-02-07 16:40 +0100 |
| Subject | Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers |
| Message-ID | <t8g25-4G5-7@gated-at.bofh.it> |
| In reply to | #1575784 |
Em Tue, Feb 07, 2017 at 04:08:02PM +0100, Borislav Petkov escreveu: > 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 :-) In that case you use procfs__read_int(), no problem. > I mean, I don't care what I do - you're the maintainer - I'm just giving > you 2 cents :-) :-) - Arnaldo
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-02-07 16:10 +0100 |
| Subject | Re: [PATCH 1/2] tools/lib/api/fs: Add procfs int read/write helpers |
| Message-ID | <t8fz3-4vL-3@gated-at.bofh.it> |
| In reply to | #1575552 |
Em Tue, Feb 07, 2017 at 11:30:01AM +0100, Borislav Petkov escreveu:
> 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?
Well, I see this as: sysctls are implemented as files in procfs, but
could conceivably be implemented somewhere else, just like the events
file was implemented in debugfs but then was moved to a separate
filesystem type, tracefs.
So it being in procfs is an implementation detail, what I'm interested
are sysctls, and names for sysctls will be appended to wherever the
sysctl is made available in the file system.
For instance:
[root@jouet ~]# sysctl kernel.watchdog
kernel.watchdog = 1
[root@jouet ~]#
So the sysctl is "kernel.watchdog" and it is set to one.
So, I suggest you use:
if (sysctl__read_int("kernel/watchdog", &watchdog_value) == 0)
/* do whatever you want with it */
And forget that under the hood this is in something called "procfs".
:-)
- Arnaldo
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-02-07 02:20 +0100 |
| Subject | [PATCH 2/2] perf stat: Disable HW watchdog around a perf stat session |
| Message-ID | <t82BQ-4jX-3@gated-at.bofh.it> |
| In reply to | #1575302 |
From: Borislav Petkov <bp@suse.de>
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 consume 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 let's disable the HW watchdog around a perf stat session running as
root and restore it after it to its previous state. This frees up the
one counter and the scheduling of the default events succeeds:
Performance counter stats for 'sleep 1':
0.806902 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.068 M/sec
1,200,677 cycles # 1.488 GHz
308,044 stalled-cycles-frontend # 25.66% frontend cycles idle
424,292 stalled-cycles-backend # 35.34% backend cycles idle
672,694 instructions # 0.56 insn per cycle
# 0.63 stalled cycles per insn
132,965 branches # 164.785 M/sec
7,300 branch-misses # 5.49% of all branches
1.001689739 seconds time elapsed
There's a --dont-disable-hwdt option which preserves the old behavior.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
tools/perf/builtin-stat.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e965628..b2aa2ed3161c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -146,6 +146,8 @@ static aggr_get_id_t aggr_get_id;
static bool append_file;
static const char *output_name;
static int output_fd;
+static bool keep_hwdt;
+static int prev_hwdt; /* previous HW watchdog state */
struct perf_stat {
bool record;
@@ -1539,6 +1541,39 @@ static void print_counters(struct timespec *ts, int argc, const char **argv)
fflush(stat_config.output);
}
+static void perf_stat_toggle_hwdt(int on)
+{
+ static const char *p = "sys/kernel/nmi_watchdog";
+ int val;
+
+ if (keep_hwdt)
+ return;
+
+ if (geteuid())
+ return;
+
+ if (procfs__read_int(p, &val) < 0)
+ return;
+
+ /* Reenable only when it was enabled before. */
+ if (on) {
+ if (prev_hwdt)
+ goto write;
+ /* Disable HWDT only when it is enabled. */
+ } else {
+ prev_hwdt = val;
+
+ if (val)
+ goto write;
+ }
+
+ return;
+
+write:
+ if (procfs__write_int(p, on) < 0)
+ return;
+}
+
static volatile int signr = -1;
static void skip_signal(int signo)
@@ -1575,6 +1610,8 @@ static void sig_atexit(void)
sigprocmask(SIG_SETMASK, &oset, NULL);
+ perf_stat_toggle_hwdt(1);
+
if (signr == -1)
return;
@@ -1659,6 +1696,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, "dont-disable-hwdt", &keep_hwdt,
+ "Do not disable HW NMI watchdog during the current session"),
OPT_END()
};
@@ -2523,6 +2562,8 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
if (perf_stat_init_aggr_mode())
goto out;
+ perf_stat_toggle_hwdt(0);
+
/*
* We dont want to block the signals - that would cause
* child tasks to inherit that and Ctrl-C would not work.
--
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-02-07 02:50 +0100 |
| Subject | Re: [PATCH 2/2] perf stat: Disable HW watchdog around a perf stat session |
| Message-ID | <t834S-4uf-9@gated-at.bofh.it> |
| In reply to | #1575308 |
Em Tue, Feb 07, 2017 at 02:10:28AM +0100, Borislav Petkov escreveu:
> From: Borislav Petkov <bp@suse.de>
>
> 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 consume 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 let's disable the HW watchdog around a perf stat session running as
> root and restore it after it to its previous state. This frees up the
> one counter and the scheduling of the default events succeeds:
>
> Performance counter stats for 'sleep 1':
>
> 0.806902 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.068 M/sec
> 1,200,677 cycles # 1.488 GHz
> 308,044 stalled-cycles-frontend # 25.66% frontend cycles idle
> 424,292 stalled-cycles-backend # 35.34% backend cycles idle
> 672,694 instructions # 0.56 insn per cycle
> # 0.63 stalled cycles per insn
> 132,965 branches # 164.785 M/sec
> 7,300 branch-misses # 5.49% of all branches
>
> 1.001689739 seconds time elapsed
>
> There's a --dont-disable-hwdt option which preserves the old behavior.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> tools/perf/builtin-stat.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index a02f2e965628..b2aa2ed3161c 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -146,6 +146,8 @@ static aggr_get_id_t aggr_get_id;
> static bool append_file;
> static const char *output_name;
> static int output_fd;
> +static bool keep_hwdt;
> +static int prev_hwdt; /* previous HW watchdog state */
>
> struct perf_stat {
> bool record;
> @@ -1539,6 +1541,39 @@ static void print_counters(struct timespec *ts, int argc, const char **argv)
> fflush(stat_config.output);
> }
>
> +static void perf_stat_toggle_hwdt(int on)
> +{
> + static const char *p = "sys/kernel/nmi_watchdog";
static const char *p = "kernel/nmi_watchdog";
> + int val;
> +
> + if (keep_hwdt)
> + return;
> +
> + if (geteuid())
> + return;
> +
> + if (procfs__read_int(p, &val) < 0)
sysctl__read_int(p, &val) < 0)
> + return;
> +
> + /* Reenable only when it was enabled before. */
> + if (on) {
> + if (prev_hwdt)
> + goto write;
> + /* Disable HWDT only when it is enabled. */
> + } else {
> + prev_hwdt = val;
> +
> + if (val)
> + goto write;
> + }
> +
> + return;
> +
> +write:
> + if (procfs__write_int(p, on) < 0)
> + return;
> +}
> +
> static volatile int signr = -1;
>
> static void skip_signal(int signo)
> @@ -1575,6 +1610,8 @@ static void sig_atexit(void)
>
> sigprocmask(SIG_SETMASK, &oset, NULL);
>
> + perf_stat_toggle_hwdt(1);
> +
> if (signr == -1)
> return;
>
> @@ -1659,6 +1696,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, "dont-disable-hwdt", &keep_hwdt,
> + "Do not disable HW NMI watchdog during the current session"),
> OPT_END()
> };
>
> @@ -2523,6 +2562,8 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
> if (perf_stat_init_aggr_mode())
> goto out;
>
> + perf_stat_toggle_hwdt(0);
> +
> /*
> * We dont want to block the signals - that would cause
> * child tasks to inherit that and Ctrl-C would not work.
> --
> 2.11.0
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-02-06 13:50 +0100 |
| Message-ID | <t7QU1-4ZM-5@gated-at.bofh.it> |
| In reply to | #1574636 |
* Borislav Petkov <bp@alien8.de> wrote: > 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? 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. :-/ Thanks, Ingo
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web