* Yan, Zheng <[email protected]> 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 <[email protected]>
> ---
> 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
> 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
On 04/18/2014 06:53 PM, Ingo Molnar wrote:
>
> * Yan, Zheng <[email protected]> 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 <[email protected]>
>> ---
>> 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
>
* Yan, Zheng <[email protected]> wrote:
> On 04/18/2014 06:53 PM, Ingo Molnar wrote:
> >
> > * Yan, Zheng <[email protected]> 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 <[email protected]>
> >> ---
> >> 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);
*/
On Tue, Apr 22, 2014 at 1:35 PM, Ingo Molnar <[email protected]> wrote:
>
> * Yan, Zheng <[email protected]> wrote:
>
>> On 04/18/2014 06:53 PM, Ingo Molnar wrote:
>> >
>> > * Yan, Zheng <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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);
> */
* Stephane Eranian <[email protected]> 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
On 04/24/2014 04:14 PM, Ingo Molnar wrote:
>
> * Stephane Eranian <[email protected]> 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
>
On Thu, Apr 24, 2014 at 12:25 PM, Yan, Zheng <[email protected]> wrote:
> On 04/24/2014 04:14 PM, Ingo Molnar wrote:
>>
>> * Stephane Eranian <[email protected]> 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
>>
>
On 04/24/2014 06:36 PM, Stephane Eranian wrote:
> On Thu, Apr 24, 2014 at 12:25 PM, Yan, Zheng <[email protected]> wrote:
>> On 04/24/2014 04:14 PM, Ingo Molnar wrote:
>>>
>>> * Stephane Eranian <[email protected]> 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
>>>
>>
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.
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.
>
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
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
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;
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