Re: [PATCH 4/5] perf metricgroup: Support metric constraint
From: Liang, Kan
Date: Fri Feb 21 2020 - 10:42:18 EST
On 2/21/2020 9:48 AM, Jiri Olsa wrote:
On Fri, Feb 21, 2020 at 09:30:15AM -0500, Liang, Kan wrote:
On 2/21/2020 8:09 AM, Jiri Olsa wrote:
On Thu, Feb 20, 2020 at 11:14:09AM -0500, Liang, Kan wrote:
On 2/20/2020 6:35 AM, Jiri Olsa wrote:
On Wed, Feb 19, 2020 at 11:08:39AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
SNIP
+static bool violate_nmi_constraint;
+
+static bool metricgroup__has_constraint(struct pmu_event *pe)
+{
+ if (!pe->metric_constraint)
+ return false;
+
+ if (!strcmp(pe->metric_constraint, "NO_NMI_WATCHDOG") &&
+ sysctl__nmi_watchdog_enabled()) {
+ pr_warning("Splitting metric group %s into standalone metrics.\n",
+ pe->metric_name);
+ violate_nmi_constraint = true;
no static flags plz.. can't you just print that rest of the warning in here?
Because we only want to print the NMI watchdog warning once.
If there are more than one metric groups with constraint, the warning may be
printed several times. For example,
$ perf stat -M Page_Walks_Utilization,Page_Walks_Utilization
Splitting metric group Page_Walks_Utilization into standalone metrics.
Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG metric
constraint:
echo 0 > /proc/sys/kernel/nmi_watchdog
perf stat ...
echo 1 > /proc/sys/kernel/nmi_watchdog
Splitting metric group Page_Walks_Utilization into standalone metrics.
Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG metric
constraint:
echo 0 > /proc/sys/kernel/nmi_watchdog
perf stat ...
echo 1 > /proc/sys/kernel/nmi_watchdog
Is it OK?
If it's OK, I think we can remove the flag.
we use the 'print once' static flags in functions,
so plz keep it inside like WARN_ONCE, or use it directly
If using WARN_ONCE, the warning is always printed for the first violation.
For example,
#perf stat -M Page_Walks_Utilization,Page_Walks_Utilization
Splitting metric group Page_Walks_Utilization into standalone metrics.
Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG metric constraint:
echo 0 > /proc/sys/kernel/nmi_watchdog
perf stat ...
echo 1 > /proc/sys/kernel/nmi_watchdog
Splitting metric group Page_Walks_Utilization into standalone metrics.
The output of current patch is as below.
#perf stat -M Page_Walks_Utilization,Page_Walks_Utilization
Splitting metric group Page_Walks_Utilization into standalone metrics.
Splitting metric group Page_Walks_Utilization into standalone metrics.
Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG metric constraint:
echo 0 > /proc/sys/kernel/nmi_watchdog
perf stat ...
echo 1 > /proc/sys/kernel/nmi_watchdog
Personally, I think the output of current patch looks better.
But there is nothing wrong with the output of WARN_ONCE.
Should I use WARN_ONCE in next V2?
I just wanted you to keep that static flag inside the function,
so we don't have another static variable used across the code
if the WARN_ONCE does not fit, just use your own flag inside
the function
Here is the current code flow. The metric groups are processed one by
one. When perf tool processes the metric group in
metricgroup__has_constraint(), we have no idea whether the following
groups break the constraint.
metricgroup__parse_groups():
metricgroup__add_metric_list():
while ((p = strsep(&llist, ",")) != NULL) {
metricgroup__add_metric():
metricgroup__has_constraint()
}
The watchdog hint has to be printed after all metric groups are processed.
What about the patch as below?
A dedicated function for warnings is introduced. But it has to be
invoked twice. One is in the middle of processing. The other is after
all metric groups are processed.
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index f9a9b50..c3a8c70 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -441,7 +441,24 @@ static void
metricgroup__add_metric_non_group(struct strbuf *events,
strbuf_addf(events, ",%s", ids[i]);
}
-static bool violate_nmi_constraint;
+static void metricgroup___watchdog_constraint_hint(const char *name,
bool foot)
+{
+ static bool violate_nmi_constraint;
+
+ if (!foot) {
+ pr_warning("Splitting metric group %s into standalone metrics.\n", name);
+ violate_nmi_constraint = true;
+ return;
+ }
+
+ if (!violate_nmi_constraint)
+ return;
+
+ pr_warning("Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG
metric constraint:\n"
+ " echo 0 > /proc/sys/kernel/nmi_watchdog\n"
+ " perf stat ...\n"
+ " echo 1 > /proc/sys/kernel/nmi_watchdog\n");
+}
static bool metricgroup__has_constraint(struct pmu_event *pe)
{
@@ -450,9 +467,7 @@ static bool metricgroup__has_constraint(struct
pmu_event *pe)
if (!strcmp(pe->metric_constraint, "NO_NMI_WATCHDOG") &&
sysctl__nmi_watchdog_enabled()) {
- pr_warning("Splitting metric group %s into standalone metrics.\n",
- pe->metric_name);
- violate_nmi_constraint = true;
+ metricgroup___watchdog_constraint_hint(pe->metric_name, false);
return true;
}
@@ -535,6 +550,10 @@ static int metricgroup__add_metric_list(const char
*list, struct strbuf *events,
}
}
free(nlist);
+
+ if (!ret)
+ metricgroup___watchdog_constraint_hint(NULL, true);
+
return ret;
}
@@ -577,13 +596,6 @@ int metricgroup__parse_groups(const struct option *opt,
strbuf_release(&extra_events);
ret = metricgroup__setup_events(&group_list, perf_evlist,
metric_events);
-
- if (violate_nmi_constraint) {
- pr_warning("Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG
metric constraint:\n"
- " echo 0 > /proc/sys/kernel/nmi_watchdog\n"
- " perf stat ...\n"
- " echo 1 > /proc/sys/kernel/nmi_watchdog\n");
- }
out:
metricgroup__free_egroups(&group_list);
return ret;
Thanks,
Kan