Messages in this thread Patch in this message | | | Subject | Re: [PATCH 4/5] perf metricgroup: Support metric constraint | From | "Liang, Kan" <> | Date | Fri, 21 Feb 2020 10:42:13 -0500 |
| |
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@linux.intel.com 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
| |