| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <42831850-9969-435f-6713-7bf82b628d29@linux.intel.com>
Date: Fri, 21 Feb 2020 10:42:13 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: acme@...nel.org, mingo@...hat.com, peterz@...radead.org,
linux-kernel@...r.kernel.org, mark.rutland@....com,
namhyung@...nel.org, ravi.bangoria@...ux.ibm.com,
yao.jin@...ux.intel.com, ak@...ux.intel.com
Subject: Re: [PATCH 4/5] perf metricgroup: Support metric constraint
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@...ux.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
Powered by blists - more mailing lists