| Message ID | 20200506110737.14904-6-kjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/hv-24x7: Expose chip/sockets info to add json file metric support for the hv_24x7 socket/chip level events | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (1bc92fe3175eb26ff37e580c0383d7a9abe06835) |
| snowpatch_ozlabs/build-ppc64le | warning | Build succeeded but added 1 new sparse warnings |
| snowpatch_ozlabs/build-ppc64be | warning | Build succeeded but added 1 new sparse warnings |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 28 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi Kajol,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on linus/master v5.7-rc4 next-20200508]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Kajol-Jain/powerpc-hv-24x7-Expose-chip-sockets-info-to-add-json-file-metric-support-for-the-hv_24x7-socket-chip-level-events/20200507-032548
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r013-20200508 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/powerpc/platforms/pseries/mobility.c:48:20: error: static declaration of 'read_sys_info_pseries' follows non-static declaration
48 | static inline void read_sys_info_pseries(void) { }
| ^~~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/platforms/pseries/mobility.c:22:
arch/powerpc/include/asm/rtas.h:485:13: note: previous declaration of 'read_sys_info_pseries' was here
485 | extern void read_sys_info_pseries(void);
| ^~~~~~~~~~~~~~~~~~~~~
vim +/read_sys_info_pseries +48 arch/powerpc/platforms/pseries/mobility.c
44
45 #ifdef CONFIG_HV_PERF_CTRS
46 void read_sys_info_pseries(void);
47 #else
> 48 static inline void read_sys_info_pseries(void) { }
49 #endif
50
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hello, Kajol Jain <kjain@linux.ibm.com> writes: > Function 'read_sys_info_pseries()' is added to get system parameter > values like number of sockets and chips per socket. > and it gets these details via rtas_call with token > "PROCESSOR_MODULE_INFO". > > Incase lpar migrate from one system to another, system > parameter details like chips per sockets or number of sockets might > change. So, it needs to be re-initialized otherwise, these values > corresponds to previous system values. > This patch adds a call to 'read_sys_info_pseries()' from > 'post-mobility_fixup()' to re-init the physsockets and physchips values > > Signed-off-by: Kajol Jain <kjain@linux.ibm.com> > --- > arch/powerpc/platforms/pseries/mobility.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) Please cc me on patches for this code, thanks. I see no technical problems with how this patch handles partition migration. However: "Update post_mobility_fixup() to handle migration" is not an appropriate summary for this change. post_mobility_fixup() already handles migration. A better summary would be "powerpc/pseries: update hv-24x7 info after migration" > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c > index b571285f6c14..0fb8f1e6e9d2 100644 > --- a/arch/powerpc/platforms/pseries/mobility.c > +++ b/arch/powerpc/platforms/pseries/mobility.c > @@ -42,6 +42,12 @@ struct update_props_workarea { > #define MIGRATION_SCOPE (1) > #define PRRN_SCOPE -2 > > +#ifdef CONFIG_HV_PERF_CTRS > +void read_sys_info_pseries(void); > +#else > +static inline void read_sys_info_pseries(void) { } > +#endif This should go in a header. > static int mobility_rtas_call(int token, char *buf, s32 scope) > { > int rc; > @@ -371,6 +377,16 @@ void post_mobility_fixup(void) > /* Possibly switch to a new RFI flush type */ > pseries_setup_rfi_flush(); > > + /* > + * In case an Lpar migrates from one system to another, system > + * parameter details like chips per sockets, cores per chip and > + * number of sockets details might change. > + * So, they needs to be re-initialized otherwise the > + * values will correspond to the previous system. > + * Call read_sys_info_pseries() to reinitialise the values. > + */ This is needlessly verbose; any literate reader of this code knows this is used immediately after resuming from a suspend (migration). If you give your hook a more descriptive name, the comment becomes unnecessary. > + read_sys_info_pseries(); > +
On 5/12/20 1:10 AM, Nathan Lynch wrote: > Hello, > > Kajol Jain <kjain@linux.ibm.com> writes: >> Function 'read_sys_info_pseries()' is added to get system parameter >> values like number of sockets and chips per socket. >> and it gets these details via rtas_call with token >> "PROCESSOR_MODULE_INFO". >> >> Incase lpar migrate from one system to another, system >> parameter details like chips per sockets or number of sockets might >> change. So, it needs to be re-initialized otherwise, these values >> corresponds to previous system values. >> This patch adds a call to 'read_sys_info_pseries()' from >> 'post-mobility_fixup()' to re-init the physsockets and physchips values >> >> Signed-off-by: Kajol Jain <kjain@linux.ibm.com> >> --- >> arch/powerpc/platforms/pseries/mobility.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) > > Please cc me on patches for this code, thanks. Hi Nathan, Thanks for reviewing the patch. I will cc you on next version of this patchset. > > I see no technical problems with how this patch handles partition > migration. However: > > "Update post_mobility_fixup() to handle migration" is not an appropriate > summary for this change. post_mobility_fixup() already handles > migration. A better summary would be > > "powerpc/pseries: update hv-24x7 info after migration" Will update. > > >> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c >> index b571285f6c14..0fb8f1e6e9d2 100644 >> --- a/arch/powerpc/platforms/pseries/mobility.c >> +++ b/arch/powerpc/platforms/pseries/mobility.c >> @@ -42,6 +42,12 @@ struct update_props_workarea { >> #define MIGRATION_SCOPE (1) >> #define PRRN_SCOPE -2 >> >> +#ifdef CONFIG_HV_PERF_CTRS >> +void read_sys_info_pseries(void); >> +#else >> +static inline void read_sys_info_pseries(void) { } >> +#endif > > This should go in a header. > > >> static int mobility_rtas_call(int token, char *buf, s32 scope) >> { >> int rc; >> @@ -371,6 +377,16 @@ void post_mobility_fixup(void) >> /* Possibly switch to a new RFI flush type */ >> pseries_setup_rfi_flush(); >> >> + /* >> + * In case an Lpar migrates from one system to another, system >> + * parameter details like chips per sockets, cores per chip and >> + * number of sockets details might change. >> + * So, they needs to be re-initialized otherwise the >> + * values will correspond to the previous system. >> + * Call read_sys_info_pseries() to reinitialise the values. >> + */ > > This is needlessly verbose; any literate reader of this code knows this > is used immediately after resuming from a suspend (migration). If you > give your hook a more descriptive name, the comment becomes unnecessary. > Yes make sense, will update. Thanks, Kajol Jain > >> + read_sys_info_pseries(); >> + >
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c index b571285f6c14..0fb8f1e6e9d2 100644 --- a/arch/powerpc/platforms/pseries/mobility.c +++ b/arch/powerpc/platforms/pseries/mobility.c @@ -42,6 +42,12 @@ struct update_props_workarea { #define MIGRATION_SCOPE (1) #define PRRN_SCOPE -2 +#ifdef CONFIG_HV_PERF_CTRS +void read_sys_info_pseries(void); +#else +static inline void read_sys_info_pseries(void) { } +#endif + static int mobility_rtas_call(int token, char *buf, s32 scope) { int rc; @@ -371,6 +377,16 @@ void post_mobility_fixup(void) /* Possibly switch to a new RFI flush type */ pseries_setup_rfi_flush(); + /* + * In case an Lpar migrates from one system to another, system + * parameter details like chips per sockets, cores per chip and + * number of sockets details might change. + * So, they needs to be re-initialized otherwise the + * values will correspond to the previous system. + * Call read_sys_info_pseries() to reinitialise the values. + */ + read_sys_info_pseries(); + return; }
Function 'read_sys_info_pseries()' is added to get system parameter values like number of sockets and chips per socket. and it gets these details via rtas_call with token "PROCESSOR_MODULE_INFO". Incase lpar migrate from one system to another, system parameter details like chips per sockets or number of sockets might change. So, it needs to be re-initialized otherwise, these values corresponds to previous system values. This patch adds a call to 'read_sys_info_pseries()' from 'post-mobility_fixup()' to re-init the physsockets and physchips values Signed-off-by: Kajol Jain <kjain@linux.ibm.com> --- arch/powerpc/platforms/pseries/mobility.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)