[PATCH v8 0/5] powerpc/hv-24x7: Expose chip/sockets info to add json file metric support for the hv_24x7 socket/chip level events

[PATCH v8 0/5] powerpc/hv-24x7: Expose chip/sockets info to add json file metric support for the hv_24x7 socket/chip level events

[Kajol Jain]

Patchset fixes the inconsistent results we are getting when
we run multiple 24x7 events.

"hv_24x7" pmu interface events needs system dependent parameter
like socket/chip/core. For example, hv_24x7 chip level events needs
specific chip-id to which the data is requested should be added as part
of pmu events.

So to enable JSON file support to "hv_24x7" interface, patchset expose
total number of sockets and chips per-socket details in sysfs
files (sockets, chips) under "/sys/devices/hv_24x7/interface/".

To get number of sockets, chips per sockets and cores per chip patchset adds a
rtas call with token "PROCESSOR_MODULE_INFO" to get these details. Patchset
also handles partition migration case to re-init these system depended
parameters by adding proper calls in post_mobility_fixup() (mobility.c).

v7: http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=167076

Changelog:
v7 -> v8
- Add support for exposing cores per details as well.
  Suggested by: Madhavan Srinivasan.
- Remove config check for 'CONFIG_PPC_RTAS' in previous
  implementation and address other comments by Michael Ellerman.

v6 -> v7
- Split patchset into two patch series, one with kernel changes
  and another with perf tool side changes. This pachset contain
  all kernel side changes.

Kajol Jain (5):
  powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple
    hv-24x7 events run
  powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor
    details
  powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show
    processor details
  Documentation/ABI: Add ABI documentation for chips and sockets
  powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

 .../sysfs-bus-event_source-devices-hv_24x7    |  21 ++++
 arch/powerpc/include/asm/rtas.h               |   1 +
 arch/powerpc/perf/hv-24x7.c                   | 106 ++++++++++++++++--
 arch/powerpc/platforms/pseries/mobility.c     |  16 +++
 4 files changed, 134 insertions(+), 10 deletions(-)

-- 
2.18.2

[PATCH v8 1/5] powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple hv-24x7 events run

[Kajol Jain]

Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter
values")' added to print _change_ in the counter value rather then raw
value for 24x7 counters. Incase of transactions, the event count
is set to 0 at the beginning of the transaction. It also sets
the event's prev_count to the raw value at the time of initialization.
Because of setting event count to 0, we are seeing some weird behaviour,
whenever we run multiple 24x7 events at a time.

For example:

command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
			   hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
	  		   -C 0 -I 1000 sleep 100

     1.000121704                120 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     1.000121704                  5 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     2.000357733                  8 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     2.000357733                 10 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.000641884                 56 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     4.000641884 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     5.000791887 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/

Getting these large values in case we do -I.

As we are setting event_count to 0, for interval case, overall event_count is not
coming in incremental order. As we may can get new delta lesser then previous count.
Because of which when we print intervals, we are getting negative value which create
these large values.

This patch removes part where we set event_count to 0 in function
'h_24x7_event_read'. There won't be much impact as we do set event->hw.prev_count
to the raw value at the time of initialization to print change value.

With this patch
In power9 platform

command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
		           hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
			   -C 0 -I 1000 sleep 100

     1.000117685                 93 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     1.000117685                  1 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     2.000349331                 98 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     2.000349331                  2 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     3.000495900                131 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     3.000495900                  4 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.000645920                204 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     4.000645920                 61 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.284169997                 22 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Suggested-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 573e0b309c0c..48e8f4b17b91 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1400,16 +1400,6 @@ static void h_24x7_event_read(struct perf_event *event)
 			h24x7hw = &get_cpu_var(hv_24x7_hw);
 			h24x7hw->events[i] = event;
 			put_cpu_var(h24x7hw);
-			/*
-			 * Clear the event count so we can compute the _change_
-			 * in the 24x7 raw counter value at the end of the txn.
-			 *
-			 * Note that we could alternatively read the 24x7 value
-			 * now and save its value in event->hw.prev_count. But
-			 * that would require issuing a hcall, which would then
-			 * defeat the purpose of using the txn interface.
-			 */
-			local64_set(&event->count, 0);
 		}
 
 		put_cpu_var(hv_24x7_reqb);
-- 
2.18.2

[PATCH v8 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[Kajol Jain]

For hv_24x7 socket/chip level events, specific chip-id to which
the data requested should be added as part of pmu events.
But number of chips/socket in the system details are not exposed.

Patch implements read_sys_info_pseries() to get system
parameter values like number of sockets and chips per socket.
Rtas_call with token "PROCESSOR_MODULE_INFO"
is used to get these values.

Sub-sequent patch exports these values via sysfs.

Patch also make these parameters default to 1.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/include/asm/rtas.h |  1 +
 arch/powerpc/perf/hv-24x7.c     | 72 +++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 3c1887351c71..1c11f814932d 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -482,6 +482,7 @@ static inline void rtas_initialize(void) { };
 #endif
 
 extern int call_rtas(const char *, int, int, unsigned long *, ...);
+extern void read_sys_info_pseries(void);
 
 #endif /* __KERNEL__ */
 #endif /* _POWERPC_RTAS_H */
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 48e8f4b17b91..8cf242aad98f 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -20,6 +20,7 @@
 #include <asm/io.h>
 #include <linux/byteorder/generic.h>
 
+#include <asm/rtas.h>
 #include "hv-24x7.h"
 #include "hv-24x7-catalog.h"
 #include "hv-common.h"
@@ -57,6 +58,75 @@ static bool is_physical_domain(unsigned domain)
 	}
 }
 
+/*
+ * The Processor Module Information system parameter allows transferring
+ * of certain processor module information from the platform to the OS.
+ * Refer PAPR+ document to get parameter token value as '43'.
+ */
+
+#define PROCESSOR_MODULE_INFO   43
+#define PROCESSOR_MAX_LENGTH	(8 * 1024)
+
+DEFINE_SPINLOCK(rtas_local_data_buf_lock);
+EXPORT_SYMBOL(rtas_local_data_buf_lock);
+
+static u32 phys_sockets;	/* Physical sockets */
+static u32 phys_chipspersocket;	/* Physical chips per socket*/
+static u32 phys_coresperchip; /* Physical cores per chip */
+
+/*
+ * Function read_sys_info_pseries() make a rtas_call which require
+ * data buffer of size 8K. As standard 'rtas_data_buf' is of size
+ * 4K, we are adding new local buffer 'rtas_local_data_buf'.
+ */
+static __be16 rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
+
+/*
+ * read_sys_info_pseries()
+ * Retrieve the number of sockets and chips per socket and cores per
+ * chip details through the get-system-parameter rtas call.
+ */
+void read_sys_info_pseries(void)
+{
+	int call_status, len, ntypes;
+
+	/*
+	 * Making system parameter: chips and sockets and cores per chip
+	 * default to 1.
+	 */
+	phys_sockets = 1;
+	phys_chipspersocket = 1;
+	phys_coresperchip = 1;
+	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH * sizeof(__be16));
+	spin_lock(&rtas_local_data_buf_lock);
+
+	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
+				NULL,
+				PROCESSOR_MODULE_INFO,
+				__pa(rtas_local_data_buf),
+				PROCESSOR_MAX_LENGTH);
+
+	spin_unlock(&rtas_local_data_buf_lock);
+
+	if (call_status != 0) {
+		pr_info("Error calling get-system-parameter (0x%x)\n",
+			call_status);
+	} else {
+		rtas_local_data_buf[PROCESSOR_MAX_LENGTH - 1] = '\0';
+		len = be16_to_cpup((__be16 *)&rtas_local_data_buf[0]);
+		if (len < 4)
+			return;
+
+		ntypes = be16_to_cpup(&rtas_local_data_buf[1]);
+
+		if (!ntypes)
+			return;
+		phys_sockets = be16_to_cpup(&rtas_local_data_buf[2]);
+		phys_chipspersocket = be16_to_cpup(&rtas_local_data_buf[3]);
+		phys_coresperchip = be16_to_cpup(&rtas_local_data_buf[4]);
+	}
+}
+
 /* Domains for which more than one result element are returned for each event. */
 static bool domain_needs_aggregation(unsigned int domain)
 {
@@ -1605,6 +1675,8 @@ static int hv_24x7_init(void)
 	if (r)
 		return r;
 
+	read_sys_info_pseries();
+
 	return 0;
 }
 
-- 
2.18.2

[PATCH v8 3/5] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show processor details

[Kajol Jain]

To expose the system dependent parameter like total number of
sockets and numbers of chips per socket, patch adds two sysfs files.
"sockets" and "chips" are added to /sys/devices/hv_24x7/interface/
of the "hv_24x7" pmu.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 8cf242aad98f..f24dee2a660a 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -456,6 +456,24 @@ static ssize_t device_show_string(struct device *dev,
 	return sprintf(buf, "%s\n", (char *)d->var);
 }
 
+static ssize_t sockets_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", phys_sockets);
+}
+
+static ssize_t chipspersocket_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", phys_chipspersocket);
+}
+
+static ssize_t coresperchip_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", phys_coresperchip);
+}
+
 static struct attribute *device_str_attr_create_(char *name, char *str)
 {
 	struct dev_ext_attribute *attr = kzalloc(sizeof(*attr), GFP_KERNEL);
@@ -1102,6 +1120,9 @@ PAGE_0_ATTR(catalog_len, "%lld\n",
 		(unsigned long long)be32_to_cpu(page_0->length) * 4096);
 static BIN_ATTR_RO(catalog, 0/* real length varies */);
 static DEVICE_ATTR_RO(domains);
+static DEVICE_ATTR_RO(sockets);
+static DEVICE_ATTR_RO(chipspersocket);
+static DEVICE_ATTR_RO(coresperchip);
 
 static struct bin_attribute *if_bin_attrs[] = {
 	&bin_attr_catalog,
@@ -1112,6 +1133,9 @@ static struct attribute *if_attrs[] = {
 	&dev_attr_catalog_len.attr,
 	&dev_attr_catalog_version.attr,
 	&dev_attr_domains.attr,
+	&dev_attr_sockets.attr,
+	&dev_attr_chipspersocket.attr,
+	&dev_attr_coresperchip.attr,
 	NULL,
 };
 
-- 
2.18.2

[PATCH v8 4/5] Documentation/ABI: Add ABI documentation for chips and sockets

[Kajol Jain]

Add documentation for the following sysfs files:
/sys/devices/hv_24x7/interface/chipspersocket,
/sys/devices/hv_24x7/interface/sockets,
/sys/devices/hv_24x7/interface/coresperchip

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 .../sysfs-bus-event_source-devices-hv_24x7    | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7 b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
index ec27c6c9e737..e8698afcd952 100644
--- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
+++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
@@ -22,6 +22,27 @@ Description:
 		Exposes the "version" field of the 24x7 catalog. This is also
 		extractable from the provided binary "catalog" sysfs entry.
 
+What:		/sys/devices/hv_24x7/interface/sockets
+Date:		May 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of sockets present in the
+		system.
+
+What:		/sys/devices/hv_24x7/interface/chipspersocket
+Date:		May 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of chips per socket
+		present in the system.
+
+What:		/sys/devices/hv_24x7/interface/coresperchip
+Date:		May 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of cores per chip
+		present in the system.
+
 What:		/sys/bus/event_source/devices/hv_24x7/event_descs/<event-name>
 Date:		February 2014
 Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
-- 
2.18.2

[PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Kajol Jain]

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(+)

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

Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[kbuild test robot]

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

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32632 bytes --]

Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[kbuild test robot]

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

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32632 bytes --]

Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Nathan Lynch]

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();
> +

Re: [PATCH v8 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[Nathan Lynch]

Hi,

Kajol Jain <kjain@linux.ibm.com> writes:
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 48e8f4b17b91..8cf242aad98f 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -20,6 +20,7 @@
>  #include <asm/io.h>
>  #include <linux/byteorder/generic.h>
>  
> +#include <asm/rtas.h>
>  #include "hv-24x7.h"
>  #include "hv-24x7-catalog.h"
>  #include "hv-common.h"
> @@ -57,6 +58,75 @@ static bool is_physical_domain(unsigned domain)
>  	}
>  }
>  
> +/*
> + * The Processor Module Information system parameter allows transferring
> + * of certain processor module information from the platform to the OS.
> + * Refer PAPR+ document to get parameter token value as '43'.
> + */
> +
> +#define PROCESSOR_MODULE_INFO   43
> +#define PROCESSOR_MAX_LENGTH	(8 * 1024)
> +
> +DEFINE_SPINLOCK(rtas_local_data_buf_lock);
> +EXPORT_SYMBOL(rtas_local_data_buf_lock);

This should be static and not exported, correct?

> +
> +static u32 phys_sockets;	/* Physical sockets */
> +static u32 phys_chipspersocket;	/* Physical chips per socket*/
> +static u32 phys_coresperchip; /* Physical cores per chip */
> +
> +/*
> + * Function read_sys_info_pseries() make a rtas_call which require
> + * data buffer of size 8K. As standard 'rtas_data_buf' is of size
> + * 4K, we are adding new local buffer 'rtas_local_data_buf'.

Sorry if this has been covered before but I don't understand why it
would require a larger buffer; by my reading this call will return *ten
bytes* of output. Also, current versions of PAPR+ limit the output
length to 4002 bytes. I feel like I'm missing something.


> + */
> +static __be16 rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
> +
> +/*
> + * read_sys_info_pseries()
> + * Retrieve the number of sockets and chips per socket and cores per
> + * chip details through the get-system-parameter rtas call.
> + */
> +void read_sys_info_pseries(void)
> +{
> +	int call_status, len, ntypes;
> +
> +	/*
> +	 * Making system parameter: chips and sockets and cores per chip
> +	 * default to 1.
> +	 */
> +	phys_sockets = 1;
> +	phys_chipspersocket = 1;
> +	phys_coresperchip = 1;
> +	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH * sizeof(__be16));

Modifying global state outside of any critical section...? How do
you prevent readers from seeing inconsistent results?


> +	spin_lock(&rtas_local_data_buf_lock);
> +
> +	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
> +				NULL,
> +				PROCESSOR_MODULE_INFO,
> +				__pa(rtas_local_data_buf),
> +				PROCESSOR_MAX_LENGTH);
> +
> +	spin_unlock(&rtas_local_data_buf_lock);

Using this lock this way fails to provide any protection to the data
buffer or the phys_* variables.


> +
> +	if (call_status != 0) {
> +		pr_info("Error calling get-system-parameter (0x%x)\n",
> +			call_status);

To be robust, this should handle busy (-2) and extended delay (990x)
statuses. And if it's going to log errors it should use pr_err() and use
decimal, not hex, to report the RTAS call status, since that's how
they're specified in PAPR+.

Re: [PATCH v8 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[kajoljain]

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();
>> +
> 

Re: [PATCH v8 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[kajoljain]

On 5/12/20 2:37 AM, Nathan Lynch wrote:
> Hi,
> 
> Kajol Jain <kjain@linux.ibm.com> writes:
>> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
>> index 48e8f4b17b91..8cf242aad98f 100644
>> --- a/arch/powerpc/perf/hv-24x7.c
>> +++ b/arch/powerpc/perf/hv-24x7.c
>> @@ -20,6 +20,7 @@
>>  #include <asm/io.h>
>>  #include <linux/byteorder/generic.h>
>>  
>> +#include <asm/rtas.h>
>>  #include "hv-24x7.h"
>>  #include "hv-24x7-catalog.h"
>>  #include "hv-common.h"
>> @@ -57,6 +58,75 @@ static bool is_physical_domain(unsigned domain)
>>  	}
>>  }
>>  
>> +/*
>> + * The Processor Module Information system parameter allows transferring
>> + * of certain processor module information from the platform to the OS.
>> + * Refer PAPR+ document to get parameter token value as '43'.
>> + */
>> +
>> +#define PROCESSOR_MODULE_INFO   43
>> +#define PROCESSOR_MAX_LENGTH	(8 * 1024)
>> +
>> +DEFINE_SPINLOCK(rtas_local_data_buf_lock);
>> +EXPORT_SYMBOL(rtas_local_data_buf_lock);
> 
> This should be static and not exported, correct?
> 
>> +
>> +static u32 phys_sockets;	/* Physical sockets */
>> +static u32 phys_chipspersocket;	/* Physical chips per socket*/
>> +static u32 phys_coresperchip; /* Physical cores per chip */
>> +
>> +/*
>> + * Function read_sys_info_pseries() make a rtas_call which require
>> + * data buffer of size 8K. As standard 'rtas_data_buf' is of size
>> + * 4K, we are adding new local buffer 'rtas_local_data_buf'.
> 
> Sorry if this has been covered before but I don't understand why it
> would require a larger buffer; by my reading this call will return *ten
> bytes* of output. Also, current versions of PAPR+ limit the output
> length to 4002 bytes. I feel like I'm missing something.
> 

Hi Nathan,
	Thanks for reviewing the patch. Actually when I was testing this patch in
both power8 and power9 machine, I got some issue in power9 because of buffer size.
And I checked the buffer size used in util_linux which is 8192. So, I increase the
buffer size.I will again test it as I did couple of changes after that with 4002 size. 

> 
>> + */
>> +static __be16 rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
>> +
>> +/*
>> + * read_sys_info_pseries()
>> + * Retrieve the number of sockets and chips per socket and cores per
>> + * chip details through the get-system-parameter rtas call.
>> + */
>> +void read_sys_info_pseries(void)
>> +{
>> +	int call_status, len, ntypes;
>> +
>> +	/*
>> +	 * Making system parameter: chips and sockets and cores per chip
>> +	 * default to 1.
>> +	 */
>> +	phys_sockets = 1;
>> +	phys_chipspersocket = 1;
>> +	phys_coresperchip = 1;
>> +	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH * sizeof(__be16));
> 
> Modifying global state outside of any critical section...? How do
> you prevent readers from seeing inconsistent results?

Yes right, Will update.

> 
> 
>> +	spin_lock(&rtas_local_data_buf_lock);
>> +
>> +	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
>> +				NULL,
>> +				PROCESSOR_MODULE_INFO,
>> +				__pa(rtas_local_data_buf),
>> +				PROCESSOR_MAX_LENGTH);
>> +
>> +	spin_unlock(&rtas_local_data_buf_lock);
> 
> Using this lock this way fails to provide any protection to the data
> buffer or the phys_* variables.
> 
> 
>> +
>> +	if (call_status != 0) {
>> +		pr_info("Error calling get-system-parameter (0x%x)\n",
>> +			call_status);
> 
> To be robust, this should handle busy (-2) and extended delay (990x)
> statuses. And if it's going to log errors it should use pr_err() and use
> decimal, not hex, to report the RTAS call status, since that's how
> they're specified in PAPR+.

Thanks for pointing it, Will update.

Thanks,
Kajol Jain