[RESEND PATCH v2] powerpc/kernel/sysfs: Add PMU_SYSFS config option to enable PMU SPRs sysfs file creation
kajoljain
kjain at linux.ibm.com
Mon Dec 9 19:21:39 AEDT 2019
Hi Christophe,
Thankyou for reviewing the patch.
On 12/5/19 11:28 AM, Christophe Leroy wrote:
>
>
> Le 05/12/2019 à 06:25, Kajol Jain a écrit :
>> Many of the performance moniroting unit (PMU) SPRs are
>> exposed in the sysfs. "perf" API is the primary interface to program
>> PMU and collect counter data in the system. So expose these
>> PMU SPRs in the absence of CONFIG_PERF_EVENTS.
>>
>> Patch adds a new CONFIG option 'CONFIG_PMU_SYSFS'. The new config
>> option used in kernel/sysfs.c for PMU SPRs sysfs file creation and
>> this new option is enabled only if 'CONFIG_PERF_EVENTS' option is
>> disabled.
>
> Not sure this new unselectable option is worth it. See below.
> By the way I also find the subject misleading, as you may believe when
> reading it that there is something to select.
Ok I wiil change the subject.
>
>>
>> Tested this patch with enable/disable CONFIG_PERF_EVENTS option
>> in powernv and pseries machines.
>> Also did compilation testing for different architecture include:
>> x86, mips, mips64, alpha, arm. And with book3s_32.config option.
>
> How do you use book3s_32.config exactly ? That's a portion of config,
> not a config by itself. You should use pmac32_defconfig I guess.
Yes you are right, its not a config option. Actually I was playing
around with 'CONFIG_PPC_BOOK3S' option in .config file. As you suggested,
I will try to build with 'pmac32_defconfig' option.
>
>>
>> Signed-off-by: Kajol Jain <kjain at linux.ibm.com>
>>
>> Reviewed-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
>> Tested-by: Nageswara R Sastry <nasastry at in.ibm.com>
>>
>> Tested using the following different scenarios:
>> 1. CONFIG_PERF_EVENT - enabled, CONFIG_PMU_SYSFS - disabled,
>> RESULT: not seen any sysfs files(mmrc*, pmc*) from
>> /sys/bus/cpu/devices/cpu?/
>> 2. CONFIG_PERF_EVENT - disabled, CONFIG_PMU_SYSFS - enabled,
>> RESULT: seen any sysfs files(mmrc*, pmc*) from
>> /sys/bus/cpu/devices/cpu?/
>> 3. CONFIG_PERF_EVENT -disabled, CONFIG_PMU_SYSFS - disabled,
>> RESULT: not possible, any one of the config options need to be enabled.
>> 4. CONFIG_PERF_EVENT -enabled, CONFIG_PMU_SYSFS - enabled,
>> RESULT: not possible, any one of the config options need to be enabled.
>> ---
>> arch/powerpc/kernel/sysfs.c | 21 +++++++++++++++++++++
>> arch/powerpc/platforms/Kconfig.cputype | 8 ++++++++
>> 2 files changed, 29 insertions(+)
>>
>> ---
>> Changelog:
>> Resend v2
>> Added 'Reviewed-by' and 'Tested-by' tag along with test scenarios.
>>
>> v1 -> v2
>> - Added new config option 'PMU_SYSFS' for PMU SPR's creation
>> rather than using PERF_EVENTS config option directly and make
>> sure SPR's file creation only if 'CONFIG_PERF_EVENTS' disabled.
>> ---
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 80a676da11cb..b7c01f1ef236 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -457,16 +457,21 @@ static ssize_t __used \
>> #if defined(CONFIG_PPC64)
>> #define HAS_PPC_PMC_CLASSIC 1
>> +#ifdef CONFIG_PMU_SYSFS
>> #define HAS_PPC_PMC_IBM 1
>> +#endif
>> #define HAS_PPC_PMC_PA6T 1
>> #elif defined(CONFIG_PPC_BOOK3S_32)
>> #define HAS_PPC_PMC_CLASSIC 1
>> +#ifdef CONFIG_PMU_SYSFS
>> #define HAS_PPC_PMC_IBM 1
>> #define HAS_PPC_PMC_G4 1
>> #endif
>> +#endif
>> #ifdef HAS_PPC_PMC_CLASSIC
>> +#ifdef CONFIG_PMU_SYSFS
>
> You don't need this big forest of #ifdefs (this one and all the ones
> after). All the objets you are protecting with this are indeed static.
> So the only thing you have to do is to register them only when
> relevant, and GCC will get rid of the objects by itself when the
> config option is not enabled. See below.
>
> And the advantage of doing that way is that you don't need to build it
> with both options to check the build. That's recommended by kernel
> codying style (Refer
> https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation)
>
> [...]
>
>> @@ -787,8 +804,10 @@ static int register_cpu_online(unsigned int cpu)
>> device_create_file(s, &pmc_attrs[i]);
>> #ifdef CONFIG_PPC64
>> +#ifdef CONFIG_PMU_SYSFS
>
> Don't use #ifdef here, just do instead:
>
> if (IS_ENABLED(CONFIG_PMU_SYSFS) && cpu_has_feature(CPU_FTR_MMCRA))
Thanks for the suggestion I will use IS_ENABLED here.
>
>> if (cpu_has_feature(CPU_FTR_MMCRA))
>> device_create_file(s, &dev_attr_mmcra);
>> +#endif /* CONFIG_PMU_SYSFS */
>> if (cpu_has_feature(CPU_FTR_PURR)) {
>> if (!firmware_has_feature(FW_FEATURE_LPAR))
>> @@ -876,8 +895,10 @@ static int unregister_cpu_online(unsigned int cpu)
>> device_remove_file(s, &pmc_attrs[i]);
>> #ifdef CONFIG_PPC64
>> +#ifdef CONFIG_PMU_SYSFS
>
> Same, use IS_ENABLED() here as well.
>
>> if (cpu_has_feature(CPU_FTR_MMCRA))
>> device_remove_file(s, &dev_attr_mmcra);
>> +#endif /* CONFIG_PMU_SYSFS */
>> if (cpu_has_feature(CPU_FTR_PURR))
>> device_remove_file(s, &dev_attr_purr);
>> diff --git a/arch/powerpc/platforms/Kconfig.cputype
>> b/arch/powerpc/platforms/Kconfig.cputype
>> index 12543e53fa96..f3ad579c559f 100644
>> --- a/arch/powerpc/platforms/Kconfig.cputype
>> +++ b/arch/powerpc/platforms/Kconfig.cputype
>> @@ -417,6 +417,14 @@ config PPC_MM_SLICES
>> config PPC_HAVE_PMU_SUPPORT
>> bool
>> +config PMU_SYSFS
>> + bool
>> + default y if !PERF_EVENTS
>> + help
>> + This option enables PMU SPR sysfs file creation. Since PMU
>> SPRs are
>> + intended to be used via "perf" interface, config option is
>> enabled
>> + only when CONFIG_PERF_EVENTS is disabled.
>> +
>
> Not sure you need this at all. Once you have changed to just using
> IS_ENABLED() in the two places above, I think it is acceptable to use
> !IS_ENABLED(CONFIG_PERF_EVENTS) instead.
Actually with v1 of the patch, I tried with PERF_EVENT option, but it
was getting bit messy to recreate the current arrangement in the file.
So took a new config option path.
>
>> config PPC_PERF_CTRS
>> def_bool y
>> depends on PERF_EVENTS && PPC_HAVE_PMU_SUPPORT
>>
>
>
> Christophe
Thanks,
Kajol Jain
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20191209/040e7d54/attachment-0001.htm>
More information about the Linuxppc-dev
mailing list