[PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

This patch adds support for building Intel uncore driver as module.
It adds clean-up code and config option for the Intel uncore driver

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
Changes since v1:
 move config option to "General setup/Kernel Performance Events and Counters"

 arch/x86/kernel/cpu/Makefile                  |  3 +-
 arch/x86/kernel/cpu/perf_event_intel_uncore.c | 48 ++++++++++++++++++++++++---
 init/Kconfig                                  | 10 ++++++
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 7fd54f0..5d3da70 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -36,7 +36,8 @@ obj-$(CONFIG_CPU_SUP_AMD)		+= perf_event_amd_iommu.o
 endif
 obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_p6.o perf_event_knc.o perf_event_p4.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_lbr.o perf_event_intel_ds.o perf_event_intel.o
-obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_uncore.o perf_event_intel_rapl.o
+obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_rapl.o
+obj-$(CONFIG_PERF_X86_INTEL_UNCORE)	+= perf_event_intel_uncore.o
 endif
 
 
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
index 618d502..fd5e883 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -3643,6 +3643,7 @@ static int __init uncore_pmu_register(struct intel_uncore_pmu *pmu)
 
 	if (!pmu->type->pmu) {
 		pmu->pmu = (struct pmu) {
+			.module		= THIS_MODULE,
 			.attr_groups	= pmu->type->attr_groups,
 			.task_ctx_nr	= perf_invalid_context,
 			.event_init	= uncore_pmu_event_init,
@@ -3654,6 +3655,7 @@ static int __init uncore_pmu_register(struct intel_uncore_pmu *pmu)
 		};
 	} else {
 		pmu->pmu = *pmu->type->pmu;
+		pmu->pmu.module = THIS_MODULE;
 		pmu->pmu.attr_groups = pmu->type->attr_groups;
 	}
 
@@ -3671,7 +3673,7 @@ static int __init uncore_pmu_register(struct intel_uncore_pmu *pmu)
 	return ret;
 }
 
-static void __init uncore_type_exit(struct intel_uncore_type *type)
+static void uncore_type_exit(struct intel_uncore_type *type)
 {
 	int i;
 
@@ -3683,7 +3685,7 @@ static void __init uncore_type_exit(struct intel_uncore_type *type)
 	type->events_group = NULL;
 }
 
-static void __init uncore_types_exit(struct intel_uncore_type **types)
+static void uncore_types_exit(struct intel_uncore_type **types)
 {
 	int i;
 	for (i = 0; types[i]; i++)
@@ -3910,7 +3912,7 @@ static int __init uncore_pci_init(void)
 	return ret;
 }
 
-static void __init uncore_pci_exit(void)
+static void uncore_pci_exit(void)
 {
 	if (pcidrv_registered) {
 		pcidrv_registered = false;
@@ -4293,4 +4295,42 @@ static int __init intel_uncore_init(void)
 fail:
 	return ret;
 }
-device_initcall(intel_uncore_init);
+
+static void __exit uncore_pmus_unregister(void)
+{
+	struct intel_uncore_pmu *pmu;
+	struct intel_uncore_type *type;
+	int i, j;
+
+	for (i = 0; msr_uncores[i]; i++) {
+		type = msr_uncores[i];
+		for (j = 0; j < type->num_boxes; j++) {
+			pmu = &type->pmus[j];
+			perf_pmu_unregister(&pmu->pmu);
+		}
+	}
+
+	for (i = 0; pci_uncores[i]; i++) {
+		type = pci_uncores[i];
+		for (j = 0; j < type->num_boxes; j++) {
+			pmu = &type->pmus[j];
+			perf_pmu_unregister(&pmu->pmu);
+		}
+	}
+}
+
+static void __exit intel_uncore_exit(void)
+{
+	uncore_pmus_unregister();
+
+	unregister_cpu_notifier(&uncore_cpu_nb);
+
+	uncore_pci_exit();
+
+	uncore_types_exit(msr_uncores);
+}
+
+module_init(intel_uncore_init);
+module_exit(intel_uncore_exit);
+MODULE_DESCRIPTION("Intel Uncore Performance Monitoring driver");
+MODULE_LICENSE("GPL");
diff --git a/init/Kconfig b/init/Kconfig
index 009a797..751e06c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1502,6 +1502,16 @@ config PERF_EVENTS
 
 	  Say Y if unsure.
 
+config PERF_X86_INTEL_UNCORE
+	default y
+	tristate "Intel uncore performance monitoring support"
+	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
+	help
+	  This option allows kernel to access the Uncore performance
+	  monitoring units of Intel processors.
+
+	  Say Y if unsure.
+
 config DEBUG_PERF_USE_VMALLOC
 	default n
 	bool "Debug: use vmalloc to back perf mmap() buffers"
-- 
1.8.5.3

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Peter Zijlstra]

On Thu, Mar 20, 2014 at 02:03:46PM +0800, Yan, Zheng wrote:
> This patch adds support for building Intel uncore driver as module.
> It adds clean-up code and config option for the Intel uncore driver
> 
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> Changes since v1:
>  move config option to "General setup/Kernel Performance Events and Counters"

Thanks!

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Ingo Molnar]

* Yan, Zheng <zheng.z.yan@intel.com> wrote:

> This patch adds support for building Intel uncore driver as module.
> It adds clean-up code and config option for the Intel uncore driver
> 
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> Changes since v1:
>  move config option to "General setup/Kernel Performance Events and Counters"
> 
>  arch/x86/kernel/cpu/Makefile                  |  3 +-
>  arch/x86/kernel/cpu/perf_event_intel_uncore.c | 48 ++++++++++++++++++++++++---
>  init/Kconfig                                  | 10 ++++++
>  3 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 7fd54f0..5d3da70 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -36,7 +36,8 @@ obj-$(CONFIG_CPU_SUP_AMD)		+= perf_event_amd_iommu.o
>  endif
>  obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_p6.o perf_event_knc.o perf_event_p4.o
>  obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_lbr.o perf_event_intel_ds.o perf_event_intel.o
> -obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_uncore.o perf_event_intel_rapl.o
> +obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_rapl.o
> +obj-$(CONFIG_PERF_X86_INTEL_UNCORE)	+= perf_event_intel_uncore.o
>  endif
>  
>  
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> index 618d502..fd5e883 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c

So I'm not willing to apply this patch until the documentation of 
perf_event_intel_uncore.c is improved. Right now the file starts 
without a single comment (!). Just lines after lines of code, without 
any explanation what it does, what its scope is, etc.

How should even a knowledgable user know about what it's all about?

That problem percolates to the Kconfig entry as well:

> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1502,6 +1502,16 @@ config PERF_EVENTS
>  
>  	  Say Y if unsure.
>  
> +config PERF_X86_INTEL_UNCORE
> +	default y
> +	tristate "Intel uncore performance monitoring support"
> +	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
> +	help
> +	  This option allows kernel to access the Uncore performance
> +	  monitoring units of Intel processors.
> +
> +	  Say Y if unsure.

Firstly, it should probably not be 'default y'.

Secondly, the description is essentially non-existent. What does the 
hardware do? Why should users care about uncore PMUs?

This needs to be fixed before we can make this modular.

Thanks,

	Ingo

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Andi Kleen]

> So I'm not willing to apply this patch until the documentation of 
> perf_event_intel_uncore.c is improved. Right now the file starts 
> without a single comment (!). Just lines after lines of code, without 
> any explanation what it does, what its scope is, etc.
> 
> How should even a knowledgable user know about what it's all about?

While I agree with you that the lack of documentation is bad,
I want to point out that the rest of perf (except for some recent
code from Stephane) doesn't have any such file comments and useful
Kconfig descriptions either.

It seems unfair and inconsistent to require contributors to follow rules that
is not followed by the original code.

-Andi

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

On 04/18/2014 06:53 PM, Ingo Molnar wrote:
> 
> * Yan, Zheng <zheng.z.yan@intel.com> wrote:
> 
>> This patch adds support for building Intel uncore driver as module.
>> It adds clean-up code and config option for the Intel uncore driver
>>
>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>> ---
>> Changes since v1:
>>  move config option to "General setup/Kernel Performance Events and Counters"
>>
>>  arch/x86/kernel/cpu/Makefile                  |  3 +-
>>  arch/x86/kernel/cpu/perf_event_intel_uncore.c | 48 ++++++++++++++++++++++++---
>>  init/Kconfig                                  | 10 ++++++
>>  3 files changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> index 7fd54f0..5d3da70 100644
>> --- a/arch/x86/kernel/cpu/Makefile
>> +++ b/arch/x86/kernel/cpu/Makefile
>> @@ -36,7 +36,8 @@ obj-$(CONFIG_CPU_SUP_AMD)		+= perf_event_amd_iommu.o
>>  endif
>>  obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_p6.o perf_event_knc.o perf_event_p4.o
>>  obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_lbr.o perf_event_intel_ds.o perf_event_intel.o
>> -obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_uncore.o perf_event_intel_rapl.o
>> +obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_rapl.o
>> +obj-$(CONFIG_PERF_X86_INTEL_UNCORE)	+= perf_event_intel_uncore.o
>>  endif
>>  
>>  
>> diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
>> index 618d502..fd5e883 100644
>> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
>> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> 
> So I'm not willing to apply this patch until the documentation of 
> perf_event_intel_uncore.c is improved. Right now the file starts 
> without a single comment (!). Just lines after lines of code, without 
> any explanation what it does, what its scope is, etc.

there are comments mark the scope of code for different CPU.

> 
> How should even a knowledgable user know about what it's all about?
> 
> That problem percolates to the Kconfig entry as well:
> 

Most of the codes without comments are hardware specific codes. The corresponding
contents in Intel uncore documents are big tables/lists, nothing tricky/interesting.
I really don't know how to comment these code.

Regards
Yan, Zheng
 

>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1502,6 +1502,16 @@ config PERF_EVENTS
>>  
>>  	  Say Y if unsure.
>>  
>> +config PERF_X86_INTEL_UNCORE
>> +	default y
>> +	tristate "Intel uncore performance monitoring support"
>> +	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
>> +	help
>> +	  This option allows kernel to access the Uncore performance
>> +	  monitoring units of Intel processors.
>> +
>> +	  Say Y if unsure.
> 
> Firstly, it should probably not be 'default y'.
> 
> Secondly, the description is essentially non-existent. What does the 
> hardware do? Why should users care about uncore PMUs?
> 
> This needs to be fixed before we can make this modular.
> 
> Thanks,
> 
> 	Ingo
> 

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Ingo Molnar]

* Yan, Zheng <zheng.z.yan@intel.com> wrote:

> On 04/18/2014 06:53 PM, Ingo Molnar wrote:
> > 
> > * Yan, Zheng <zheng.z.yan@intel.com> wrote:
> > 
> >> This patch adds support for building Intel uncore driver as module.
> >> It adds clean-up code and config option for the Intel uncore driver
> >>
> >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> >> ---
> >> Changes since v1:
> >>  move config option to "General setup/Kernel Performance Events and Counters"
> >>
> >>  arch/x86/kernel/cpu/Makefile                  |  3 +-
> >>  arch/x86/kernel/cpu/perf_event_intel_uncore.c | 48 ++++++++++++++++++++++++---
> >>  init/Kconfig                                  | 10 ++++++
> >>  3 files changed, 56 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> >> index 7fd54f0..5d3da70 100644
> >> --- a/arch/x86/kernel/cpu/Makefile
> >> +++ b/arch/x86/kernel/cpu/Makefile
> >> @@ -36,7 +36,8 @@ obj-$(CONFIG_CPU_SUP_AMD)		+= perf_event_amd_iommu.o
> >>  endif
> >>  obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_p6.o perf_event_knc.o perf_event_p4.o
> >>  obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_lbr.o perf_event_intel_ds.o perf_event_intel.o
> >> -obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_uncore.o perf_event_intel_rapl.o
> >> +obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_rapl.o
> >> +obj-$(CONFIG_PERF_X86_INTEL_UNCORE)	+= perf_event_intel_uncore.o
> >>  endif
> >>  
> >>  
> >> diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> >> index 618d502..fd5e883 100644
> >> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> >> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> > 
> > So I'm not willing to apply this patch until the documentation of 
> > perf_event_intel_uncore.c is improved. Right now the file starts 
> > without a single comment (!). Just lines after lines of code, without 
> > any explanation what it does, what its scope is, etc.
> 
> there are comments mark the scope of code for different CPU.
> 
> > 
> > How should even a knowledgable user know about what it's all about?
> > 
> > That problem percolates to the Kconfig entry as well:
> > 
> 
> Most of the codes without comments are hardware specific codes. The 
> corresponding contents in Intel uncore documents are big 
> tables/lists, nothing tricky/interesting. I really don't know how to 
> comment these code.

Have a look at other PMU drivers, such as 
arch/x86/kernel/cpu/perf_event_intel_rapl.c, which begin with a 
general explanation attached below.

Thanks,

	Ingo


/*
 * perf_event_intel_rapl.c: support Intel RAPL energy consumption counters
 * Copyright (C) 2013 Google, Inc., Stephane Eranian
 *
 * Intel RAPL interface is specified in the IA-32 Manual Vol3b
 * section 14.7.1 (September 2013)
 *
 * RAPL provides more controls than just reporting energy consumption
 * however here we only expose the 3 energy consumption free running
 * counters (pp0, pkg, dram).
 *
 * Each of those counters increments in a power unit defined by the
 * RAPL_POWER_UNIT MSR. On SandyBridge, this unit is 1/(2^16) Joules
 * but it can vary.
 *
 * Counter to rapl events mappings:
 *
 *  pp0 counter: consumption of all physical cores (power plane 0)
 * 	  event: rapl_energy_cores
 *    perf code: 0x1
 *
 *  pkg counter: consumption of the whole processor package
 *	  event: rapl_energy_pkg
 *    perf code: 0x2
 *
 * dram counter: consumption of the dram domain (servers only)
 *	  event: rapl_energy_dram
 *    perf code: 0x3
 *
 * dram counter: consumption of the builtin-gpu domain (client only)
 *	  event: rapl_energy_gpu
 *    perf code: 0x4
 *
 * We manage those counters as free running (read-only). They may be
 * use simultaneously by other tools, such as turbostat.
 *
 * The events only support system-wide mode counting. There is no
 * sampling support because it does not make sense and is not
 * supported by the RAPL hardware.
 *
 * Because we want to avoid floating-point operations in the kernel,
 * the events are all reported in fixed point arithmetic (32.32).
 * Tools must adjust the counts to convert them to Watts using
 * the duration of the measurement. Tools may use a function such as
 * ldexp(raw_count, -32);
 */

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Stephane Eranian]

On Tue, Apr 22, 2014 at 1:35 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Yan, Zheng <zheng.z.yan@intel.com> wrote:
>
>> On 04/18/2014 06:53 PM, Ingo Molnar wrote:
>> >
>> > * Yan, Zheng <zheng.z.yan@intel.com> wrote:
>> >
>> >> This patch adds support for building Intel uncore driver as module.
>> >> It adds clean-up code and config option for the Intel uncore driver
>> >>
>> >> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>> >> ---
>> >> Changes since v1:
>> >>  move config option to "General setup/Kernel Performance Events and Counters"
>> >>
>> >>  arch/x86/kernel/cpu/Makefile                  |  3 +-
>> >>  arch/x86/kernel/cpu/perf_event_intel_uncore.c | 48 ++++++++++++++++++++++++---
>> >>  init/Kconfig                                  | 10 ++++++
>> >>  3 files changed, 56 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> >> index 7fd54f0..5d3da70 100644
>> >> --- a/arch/x86/kernel/cpu/Makefile
>> >> +++ b/arch/x86/kernel/cpu/Makefile
>> >> @@ -36,7 +36,8 @@ obj-$(CONFIG_CPU_SUP_AMD)                += perf_event_amd_iommu.o
>> >>  endif
>> >>  obj-$(CONFIG_CPU_SUP_INTEL)               += perf_event_p6.o perf_event_knc.o perf_event_p4.o
>> >>  obj-$(CONFIG_CPU_SUP_INTEL)               += perf_event_intel_lbr.o perf_event_intel_ds.o perf_event_intel.o
>> >> -obj-$(CONFIG_CPU_SUP_INTEL)               += perf_event_intel_uncore.o perf_event_intel_rapl.o
>> >> +obj-$(CONFIG_CPU_SUP_INTEL)               += perf_event_intel_rapl.o
>> >> +obj-$(CONFIG_PERF_X86_INTEL_UNCORE)       += perf_event_intel_uncore.o
>> >>  endif
>> >>
>> >>
>> >> diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
>> >> index 618d502..fd5e883 100644
>> >> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
>> >> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
>> >
>> > So I'm not willing to apply this patch until the documentation of
>> > perf_event_intel_uncore.c is improved. Right now the file starts
>> > without a single comment (!). Just lines after lines of code, without
>> > any explanation what it does, what its scope is, etc.
>>
>> there are comments mark the scope of code for different CPU.
>>
>> >
>> > How should even a knowledgable user know about what it's all about?
>> >
>> > That problem percolates to the Kconfig entry as well:
>> >
>>
>> Most of the codes without comments are hardware specific codes. The
>> corresponding contents in Intel uncore documents are big
>> tables/lists, nothing tricky/interesting. I really don't know how to
>> comment these code.
>
> Have a look at other PMU drivers, such as
> arch/x86/kernel/cpu/perf_event_intel_rapl.c, which begin with a
> general explanation attached below.
>
I think a more useful modularization would be to split that huge file
(perf_event_intel_uncore.c) into smaller files like we do for the core
PMU. There is just too much stuff in this file for my own taste. Hard
to navigate and I spend quite some time looking at it and modifying it!

You could follow the model of the core PMU support files.
You'd have a "core" file with the common routines, and then
a file perf processor:
    - perf_event_intel_uncore.c
    - perf_event_intel_snbep_uncore.c
    - perf_event_intel_nhmex_uncore.c
    - perf_event_intel_ivt_uncore.c
    - ...

Each processor specific module, would be a kernel module.
The core could be one too. Note that this would not alleviate
the need for some basic descriptions at the beginning of
each file outlining the PMU boxes exported to a minimum.


> /*
>  * perf_event_intel_rapl.c: support Intel RAPL energy consumption counters
>  * Copyright (C) 2013 Google, Inc., Stephane Eranian
>  *
>  * Intel RAPL interface is specified in the IA-32 Manual Vol3b
>  * section 14.7.1 (September 2013)
>  *
>  * RAPL provides more controls than just reporting energy consumption
>  * however here we only expose the 3 energy consumption free running
>  * counters (pp0, pkg, dram).
>  *
>  * Each of those counters increments in a power unit defined by the
>  * RAPL_POWER_UNIT MSR. On SandyBridge, this unit is 1/(2^16) Joules
>  * but it can vary.
>  *
>  * Counter to rapl events mappings:
>  *
>  *  pp0 counter: consumption of all physical cores (power plane 0)
>  *        event: rapl_energy_cores
>  *    perf code: 0x1
>  *
>  *  pkg counter: consumption of the whole processor package
>  *        event: rapl_energy_pkg
>  *    perf code: 0x2
>  *
>  * dram counter: consumption of the dram domain (servers only)
>  *        event: rapl_energy_dram
>  *    perf code: 0x3
>  *
>  * dram counter: consumption of the builtin-gpu domain (client only)
>  *        event: rapl_energy_gpu
>  *    perf code: 0x4
>  *
>  * We manage those counters as free running (read-only). They may be
>  * use simultaneously by other tools, such as turbostat.
>  *
>  * The events only support system-wide mode counting. There is no
>  * sampling support because it does not make sense and is not
>  * supported by the RAPL hardware.
>  *
>  * Because we want to avoid floating-point operations in the kernel,
>  * the events are all reported in fixed point arithmetic (32.32).
>  * Tools must adjust the counts to convert them to Watts using
>  * the duration of the measurement. Tools may use a function such as
>  * ldexp(raw_count, -32);
>  */

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Ingo Molnar]

* Stephane Eranian <eranian@google.com> wrote:

> >> Most of the codes without comments are hardware specific codes. 
> >> The corresponding contents in Intel uncore documents are big 
> >> tables/lists, nothing tricky/interesting. I really don't know how 
> >> to comment these code.
> >
> > Have a look at other PMU drivers, such as
> > arch/x86/kernel/cpu/perf_event_intel_rapl.c, which begin with a
> > general explanation attached below.
>
> I think a more useful modularization would be to split that huge 
> file (perf_event_intel_uncore.c) into smaller files like we do for 
> the core PMU. There is just too much stuff in this file for my own 
> taste. Hard to navigate and I spend quite some time looking at it 
> and modifying it!
> 
> You could follow the model of the core PMU support files.
> You'd have a "core" file with the common routines, and then
> a file perf processor:
>     - perf_event_intel_uncore.c
>     - perf_event_intel_snbep_uncore.c
>     - perf_event_intel_nhmex_uncore.c
>     - perf_event_intel_ivt_uncore.c
>     - ...
> 
> Each processor specific module, would be a kernel module. The core 
> could be one too. Note that this would not alleviate the need for 
> some basic descriptions at the beginning of each file outlining the 
> PMU boxes exported to a minimum.

This structure you outline sounds like a good first step, I like it.

To simplify this restructuring, initially we could even keep the core 
uncore bits in the core (ha!), to not have module-on-module 
dependencies.

Thanks,

	Ingo

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

On 04/24/2014 04:14 PM, Ingo Molnar wrote:
> 
> * Stephane Eranian <eranian@google.com> wrote:
> 
>>>> Most of the codes without comments are hardware specific codes. 
>>>> The corresponding contents in Intel uncore documents are big 
>>>> tables/lists, nothing tricky/interesting. I really don't know how 
>>>> to comment these code.
>>>
>>> Have a look at other PMU drivers, such as
>>> arch/x86/kernel/cpu/perf_event_intel_rapl.c, which begin with a
>>> general explanation attached below.
>>
>> I think a more useful modularization would be to split that huge 
>> file (perf_event_intel_uncore.c) into smaller files like we do for 
>> the core PMU. There is just too much stuff in this file for my own 
>> taste. Hard to navigate and I spend quite some time looking at it 
>> and modifying it!
>>
>> You could follow the model of the core PMU support files.
>> You'd have a "core" file with the common routines, and then
>> a file perf processor:
>>     - perf_event_intel_uncore.c
>>     - perf_event_intel_snbep_uncore.c
>>     - perf_event_intel_nhmex_uncore.c
>>     - perf_event_intel_ivt_uncore.c
>>     - ...
>>
>> Each processor specific module, would be a kernel module. The core 
>> could be one too. Note that this would not alleviate the need for 
>> some basic descriptions at the beginning of each file outlining the 
>> PMU boxes exported to a minimum.

Most of hardware specific codes in the Intel uncore driver are for SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are similar. One module per CPU type means we have to duplicate lots of code. I don't think it's a good idea.

Regards
Yan, Zheng

> 
> This structure you outline sounds like a good first step, I like it.
> 
> To simplify this restructuring, initially we could even keep the core 
> uncore bits in the core (ha!), to not have module-on-module 
> dependencies.
> 
> Thanks,
> 
> 	Ingo
> 

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Stephane Eranian]

On Thu, Apr 24, 2014 at 12:25 PM, Yan, Zheng <zheng.z.yan@intel.com> wrote:
> On 04/24/2014 04:14 PM, Ingo Molnar wrote:
>>
>> * Stephane Eranian <eranian@google.com> wrote:
>>
>>>>> Most of the codes without comments are hardware specific codes.
>>>>> The corresponding contents in Intel uncore documents are big
>>>>> tables/lists, nothing tricky/interesting. I really don't know how
>>>>> to comment these code.
>>>>
>>>> Have a look at other PMU drivers, such as
>>>> arch/x86/kernel/cpu/perf_event_intel_rapl.c, which begin with a
>>>> general explanation attached below.
>>>
>>> I think a more useful modularization would be to split that huge
>>> file (perf_event_intel_uncore.c) into smaller files like we do for
>>> the core PMU. There is just too much stuff in this file for my own
>>> taste. Hard to navigate and I spend quite some time looking at it
>>> and modifying it!
>>>
>>> You could follow the model of the core PMU support files.
>>> You'd have a "core" file with the common routines, and then
>>> a file perf processor:
>>>     - perf_event_intel_uncore.c
>>>     - perf_event_intel_snbep_uncore.c
>>>     - perf_event_intel_nhmex_uncore.c
>>>     - perf_event_intel_ivt_uncore.c
>>>     - ...
>>>
>>> Each processor specific module, would be a kernel module. The core
>>> could be one too. Note that this would not alleviate the need for
>>> some basic descriptions at the beginning of each file outlining the
>>> PMU boxes exported to a minimum.
>
> Most of hardware specific codes in the Intel uncore driver are for SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are similar. One module per CPU type means we have to duplicate lots of code. I don't think it's a good idea.
>
Then, at least split nhm_ex from the rest. It is very big.

> Regards
> Yan, Zheng
>
>>
>> This structure you outline sounds like a good first step, I like it.
>>
>> To simplify this restructuring, initially we could even keep the core
>> uncore bits in the core (ha!), to not have module-on-module
>> dependencies.
>>
>> Thanks,
>>
>>       Ingo
>>
>

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

On 04/24/2014 06:36 PM, Stephane Eranian wrote:
> On Thu, Apr 24, 2014 at 12:25 PM, Yan, Zheng <zheng.z.yan@intel.com> wrote:
>> On 04/24/2014 04:14 PM, Ingo Molnar wrote:
>>>
>>> * Stephane Eranian <eranian@google.com> wrote:
>>>
>>>>>> Most of the codes without comments are hardware specific codes.
>>>>>> The corresponding contents in Intel uncore documents are big
>>>>>> tables/lists, nothing tricky/interesting. I really don't know how
>>>>>> to comment these code.
>>>>>
>>>>> Have a look at other PMU drivers, such as
>>>>> arch/x86/kernel/cpu/perf_event_intel_rapl.c, which begin with a
>>>>> general explanation attached below.
>>>>
>>>> I think a more useful modularization would be to split that huge
>>>> file (perf_event_intel_uncore.c) into smaller files like we do for
>>>> the core PMU. There is just too much stuff in this file for my own
>>>> taste. Hard to navigate and I spend quite some time looking at it
>>>> and modifying it!
>>>>
>>>> You could follow the model of the core PMU support files.
>>>> You'd have a "core" file with the common routines, and then
>>>> a file perf processor:
>>>>     - perf_event_intel_uncore.c
>>>>     - perf_event_intel_snbep_uncore.c
>>>>     - perf_event_intel_nhmex_uncore.c
>>>>     - perf_event_intel_ivt_uncore.c
>>>>     - ...
>>>>
>>>> Each processor specific module, would be a kernel module. The core
>>>> could be one too. Note that this would not alleviate the need for
>>>> some basic descriptions at the beginning of each file outlining the
>>>> PMU boxes exported to a minimum.
>>
>> Most of hardware specific codes in the Intel uncore driver are for SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are similar. One module per CPU type means we have to duplicate lots of code. I don't think it's a good idea.
>>
> Then, at least split nhm_ex from the rest. It is very big.

Ok, will do.

Regards
Yan, Zheng

>>
>>>
>>> This structure you outline sounds like a good first step, I like it.
>>>
>>> To simplify this restructuring, initially we could even keep the core
>>> uncore bits in the core (ha!), to not have module-on-module
>>> dependencies.
>>>
>>> Thanks,
>>>
>>>       Ingo
>>>
>>

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Peter Zijlstra]

On Thu, Apr 24, 2014 at 12:36:01PM +0200, Stephane Eranian wrote:
> > Most of hardware specific codes in the Intel uncore driver are for
> > SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are
> > similar. One module per CPU type means we have to duplicate lots of
> > code. I don't think it's a good idea.
> >
> Then, at least split nhm_ex from the rest. It is very big.

Aren't the EX parts in general far more complex and different from the
EP parts? Or will the SNB/IVB/HSW-EX parts also be similar again? (this
would be a good thing).

But yes, it makes sense to at least split the EP and EX parts, and split
where the families are different.

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

On 04/24/2014 07:27 PM, Peter Zijlstra wrote:
> On Thu, Apr 24, 2014 at 12:36:01PM +0200, Stephane Eranian wrote:
>>> Most of hardware specific codes in the Intel uncore driver are for
>>> SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are
>>> similar. One module per CPU type means we have to duplicate lots of
>>> code. I don't think it's a good idea.
>>>
>> Then, at least split nhm_ex from the rest. It is very big.
> 
> Aren't the EX parts in general far more complex and different from the
> EP parts? Or will the SNB/IVB/HSW-EX parts also be similar again? (this
> would be a good thing).

SNB/IVB/HSW-EX are almost identical to SNB/IVB/HSW-EP. NHM/WSM-EX are complete
different from SNB/IVB/HSW-EX

Regards
Yan, Zheng


> 
> But yes, it makes sense to at least split the EP and EX parts, and split
> where the families are different.
> 

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Peter Zijlstra]

On Fri, Apr 25, 2014 at 09:17:36PM +0800, Yan, Zheng wrote:
> On 04/24/2014 07:27 PM, Peter Zijlstra wrote:
> > On Thu, Apr 24, 2014 at 12:36:01PM +0200, Stephane Eranian wrote:
> >>> Most of hardware specific codes in the Intel uncore driver are for
> >>> SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are
> >>> similar. One module per CPU type means we have to duplicate lots of
> >>> code. I don't think it's a good idea.
> >>>
> >> Then, at least split nhm_ex from the rest. It is very big.
> > 
> > Aren't the EX parts in general far more complex and different from the
> > EP parts? Or will the SNB/IVB/HSW-EX parts also be similar again? (this
> > would be a good thing).
> 
> SNB/IVB/HSW-EX are almost identical to SNB/IVB/HSW-EP.

Ah, good, sanity prevails!

> NHM/WSM-EX are complete different from SNB/IVB/HSW-EX

Yeah, NHM/WSM-EX are different from pretty much anything.

Where do NHM/WSM-EP fall? I know they're radically different to the EX
parts but are they similar again to the SNB/IVB/HSW EP parts?

If not we'd have 3 groups:

NHM/WSM-EP
NHM/WSM-EX
SNB/IVB/HSW

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

On 04/25/2014 09:55 PM, Peter Zijlstra wrote:
> On Fri, Apr 25, 2014 at 09:17:36PM +0800, Yan, Zheng wrote:
>> On 04/24/2014 07:27 PM, Peter Zijlstra wrote:
>>> On Thu, Apr 24, 2014 at 12:36:01PM +0200, Stephane Eranian wrote:
>>>>> Most of hardware specific codes in the Intel uncore driver are for
>>>>> SandyBridge/IvyBridge/Haswell. Uncore subsystem in these CPUs are
>>>>> similar. One module per CPU type means we have to duplicate lots of
>>>>> code. I don't think it's a good idea.
>>>>>
>>>> Then, at least split nhm_ex from the rest. It is very big.
>>>
>>> Aren't the EX parts in general far more complex and different from the
>>> EP parts? Or will the SNB/IVB/HSW-EX parts also be similar again? (this
>>> would be a good thing).
>>
>> SNB/IVB/HSW-EX are almost identical to SNB/IVB/HSW-EP.
> 
> Ah, good, sanity prevails!
> 
>> NHM/WSM-EX are complete different from SNB/IVB/HSW-EX
> 
> Yeah, NHM/WSM-EX are different from pretty much anything.
> 
> Where do NHM/WSM-EP fall? I know they're radically different to the EX
> parts but are they similar again to the SNB/IVB/HSW EP parts?
> 

the uncore driver does not support NHM/WSM-EP (I don't know the module numbers for NHM/WSM-EP, maybe I'm wrong)

> If not we'd have 3 groups:
> 
> NHM/WSM-EP
> NHM/WSM-EX
> SNB/IVB/HSW
> 

I think we'd have:

NHM/WSM-EX
SNB/IVB/HSW-EP
Desktop version of NHM/SNB/IVB/HSW

Regards
Yan, Zheng

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Peter Zijlstra]

On Fri, Apr 25, 2014 at 10:06:15PM +0800, Yan, Zheng wrote:
> the uncore driver does not support NHM/WSM-EP (I don't know the module numbers for NHM/WSM-EP, maybe I'm wrong)

My wsm-ep has /sys/bus/event_source/devices/uncore so I suppose the
driver loaded. Its model 44 fwiw.

Also, from the uncore driver:

	switch (boot_cpu_data.x86_model) {
	case 26: /* Nehalem */
	case 30:
	case 37: /* Westmere */
	case 44:
		msr_uncores = nhm_msr_uncores;
		break;

Re: [PATCH v2 4/4] perf/x86/uncore: modularize Intel uncore driver

[Yan, Zheng]

On 04/25/2014 10:35 PM, Peter Zijlstra wrote:
> On Fri, Apr 25, 2014 at 10:06:15PM +0800, Yan, Zheng wrote:
>> the uncore driver does not support NHM/WSM-EP (I don't know the module numbers for NHM/WSM-EP, maybe I'm wrong)
> 
> My wsm-ep has /sys/bus/event_source/devices/uncore so I suppose the
> driver loaded. Its model 44 fwiw.
> 
> Also, from the uncore driver:
> 
> 	switch (boot_cpu_data.x86_model) {
> 	case 26: /* Nehalem */
> 	case 30:
> 	case 37: /* Westmere */
> 	case 44:
> 		msr_uncores = nhm_msr_uncores;
> 		break;
> 

looks like the uncore in NHM/WSM-EP is the same as uncore in desktop version NHM/WSM.

Regards
Yan, Zheng