Re: [PATCH 4/5] perf metricgroup: Support metric constraint

["Liang, Kan" <kan.liang@linux.intel.com>]

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