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

From: Jiri Olsa
Date: Fri Feb 21 2020 - 09:48:20 EST


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

jirka