Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration
On 5/12/20 1:10 AM, Nathan Lynch wrote:
> Hello,
>
> Kajol Jain 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
>> ---
>> 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();
>> +
>
Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration
Hello,
Kajol Jain 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
> ---
> 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();
> +
Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration
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
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-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration
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
---
arch/powerpc/platforms/pseries/mobility.c | 16
1 file changed, 16 insertions(+)
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;
}
--
2.18.2
