Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1186773 > unrolled thread

Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support Mark Rutland <mark.rutland@arm.com> - 2015-07-17 14:20 +0200
    Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support Peter Zijlstra <peterz@infradead.org> - 2015-07-17 15:50 +0200
      Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support Peter Zijlstra <peterz@infradead.org> - 2015-07-17 16:00 +0200

#1186773 — Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support

On Thu, Jul 16, 2015 at 09:33:44PM +0100, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> This patch implements core_misc PMU disable and enable functions.
> core_misc PMU counters are free running counters, so it's impossible to
> stop/start them.

Doesn't that effectively mean you can't group them? You'll get arbitrary
noise because counters will be incrementing as you read them.

[...]

> @@ -927,6 +933,10 @@ int p6_pmu_init(void);
>  
>  int knc_pmu_init(void);
>  
> +void intel_core_misc_pmu_enable(void);
> +
> +void intel_core_misc_pmu_disable(void);
> +
>  ssize_t events_sysfs_show(struct device *dev, struct device_attribute *attr,
>  			  char *page);
>  
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index b9826a9..651a86d 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1586,6 +1586,8 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
>  	if (!x86_pmu.late_ack)
>  		apic_write(APIC_LVTPC, APIC_DM_NMI);
>  	__intel_pmu_disable_all();
> +	if (cpuc->core_misc_active_mask)
> +		intel_core_misc_pmu_disable();

Huh? Free running counters have nothing to do with the PMU interrupt;
there's nothing they can do to trigger it. This feels very hacky.

If this is necessary, surely it should live in __intel_pmu_disable_all?

[...]

> +void intel_core_misc_pmu_enable(void)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	struct perf_event *event;
> +	u64 start;
> +	int bit;
> +
> +	for_each_set_bit(bit, cpuc->core_misc_active_mask,
> +			 X86_CORE_MISC_COUNTER_MAX) {
> +		event = cpuc->core_misc_events[bit];
> +		start = core_misc_pmu_read_counter(event);
> +		local64_set(&event->hw.prev_count, start);
> +	}
> +}
> +
> +void intel_core_misc_pmu_disable(void)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	int bit;
> +
> +	for_each_set_bit(bit, cpuc->core_misc_active_mask,
> +			 X86_CORE_MISC_COUNTER_MAX) {
> +		core_misc_pmu_event_update(cpuc->core_misc_events[bit]);
> +	}
> +}
> +
>  static void core_misc_pmu_event_del(struct perf_event *event, int mode)
>  {
>  	core_misc_pmu_event_stop(event, PERF_EF_UPDATE);
> @@ -863,6 +899,11 @@ static void __init core_misc_pmus_register(void)
>  			.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
>  		};
>  
> +		if (type->type == perf_intel_core_misc_thread) {
> +			type->pmu.pmu_disable = (void *) intel_core_misc_pmu_disable;
> +			type->pmu.pmu_enable = (void *) intel_core_misc_pmu_enable;

Why are you suprressing an entirely valid compiler warning here?

The signatures of intel_core_misc_pmu_{enable,disable} aren't right. Fix
them to take a struct pmu *.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1186841

On Fri, Jul 17, 2015 at 01:11:41PM +0100, Mark Rutland wrote:
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> > index b9826a9..651a86d 100644
> > --- a/arch/x86/kernel/cpu/perf_event_intel.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> > @@ -1586,6 +1586,8 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
> >  	if (!x86_pmu.late_ack)
> >  		apic_write(APIC_LVTPC, APIC_DM_NMI);
> >  	__intel_pmu_disable_all();
> > +	if (cpuc->core_misc_active_mask)
> > +		intel_core_misc_pmu_disable();
> 
> Huh? Free running counters have nothing to do with the PMU interrupt;
> there's nothing they can do to trigger it. This feels very hacky.
> 
> If this is necessary, surely it should live in __intel_pmu_disable_all?
> 
> [...]

Yeah this is crazy. It should not live in the regular PMU at all, not be
Intel specific.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186848

On Fri, Jul 17, 2015 at 03:46:29PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 17, 2015 at 01:11:41PM +0100, Mark Rutland wrote:
> > > diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> > > index b9826a9..651a86d 100644
> > > --- a/arch/x86/kernel/cpu/perf_event_intel.c
> > > +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> > > @@ -1586,6 +1586,8 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
> > >  	if (!x86_pmu.late_ack)
> > >  		apic_write(APIC_LVTPC, APIC_DM_NMI);
> > >  	__intel_pmu_disable_all();
> > > +	if (cpuc->core_misc_active_mask)
> > > +		intel_core_misc_pmu_disable();
> > 
> > Huh? Free running counters have nothing to do with the PMU interrupt;
> > there's nothing they can do to trigger it. This feels very hacky.
> > 
> > If this is necessary, surely it should live in __intel_pmu_disable_all?
> > 
> > [...]
> 
> Yeah this is crazy. It should not live in the regular PMU at all, not be
> Intel specific.

lkml.kernel.org/r/2c37309d20afadf88ad4a82cf0ce02b9152801e2.1430256154.git.luto@kernel.org

That does the right thing for free running MSRs.

Take it and expand.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web