Re: [PATCH 2/3] x86, perf: Add a separate Arch Perfmon v4 PMI handler

[Liang, Kan]

On 8/6/2018 2:35 PM, Peter Zijlstra wrote:
> On Mon, Aug 06, 2018 at 10:23:42AM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
> > @@ -2044,6 +2056,14 @@ static void intel_pmu_disable_event(struct perf_event *event)
> >   	if (unlikely(event->attr.precise_ip))
> >   		intel_pmu_pebs_disable(event);
> >   
> > +	/*
> > +	 * We could disable freezing here, but doesn't hurt if it's on.
> > +	 * perf remembers the state, and someone else will likely
> > +	 * reinitialize.
> > +	 *
> > +	 * This avoids an extra MSR write in many situations.
> > +	 */
> > +
> >   	if (unlikely(hwc->config_base == MSR_ARCH_PERFMON_FIXED_CTR_CTRL)) {
> >   		intel_pmu_disable_fixed(hwc);
> >   		return;
> > @@ -2119,6 +2139,11 @@ static void intel_pmu_enable_event(struct perf_event *event)
> >   	if (event->attr.exclude_guest)
> >   		cpuc->intel_ctrl_host_mask |= (1ull << hwc->idx);
> >   
> > +	if (x86_pmu.counter_freezing && !cpuc->frozen_enabled) {
> > +		enable_counter_freeze();
> > +		cpuc->frozen_enabled = 1;
> > +	}
> > +
> >   	if (unlikely(event_is_checkpointed(event)))
> >   		cpuc->intel_cp_status |= (1ull << hwc->idx);
> >   
>
> Why here? That doesn't really make sense; should this not be in
> intel_pmu_cpu_starting() or something?


For Goldmont Plus, the counter freezing feature can be re-enabled at 
run-time by loading a newer microcode.
We need to check the x86_pmu.counter_freezing every time.


> > +static bool disable_counter_freezing;
> > +module_param(disable_counter_freezing, bool, 0444);
> > +MODULE_PARM_DESC(disable_counter_freezing, "Disable counter freezing feature."
> > +		"The PMI handler will fall back to generic handler."
> > +		"Default is false (enable counter freezing feature).");
>
> Why?
>
> > +	/*
> > +	 * Ack the PMU late after the APIC.  This avoids bogus
>
> That doesn't make sense. PMU and APIC do not have order.

The moment FROZEN bit is cleared, counters start counting immediately.
Here, we try to make it as close as possible to IRET.


Thanks,
Kan