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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <300208e8-2526-8f17-a28a-d4e244baaf90@linux.intel.com>
Date:   Fri, 21 Feb 2020 09:30:15 -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 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?

Thanks,
Kan