Re: [RESEND PATCH v2] powerpc/kernel/sysfs: Add PMU_SYSFS config option to enable PMU SPRs sysfs file creation

[kajoljain <kjain@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 6466 bytes --]

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@linux.ibm.com>
>>
>> Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> Tested-by: Nageswara R Sastry <nasastry@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


[-- Attachment #2: Type: text/html, Size: 12939 bytes --]