[PATCH 0/3] powerpc: Instrument Hypervisor Calls

[PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

This patch series adds instrumentation to Hypervisor calls.  Code and
ideas were taken from the patch set provided by Christopher Yeoh.

The idea is to put all instrumentation code behind a configuration
option so that it can easily be added/removed.  The instrumentation
code is kept to a minimum in the hopes that it's impact to performance
will not be noticed.  Statistics are made available via files in sysfs.

-- 
Mike

[PATCH 1/3] powerpc: Instrument Hypervisor Calls: merge headers

[Mike Kravetz]

Move all the Hypervisor call definitions to to a single header file.

-- 
Signed-off-by: Mike Kravetz <kravetz@us.ibm.com>


diff -Naupr linux-2.6.17-rc6/drivers/net/ibmveth.h linux-2.6.17-rc6.work/drivers/net/ibmveth.h
--- linux-2.6.17-rc6/drivers/net/ibmveth.h	2006-06-06 00:57:02.000000000 +0000
+++ linux-2.6.17-rc6.work/drivers/net/ibmveth.h	2006-06-13 21:35:10.000000000 +0000
@@ -41,16 +41,6 @@
 #define IbmVethMcastRemoveFilter     0x2UL
 #define IbmVethMcastClearFilterTable 0x3UL
 
-/* hcall numbers */
-#define H_VIO_SIGNAL             0x104
-#define H_REGISTER_LOGICAL_LAN   0x114
-#define H_FREE_LOGICAL_LAN       0x118
-#define H_ADD_LOGICAL_LAN_BUFFER 0x11C
-#define H_SEND_LOGICAL_LAN       0x120
-#define H_MULTICAST_CTRL         0x130
-#define H_CHANGE_LOGICAL_LAN_MAC 0x14C
-#define H_FREE_LOGICAL_LAN_BUFFER 0x1D4
-
 /* hcall macros */
 #define h_register_logical_lan(ua, buflst, rxq, fltlst, mac) \
   plpar_hcall_norets(H_REGISTER_LOGICAL_LAN, ua, buflst, rxq, fltlst, mac)
diff -Naupr linux-2.6.17-rc6/include/asm-powerpc/hvcall.h linux-2.6.17-rc6.work/include/asm-powerpc/hvcall.h
--- linux-2.6.17-rc6/include/asm-powerpc/hvcall.h	2006-06-06 00:57:02.000000000 +0000
+++ linux-2.6.17-rc6.work/include/asm-powerpc/hvcall.h	2006-06-13 21:35:10.000000000 +0000
@@ -155,9 +155,15 @@
 #define H_VIO_SIGNAL		0x104
 #define H_SEND_CRQ		0x108
 #define H_COPY_RDMA		0x110
+#define H_REGISTER_LOGICAL_LAN	0x114
+#define H_FREE_LOGICAL_LAN	0x118
+#define H_ADD_LOGICAL_LAN_BUFFER 0x11C
+#define H_SEND_LOGICAL_LAN	0x120
+#define H_MULTICAST_CTRL	0x130
 #define H_SET_XDABR		0x134
 #define H_STUFF_TCE		0x138
 #define H_PUT_TCE_INDIRECT	0x13C
+#define H_CHANGE_LOGICAL_LAN_MAC 0x14C
 #define H_VTERM_PARTNER_INFO	0x150
 #define H_REGISTER_VTERM	0x154
 #define H_FREE_VTERM		0x158
@@ -187,11 +193,14 @@
 #define H_GET_HCA_INFO          0x1B8
 #define H_GET_PERF_COUNT        0x1BC
 #define H_MANAGE_TRACE          0x1C0
+#define H_FREE_LOGICAL_LAN_BUFFER 0x1D4
 #define H_QUERY_INT_STATE       0x1E4
 #define H_POLL_PENDING		0x1D8
 #define H_JOIN			0x298
 #define H_ENABLE_CRQ		0x2B0
 
+#define MAX_HCALL_OPCODES	(H_ENABLE_CRQ >> 2)
+
 #ifndef __ASSEMBLY__
 
 /* plpar_hcall() -- Generic call interface using above opcodes

[PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Mike Kravetz]

Add wrappers which perform the actual hypervisor call instrumentation.

-- 
Signed-off-by: Mike Kravetz <kravetz@us.ibm.com>


diff -Naupr linux-2.6.17-rc6/arch/powerpc/Kconfig.debug linux-2.6.17-rc6.work/arch/powerpc/Kconfig.debug
--- linux-2.6.17-rc6/arch/powerpc/Kconfig.debug	2006-06-06 00:57:02.000000000 +0000
+++ linux-2.6.17-rc6.work/arch/powerpc/Kconfig.debug	2006-06-13 21:39:24.000000000 +0000
@@ -18,6 +18,16 @@ config DEBUG_STACK_USAGE
 
 	  This option will slow down process creation somewhat.
 
+config HCALL_STATS
+	bool "Hypervisor call instrumentation"
+	depends on PPC_PSERIES
+	help
+	  Adds code to keep track of number of hypervisor calls made and
+	  the amount of time spent in hypervisor calls.
+
+	  This option will add a small amount of overhead to all hypervisor
+	  calls.
+
 config DEBUGGER
 	bool "Enable debugger hooks"
 	depends on DEBUG_KERNEL
diff -Naupr linux-2.6.17-rc6/arch/powerpc/platforms/pseries/Makefile linux-2.6.17-rc6.work/arch/powerpc/platforms/pseries/Makefile
--- linux-2.6.17-rc6/arch/powerpc/platforms/pseries/Makefile	2006-06-06 00:57:02.000000000 +0000
+++ linux-2.6.17-rc6.work/arch/powerpc/platforms/pseries/Makefile	2006-06-13 21:39:24.000000000 +0000
@@ -9,3 +9,4 @@ obj-$(CONFIG_EEH)	+= eeh.o eeh_cache.o e
 
 obj-$(CONFIG_HVC_CONSOLE)	+= hvconsole.o
 obj-$(CONFIG_HVCS)		+= hvcserver.o
+obj-$(CONFIG_HCALL_STATS)	+= hvCall_inst.o
diff -Naupr linux-2.6.17-rc6/arch/powerpc/platforms/pseries/hvCall.S linux-2.6.17-rc6.work/arch/powerpc/platforms/pseries/hvCall.S
--- linux-2.6.17-rc6/arch/powerpc/platforms/pseries/hvCall.S	2006-06-06 00:57:02.000000000 +0000
+++ linux-2.6.17-rc6.work/arch/powerpc/platforms/pseries/hvCall.S	2006-06-13 21:39:24.000000000 +0000
@@ -25,7 +25,11 @@
 			unsigned long *out2,		R9
 			unsigned long *out3);		R10
  */
+#ifdef CONFIG_HCALL_STATS
+_GLOBAL(plpar_hcall_base)
+#else
 _GLOBAL(plpar_hcall)
+#endif
 	HMT_MEDIUM
 
 	mfcr	r0
@@ -52,7 +56,15 @@ _GLOBAL(plpar_hcall)
 
 
 /* Simple interface with no output values (other than status) */
+#ifdef CONFIG_HCALL_STATS
 _GLOBAL(plpar_hcall_norets)
+	b	plpar_hcall_norets_C
+
+
+_GLOBAL(plpar_hcall_norets_base)
+#else
+_GLOBAL(plpar_hcall_norets)
+#endif
 	HMT_MEDIUM
 
 	mfcr	r0
@@ -76,7 +88,11 @@ _GLOBAL(plpar_hcall_norets)
 			unsigned long arg8,		112(R1)
 			unsigned long *out1);		120(R1)
  */
+#ifdef CONFIG_HCALL_STATS
+_GLOBAL(plpar_hcall_8arg_2ret_base)
+#else
 _GLOBAL(plpar_hcall_8arg_2ret)
+#endif
 	HMT_MEDIUM
 
 	mfcr	r0
@@ -102,7 +118,11 @@ _GLOBAL(plpar_hcall_8arg_2ret)
 		 	unsigned long *out3,		R10
 		 	unsigned long *out4);		112(R1)
  */
+#ifdef CONFIG_HCALL_STATS
+_GLOBAL(plpar_hcall_4out_base)
+#else
 _GLOBAL(plpar_hcall_4out)
+#endif
 	HMT_MEDIUM
 
 	mfcr	r0
@@ -144,7 +164,11 @@ _GLOBAL(plpar_hcall_4out)
 			 unsigned long *out6,		102(R1)
 			 unsigned long *out7);		100(R1)
 */
+#ifdef CONFIG_HCALL_STATS
+_GLOBAL(plpar_hcall_7arg_7ret_base)
+#else
 _GLOBAL(plpar_hcall_7arg_7ret)
+#endif
 	HMT_MEDIUM
 
 	mfcr	r0
@@ -193,7 +217,11 @@ _GLOBAL(plpar_hcall_7arg_7ret)
 			 unsigned long *out8,		 94(R1)
 		         unsigned long *out9,            92(R1)
 */
+#ifdef CONFIG_HCALL_STATS
+_GLOBAL(plpar_hcall_9arg_9ret_base)
+#else
 _GLOBAL(plpar_hcall_9arg_9ret)
+#endif
 	HMT_MEDIUM
 
 	mfcr	r0
diff -Naupr linux-2.6.17-rc6/arch/powerpc/platforms/pseries/hvCall_inst.c linux-2.6.17-rc6.work/arch/powerpc/platforms/pseries/hvCall_inst.c
--- linux-2.6.17-rc6/arch/powerpc/platforms/pseries/hvCall_inst.c	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.17-rc6.work/arch/powerpc/platforms/pseries/hvCall_inst.c	2006-06-13 21:39:24.000000000 +0000
@@ -0,0 +1,239 @@
+/*
+ * Copyright (C) 2006 Mike Kravetz IBM Corporation
+ *
+ * Hypervisor Call Instrumentation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/percpu.h>
+#include <asm/hvcall.h>
+
+DEFINE_PER_CPU(struct hcall_stats[MAX_HCALL_OPCODES+1], hcall_stats);
+
+static inline void update_stats(unsigned long opcode, unsigned long t_before)
+{
+	unsigned long op_index= opcode >> 2;
+	struct hcall_stats *hs = &__get_cpu_var(hcall_stats[op_index]);
+
+	/* racey, but acceptable for non-critical stats */
+	hs->total_time += (mfspr(SPRN_PURR) - t_before);
+	hs->num_calls++;
+}
+
+extern long plpar_hcall_base (unsigned long opcode,
+                 unsigned long arg1,
+                 unsigned long arg2,
+                 unsigned long arg3,
+                 unsigned long arg4,
+                 unsigned long *out1,
+                 unsigned long *out2,
+                 unsigned long *out3);
+
+long plpar_hcall(unsigned long opcode,
+                 unsigned long arg1,
+                 unsigned long arg2,
+                 unsigned long arg3,
+                 unsigned long arg4,
+                 unsigned long *out1,
+                 unsigned long *out2,
+                 unsigned long *out3)
+{
+	long rc;
+	unsigned long t_before = mfspr(SPRN_PURR);
+
+	rc = plpar_hcall_base(opcode, arg1, arg2, arg3, arg4, out1, out2, out3);
+
+	update_stats(opcode, t_before);
+	return rc;
+}
+
+extern long plpar_hcall_norets_base(unsigned long opcode, ...);
+
+long plpar_hcall_norets_C(unsigned long opcode,
+				unsigned long arg1,
+				unsigned long arg2,
+				unsigned long arg3,
+				unsigned long arg4,
+				unsigned long arg5,
+				unsigned long arg6)
+{
+	long rc;
+	unsigned long t_before = mfspr(SPRN_PURR);
+
+	rc = plpar_hcall_norets_base(opcode, arg1, arg2, arg3, arg4, arg5,
+				     arg6);
+
+	update_stats(opcode, t_before);
+	return rc;
+}
+
+extern long plpar_hcall_8arg_2ret_base(unsigned long opcode,
+                           unsigned long arg1,
+                           unsigned long arg2,
+                           unsigned long arg3,
+                           unsigned long arg4,
+                           unsigned long arg5,
+                           unsigned long arg6,
+                           unsigned long arg7,
+                           unsigned long arg8,
+                           unsigned long *out1);
+
+long plpar_hcall_8arg_2ret(unsigned long opcode,
+                           unsigned long arg1,
+                           unsigned long arg2,
+                           unsigned long arg3,
+                           unsigned long arg4,
+                           unsigned long arg5,
+                           unsigned long arg6,
+                           unsigned long arg7,
+                           unsigned long arg8,
+                           unsigned long *out1)
+{
+	long rc;
+	unsigned long t_before = mfspr(SPRN_PURR);
+
+	rc = plpar_hcall_8arg_2ret_base(opcode, arg1, arg2, arg3, arg4, arg5,
+					arg6, arg7, arg8, out1);
+
+	update_stats(opcode, t_before);
+	return rc;
+}
+
+extern long plpar_hcall_4out_base(unsigned long opcode,
+                      unsigned long arg1,
+                      unsigned long arg2,
+                      unsigned long arg3,
+                      unsigned long arg4,
+                      unsigned long *out1,
+                      unsigned long *out2,
+                      unsigned long *out3,
+                      unsigned long *out4);
+
+long plpar_hcall_4out(unsigned long opcode,
+                      unsigned long arg1,
+                      unsigned long arg2,
+                      unsigned long arg3,
+                      unsigned long arg4,
+                      unsigned long *out1,
+                      unsigned long *out2,
+                      unsigned long *out3,
+                      unsigned long *out4)
+{
+	long rc;
+	unsigned long t_before = mfspr(SPRN_PURR);
+
+	rc = plpar_hcall_4out_base(opcode, arg1, arg2, arg3, arg4, out1,
+				   out2, out3, out4);
+
+	update_stats(opcode, t_before);
+	return rc;
+}
+
+extern long plpar_hcall_7arg_7ret_base(unsigned long opcode,
+                           unsigned long arg1,
+                           unsigned long arg2,
+                           unsigned long arg3,
+                           unsigned long arg4,
+                           unsigned long arg5,
+                           unsigned long arg6,
+                           unsigned long arg7,
+                           unsigned long *out1,
+                           unsigned long *out2,
+                           unsigned long *out3,
+                           unsigned long *out4,
+                           unsigned long *out5,
+                           unsigned long *out6,
+                           unsigned long *out7);
+
+long plpar_hcall_7arg_7ret(unsigned long opcode,
+                           unsigned long arg1,
+                           unsigned long arg2,
+                           unsigned long arg3,
+                           unsigned long arg4,
+                           unsigned long arg5,
+                           unsigned long arg6,
+                           unsigned long arg7,
+                           unsigned long *out1,
+                           unsigned long *out2,
+                           unsigned long *out3,
+                           unsigned long *out4,
+                           unsigned long *out5,
+                           unsigned long *out6,
+                           unsigned long *out7)
+{
+	long rc;
+	unsigned long t_before = mfspr(SPRN_PURR);
+
+	rc = plpar_hcall_7arg_7ret_base(opcode, arg1, arg2, arg3, arg4, arg5,
+					arg6, arg7, out1, out2, out3, out4,
+					out5, out6, out7);
+
+	update_stats(opcode, t_before);
+	return rc;
+}
+
+extern long plpar_hcall_9arg_9ret_base(unsigned long opcode,
+                           unsigned long arg1,
+                           unsigned long arg2,
+                           unsigned long arg3,
+                           unsigned long arg4,
+                           unsigned long arg5,
+                           unsigned long arg6,
+                           unsigned long arg7,
+                           unsigned long arg8,
+                           unsigned long arg9,
+                           unsigned long *out1,
+                           unsigned long *out2,
+                           unsigned long *out3,
+                           unsigned long *out4,
+                           unsigned long *out5,
+                           unsigned long *out6,
+                           unsigned long *out7,
+                           unsigned long *out8,
+                           unsigned long *out9);
+
+long plpar_hcall_9arg_9ret(unsigned long opcode,
+                           unsigned long arg1,
+                           unsigned long arg2,
+                           unsigned long arg3,
+                           unsigned long arg4,
+                           unsigned long arg5,
+                           unsigned long arg6,
+                           unsigned long arg7,
+                           unsigned long arg8,
+                           unsigned long arg9,
+                           unsigned long *out1,
+                           unsigned long *out2,
+                           unsigned long *out3,
+                           unsigned long *out4,
+                           unsigned long *out5,
+                           unsigned long *out6,
+                           unsigned long *out7,
+                           unsigned long *out8,
+                           unsigned long *out9)
+{
+	long rc;
+	unsigned long t_before = mfspr(SPRN_PURR);
+
+	rc = plpar_hcall_9arg_9ret_base(opcode, arg1, arg2, arg3, arg4, arg5,
+					arg6, arg7, arg8, arg9, out1, out2,
+					out3, out4, out5, out6, out7, out8,
+					out9);
+
+	update_stats(opcode, t_before);
+	return rc;
+}
diff -Naupr linux-2.6.17-rc6/include/asm-powerpc/hvcall.h linux-2.6.17-rc6.work/include/asm-powerpc/hvcall.h
--- linux-2.6.17-rc6/include/asm-powerpc/hvcall.h	2006-06-13 21:38:45.000000000 +0000
+++ linux-2.6.17-rc6.work/include/asm-powerpc/hvcall.h	2006-06-13 21:39:24.000000000 +0000
@@ -292,6 +292,13 @@ long plpar_hcall_9arg_9ret(unsigned long
 			   unsigned long *out8,
 			   unsigned long *out9);
 
+#ifdef CONFIG_HCALL_STATS
+struct hcall_stats {
+	unsigned long	num_calls;
+	unsigned long	total_time;
+};
+#endif /* CONFIG_HCALL_STATS */
+
 #endif /* __ASSEMBLY__ */
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_HVCALL_H */

[PATCH 0/3] powerpc: Instrument Hypervisor Calls: add sysfs files

[Mike Kravetz]

Make statistics available via files in sysfs.

-- 
Signed-off-by: Mike Kravetz <kravetz@us.ibm.com>


diff -Naupr linux-2.6.17-rc6/arch/powerpc/kernel/sysfs.c linux-2.6.17-rc6.work/arch/powerpc/kernel/sysfs.c
--- linux-2.6.17-rc6/arch/powerpc/kernel/sysfs.c	2006-06-06 00:57:02.000000000 +0000
+++ linux-2.6.17-rc6.work/arch/powerpc/kernel/sysfs.c	2006-06-13 21:42:32.000000000 +0000
@@ -356,6 +356,39 @@ static ssize_t show_physical_id(struct s
 }
 static SYSDEV_ATTR(physical_id, 0444, show_physical_id, NULL);
 
+#ifdef CONFIG_HCALL_STATS
+DECLARE_PER_CPU(struct hcall_stats[MAX_HCALL_OPCODES+1], hcall_stats);
+
+static ssize_t show_hcall_stats(struct sys_device *dev, char *buf)
+{
+	struct cpu *cpu = container_of(dev, struct cpu, sysdev);
+	struct hcall_stats *hs = per_cpu(hcall_stats, cpu->sysdev.id);
+	size_t rc = 0;
+	size_t b_written = 0;
+	size_t b_remain = PAGE_SIZE;
+	unsigned long i;
+
+	for (i=0; i<MAX_HCALL_OPCODES+1; i++){
+		if (!hs[i].num_calls)
+			continue;
+
+		b_written = snprintf(buf+rc, b_remain,
+				"%lu %lu %lu\n",
+				i<<2, hs[i].num_calls, hs[i].total_time);
+
+		if (b_written >= b_remain)
+			break;	/* end of buffer */
+
+		rc += b_written;
+		b_remain -= b_written;
+	}
+
+	return rc;
+}
+
+static SYSDEV_ATTR(hcall_stats, 0666, show_hcall_stats, NULL);
+#endif /* CONFIG_HCALL_STATS */
+
 static int __init topology_init(void)
 {
 	int cpu;
@@ -390,6 +423,9 @@ static int __init topology_init(void)
 			register_cpu(c, cpu, parent);
 
 			sysdev_create_file(&c->sysdev, &attr_physical_id);
+#ifdef CONFIG_HCALL_STATS
+			sysdev_create_file(&c->sysdev, &attr_hcall_stats);
+#endif
 		}
 
 		if (cpu_online(cpu))

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Arnd Bergmann]

T24gV2VkbmVzZGF5IDE0IEp1bmUgMjAwNiAwNTo1MiwgTWlrZSBLcmF2ZXR6IHdyb3RlOgoKPiCg
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1bnNpZ25lZCBsb25nICpvdXQyLKCgoKCgoKCgoKCgoFI5
Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgdW5zaWduZWQgbG9uZyAqb3V0Myk7oKCgoKCgoKCg
oKBSMTAKPiCgICovCj4gKyNpZmRlZiBDT05GSUdfSENBTExfU1RBVFMKPiArX0dMT0JBTChwbHBh
cl9oY2FsbF9iYXNlKQo+ICsjZWxzZQo+IKBfR0xPQkFMKHBscGFyX2hjYWxsKQo+ICsjZW5kaWYK
PiCgoKCgoKCgoEhNVF9NRURJVU0KPiCgCj4goKCgoKCgoKBtZmNyoKCgoHIwCgpUaGUgYXNzZW1i
bHkgZmlsZXMgYXJlIGFscmVhZHkgaGFyZCB0byByZWFkLiBDYW4gd2UgZG8gdGhpcyB3aXRob3V0
CmFuIGV4dHJhICNpZmRlZiBpbiB0aGVyZSBmb3IgZXZlcnkgY2FsbD8KTW9yZW92ZXIsIHdoZW4g
dXNpbmcgY3RhZ3Mgd2l0aCB5b3VyIHZlcnNpb24sIGxvb2tpbmcgZm9yIHBscGFyX2hjYWxsCndp
bGwgZmluZCB0aGUgaW5zdHJ1bWVudGVkIHZlcnNpb24gb25seSwgd2hpY2ggbWF5IGJlIGV4dHJh
IGNvbmZ1c2luZwp3aGVuIHRyeWluZyB0byB3b3JrIG91dCB0aGUgY2FsbCBjaGFpbi4KCj4gZGlm
ZiAtTmF1cHIgbGludXgtMi42LjE3LXJjNi9hcmNoL3Bvd2VycGMvcGxhdGZvcm1zL3BzZXJpZXMv
aHZDYWxsX2luc3QuYyBsaW51eC0yLjYuMTctcmM2LndvcmsvYXJjaC9wb3dlcnBjL3BsYXRmb3Jt
cy9wc2VyaWVzL2h2Q2FsbF9pbnN0LmMKPiAtLS0gbGludXgtMi42LjE3LXJjNi9hcmNoL3Bvd2Vy
cGMvcGxhdGZvcm1zL3BzZXJpZXMvaHZDYWxsX2luc3QuYyAgICAgICAxOTcwLTAxLTAxIDAwOjAw
OjAwLjAwMDAwMDAwMCArMDAwMAo+ICsrKyBsaW51eC0yLjYuMTctcmM2LndvcmsvYXJjaC9wb3dl
cnBjL3BsYXRmb3Jtcy9wc2VyaWVzL2h2Q2FsbF9pbnN0LmMgIDIwMDYtMDYtMTMgMjE6Mzk6MjQu
MDAwMDAwMDAwICswMDAwCi4uLgo+ICtleHRlcm4gbG9uZyBwbHBhcl9oY2FsbF9iYXNlICh1bnNp
Z25lZCBsb25nIG9wY29kZSwKPiArIKAgoCCgIKAgoCCgIKAgoCB1bnNpZ25lZCBsb25nIGFyZzEs
Cj4gKyCgIKAgoCCgIKAgoCCgIKAgdW5zaWduZWQgbG9uZyBhcmcyLAo+ICsgoCCgIKAgoCCgIKAg
oCCgIHVuc2lnbmVkIGxvbmcgYXJnMywKPiArIKAgoCCgIKAgoCCgIKAgoCB1bnNpZ25lZCBsb25n
IGFyZzQsCj4gKyCgIKAgoCCgIKAgoCCgIKAgdW5zaWduZWQgbG9uZyAqb3V0MSwKPiArIKAgoCCg
IKAgoCCgIKAgoCB1bnNpZ25lZCBsb25nICpvdXQyLAo+ICsgoCCgIKAgoCCgIKAgoCCgIHVuc2ln
bmVkIGxvbmcgKm91dDMpOwoKQXQgdGhlIHNhbWUgdGltZSwgdGhlIGRlY2xhcmF0aW9ucyBzaG91
bGQgZ28gdG8gdGhlIGhlYWRlciBmaWxlLCBhcyB0aGV5CmFyZSBhbiBpbnRlcmZhY2UgYmV0d2Vl
biB0d28gZmlsZXMuCgo+ICtleHRlcm4gbG9uZyBwbHBhcl9oY2FsbF9ub3JldHNfYmFzZSh1bnNp
Z25lZCBsb25nIG9wY29kZSwgLi4uKTsKPiArCj4gK2xvbmcgcGxwYXJfaGNhbGxfbm9yZXRzX0Mo
dW5zaWduZWQgbG9uZyBvcGNvZGUsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1
bnNpZ25lZCBsb25nIGFyZzEsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1bnNp
Z25lZCBsb25nIGFyZzIsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1bnNpZ25l
ZCBsb25nIGFyZzMsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1bnNpZ25lZCBs
b25nIGFyZzQsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1bnNpZ25lZCBsb25n
IGFyZzUsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB1bnNpZ25lZCBsb25nIGFy
ZzYpCj4gK3sKPiAroKCgoKCgoGxvbmcgcmM7Cj4gK6CgoKCgoKB1bnNpZ25lZCBsb25nIHRfYmVm
b3JlID0gbWZzcHIoU1BSTl9QVVJSKTsKPiArCj4gK6CgoKCgoKByYyA9IHBscGFyX2hjYWxsX25v
cmV0c19iYXNlKG9wY29kZSwgYXJnMSwgYXJnMiwgYXJnMywgYXJnNCwgYXJnNSwKPiAroKCgoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoCCgIKAgYXJnNik7Cj4gKwo+ICugoKCgoKCgdXBkYXRl
X3N0YXRzKG9wY29kZSwgdF9iZWZvcmUpOwo+ICugoKCgoKCgcmV0dXJuIHJjOwo+ICt9CgpNYXli
ZSBhIGxpdHRsZSBjb21tZW50IGhpbnRpbmcgYWJvdXQgdGhlIG1hZ2ljIGdvaW5nIG9uIHVuZGVy
bmVhdGggdGhpcz8KCj4gZGlmZiAtTmF1cHIgbGludXgtMi42LjE3LXJjNi9pbmNsdWRlL2FzbS1w
b3dlcnBjL2h2Y2FsbC5oIGxpbnV4LTIuNi4xNy1yYzYud29yay9pbmNsdWRlL2FzbS1wb3dlcnBj
L2h2Y2FsbC5oCj4gLS0tIGxpbnV4LTIuNi4xNy1yYzYvaW5jbHVkZS9hc20tcG93ZXJwYy9odmNh
bGwuaKCgoKCgoKAyMDA2LTA2LTEzIDIxOjM4OjQ1LjAwMDAwMDAwMCArMDAwMAo+ICsrKyBsaW51
eC0yLjYuMTctcmM2LndvcmsvaW5jbHVkZS9hc20tcG93ZXJwYy9odmNhbGwuaKCgMjAwNi0wNi0x
MyAyMTozOToyNC4wMDAwMDAwMDAgKzAwMDAKPiBAQCAtMjkyLDYgKzI5MiwxMyBAQCBsb25nIHBs
cGFyX2hjYWxsXzlhcmdfOXJldCh1bnNpZ25lZCBsb25nCj4goKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgIKAgdW5zaWduZWQgbG9uZyAqb3V0OCwKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKAgoCB1
bnNpZ25lZCBsb25nICpvdXQ5KTsKPiCgCj4gKyNpZmRlZiBDT05GSUdfSENBTExfU1RBVFMKPiAr
c3RydWN0IGhjYWxsX3N0YXRzIHsKPiAroKCgoKCgoHVuc2lnbmVkIGxvbmegoKBudW1fY2FsbHM7
Cj4gK6CgoKCgoKB1bnNpZ25lZCBsb25noKCgdG90YWxfdGltZTsKPiArfTsKPiArI2VuZGlmIC8q
IENPTkZJR19IQ0FMTF9TVEFUUyAqLwo+ICsKClRoYXQgcmVhbGx5IHNob3VsZG4ndCBuZWVkICNp
ZmRlZi4gWW91IGNvdWxkIG1vdmUgaXQgdG8gaHZDYWxsX2luc3QuYwp0aG91Z2gsIHNpbmNlIGl0
IGFwcGVhcnMgdG8gYmUgdXNlZCBvbmx5IGluIHRoZXJlIChwYXRjaCAzLzMgaGFzbid0CmNvbWUg
dGhyb3VnaCB5ZXQsIGRvbid0IGtub3cgaWYgaXQncyB1c2VkIGVsc2V3aGVyZSBpbiB0aGF0KS4K
CglBcm5kIDw+PAo=

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls: add sysfs files

[Nathan Lynch]

Hi Mike-

> +#ifdef CONFIG_HCALL_STATS
> +DECLARE_PER_CPU(struct hcall_stats[MAX_HCALL_OPCODES+1], hcall_stats);
> +
> +static ssize_t show_hcall_stats(struct sys_device *dev, char *buf)
> +{
> +	struct cpu *cpu = container_of(dev, struct cpu, sysdev);
> +	struct hcall_stats *hs = per_cpu(hcall_stats, cpu->sysdev.id);
> +	size_t rc = 0;
> +	size_t b_written = 0;
> +	size_t b_remain = PAGE_SIZE;
> +	unsigned long i;
> +
> +	for (i=0; i<MAX_HCALL_OPCODES+1; i++){

Coding style:
                  i < MAX_HCALL_OPCODES + 1

> +		if (!hs[i].num_calls)
> +			continue;
> +
> +		b_written = snprintf(buf+rc, b_remain,
> +				"%lu %lu %lu\n",
> +				i<<2, hs[i].num_calls, hs[i].total_time);
> +
> +		if (b_written >= b_remain)
> +			break;	/* end of buffer */
> +
> +		rc += b_written;
> +		b_remain -= b_written;
> +	}
> +
> +	return rc;
> +}

This flagrantly violates the one-value-per-file rule for sysfs. ;)

Maybe debugfs or some other mechanism would be more appropriate.

Do we not run the risk of having output truncated after the system has
been up for a while and has done a few million hcalls?

> +
> +static SYSDEV_ATTR(hcall_stats, 0666, show_hcall_stats, NULL);

Mode should be 0444 or 0400, no need to have write permissions.

> +#endif /* CONFIG_HCALL_STATS */
> +
>  static int __init topology_init(void)
>  {
>  	int cpu;
> @@ -390,6 +423,9 @@ static int __init topology_init(void)
>  			register_cpu(c, cpu, parent);
>  
>  			sysdev_create_file(&c->sysdev, &attr_physical_id);
> +#ifdef CONFIG_HCALL_STATS
> +			sysdev_create_file(&c->sysdev, &attr_hcall_stats);
> +#endif

Let's not create the file on non-lpar systems.

[PATCH 0/3] powerpc: Instrument Hypervisor Calls: add sysfs files

[Dave C Boutcher]

On Tue, 13 Jun 2006 20:54:20 -0700, Mike Kravetz <kravetz@us.ibm.com> said:
> 
> Make statistics available via files in sysfs.
> -- 
> Signed-off-by: Mike Kravetz <kravetz@us.ibm.com>
> 
> 
> diff -Naupr linux-2.6.17-rc6/arch/powerpc/kernel/sysfs.c linux-2.6.17-rc6.work/arch/powerpc/kernel/sysfs.c
> --- linux-2.6.17-rc6/arch/powerpc/kernel/sysfs.c	2006-06-06 00:57:02.000000000 +0000
> +++ linux-2.6.17-rc6.work/arch/powerpc/kernel/sysfs.c	2006-06-13 21:42:32.000000000 +0000
> @@ -356,6 +356,39 @@ static ssize_t show_physical_id(struct s
>  }
>  static SYSDEV_ATTR(physical_id, 0444, show_physical_id, NULL);
>  
> +#ifdef CONFIG_HCALL_STATS
> +DECLARE_PER_CPU(struct hcall_stats[MAX_HCALL_OPCODES+1], hcall_stats);
> +
> +static ssize_t show_hcall_stats(struct sys_device *dev, char *buf)
> +{
> +	struct cpu *cpu = container_of(dev, struct cpu, sysdev);
> +	struct hcall_stats *hs = per_cpu(hcall_stats, cpu->sysdev.id);
> +	size_t rc = 0;
> +	size_t b_written = 0;
> +	size_t b_remain = PAGE_SIZE;
> +	unsigned long i;
> +
> +	for (i=0; i<MAX_HCALL_OPCODES+1; i++){
> +		if (!hs[i].num_calls)
> +			continue;
> +
> +		b_written = snprintf(buf+rc, b_remain,
> +				"%lu %lu %lu\n",
> +				i<<2, hs[i].num_calls, hs[i].total_time);
> +
> +		if (b_written >= b_remain)
> +			break;	/* end of buffer */
> +
> +		rc += b_written;
> +		b_remain -= b_written;
> +	}
> +
> +	return rc;
> +}
> +

You're violating the "one value per file" rule for sysfs.  Ware the
wrath of Greg KH.

Dave B

[RFC] New hcall mechanism Was: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Jimi Xenidis]

Looking at this patch set, it reminds me how IMNSHO the plpar calling  
mechanism in Linux are a little arcane.
When we were writing "toy" OSes for hypervisors we had the following  
assembly:

### NOTE: this code does not accommodate passing args in r11 & r12  
(arg 8 and 9),
### and 'in' could be used to streamline the fetching of r11 & r12  
from the stack where the ABI has placed them.
#define PAPR(in, out, name, func_code)	\
	_GLOBAL(name);	 		\
         /* profiling code here */		\
	std r3,-GPR_WIDTH(r1); 		\
	li r3,func_code; 		\
	HSC;				\
         /* profiling code here */		\
	ld r12,-GPR_WIDTH(r1);		\
	cmpi 0,r12,0;			\
         bne ret	## out;	/* only store regs if r12 != NULL,  
indicating the caller does not care about the return value */ \
	b ret0

ret7:	std r10, 6 * GPR_WIDTH(r12)
ret6:	std r9, 5 * GPR_WIDTH(r12)
ret5:	std r8, 4 * GPR_WIDTH(r12)
ret4:	std r7, 3 * GPR_WIDTH(r12)
ret3:	std r6, 2 * GPR_WIDTH(r12)
ret2:	std r5, 1 * GPR_WIDTH(r12)
ret1:	std r4, 0 * GPR_WIDTH(r12)
	nop
ret0:	blr
	
PAPR(4, 1,papr_enter, H_ENTER)
...

This allowes for the C interface for H_ENTER to be:
   long papr_enter(ulong *slot, ulong flags, ulong hpte_group, ulong  
hpte_v, ulong hpte_r);

making the call:
   ulong slot;
   lapr_rc = papr_enter(&slot, flags, hpte_group, hpte_v, hpte_r);
Rather than:
   ulong slot, dummy0, dummy1;
   lpar_rc = plpar_hcall(H_ENTER, flags, hpte_group, hpte_v, hpte_r,  
&slot, &dummy0, &dummy1);

The latter saving pointers to stack variables on the stack before the  
call.

A mechanism such as this could also make the profiling far simpler  
and efficient as the profiling code get generated by the PAPR() macro  
above.
I is also possible to generate the prototypes from the PAPR()  
statements above.


Comments welcome?
-JX

On Jun 13, 2006, at 11:47 PM, Mike Kravetz wrote:

> This patch series adds instrumentation to Hypervisor calls.  Code and
> ideas were taken from the patch set provided by Christopher Yeoh.
>
> The idea is to put all instrumentation code behind a configuration
> option so that it can easily be added/removed.  The instrumentation
> code is kept to a minimum in the hopes that it's impact to performance
> will not be noticed.  Statistics are made available via files in  
> sysfs.
>
> -- 
> Mike
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Nathan Lynch]

Mike Kravetz wrote:
> +config HCALL_STATS
> +	bool "Hypervisor call instrumentation"
> +	depends on PPC_PSERIES
> +	help
> +	  Adds code to keep track of number of hypervisor calls made and
> +	  the amount of time spent in hypervisor calls.
> +
> +	  This option will add a small amount of overhead to all hypervisor
> +	  calls.

Would be good to say how the stats are made available to userspace
here.


> +DEFINE_PER_CPU(struct hcall_stats[MAX_HCALL_OPCODES+1], hcall_stats);
> +
> +static inline void update_stats(unsigned long opcode, unsigned long t_before)
> +{
> +	unsigned long op_index= opcode >> 2;
> +	struct hcall_stats *hs = &__get_cpu_var(hcall_stats[op_index]);
> +
> +	/* racey, but acceptable for non-critical stats */
> +	hs->total_time += (mfspr(SPRN_PURR) - t_before);
> +	hs->num_calls++;
> +}
> +
> +extern long plpar_hcall_base (unsigned long opcode,
> +                 unsigned long arg1,
> +                 unsigned long arg2,
> +                 unsigned long arg3,
> +                 unsigned long arg4,
> +                 unsigned long *out1,
> +                 unsigned long *out2,
> +                 unsigned long *out3);
> +
> +long plpar_hcall(unsigned long opcode,
> +                 unsigned long arg1,
> +                 unsigned long arg2,
> +                 unsigned long arg3,
> +                 unsigned long arg4,
> +                 unsigned long *out1,
> +                 unsigned long *out2,
> +                 unsigned long *out3)
> +{
> +	long rc;
> +	unsigned long t_before = mfspr(SPRN_PURR);
> +
> +	rc = plpar_hcall_base(opcode, arg1, arg2, arg3, arg4, out1, out2, out3);
> +
> +	update_stats(opcode, t_before);
> +	return rc;
> +}

Without disabling preemption around the mfspr ... update_stats section
in these hcall wrappers, you risk updating the stats on the wrong cpu.


> +struct hcall_stats {
> +	unsigned long	num_calls;
> +	unsigned long	total_time;
> +};

A comment explaining the unit of time used for the total_time field
would be nice.

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Paul Mackerras]

Nathan Lynch writes:

> Without disabling preemption around the mfspr ... update_stats section
> in these hcall wrappers, you risk updating the stats on the wrong cpu.

I think we are only looking for total counts and times anyway, so it
doesn't really matter which cpu updates the stats, as long as the time
gets accounted on some cpu.  The use of per-cpu counters is just for
better cache behaviour.

Paul.

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Nathan Lynch]

Paul Mackerras wrote:
> Nathan Lynch writes:
> 
> > Without disabling preemption around the mfspr ... update_stats section
> > in these hcall wrappers, you risk updating the stats on the wrong cpu.
> 
> I think we are only looking for total counts and times anyway, so it
> doesn't really matter which cpu updates the stats, as long as the time
> gets accounted on some cpu.  The use of per-cpu counters is just for
> better cache behaviour.

Makes sense... then perhaps the stats shouldn't be exported in per-cpu
files in sysfs, and the kernel should calculate and report the totals
to userspace in some other form.

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Mike Kravetz]

On Wed, Jun 14, 2006 at 05:40:40PM -0500, Nathan Lynch wrote:
> Makes sense... then perhaps the stats shouldn't be exported in per-cpu
> files in sysfs, and the kernel should calculate and report the totals
> to userspace in some other form.

I'm really wondering what would be the best way to make these stats
available to userspace.  As mentioned, putting them into sysfs would
violate the one value/one file rule.  The use of /proc is discouraged.
debugfs was mentioned, but I have never used it.  Noticed that debugfs
was enabled in the default config (at least for pseries).  Looks like
that would be the best alternative.  Agree?

-- 
Mike

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Arnd Bergmann]

On Thursday 15 June 2006 01:52, Mike Kravetz wrote:
> I'm really wondering what would be the best way to make these stats
> available to userspace. =A0As mentioned, putting them into sysfs would
> violate the one value/one file rule. =A0The use of /proc is discouraged.
> debugfs was mentioned, but I have never used it. =A0Noticed that debugfs
> was enabled in the default config (at least for pseries). =A0Looks like
> that would be the best alternative. =A0Agree?

Debugfs sounds good. If you use one file per hcall, it's probably as
easy as

static u64 hcall_stats_get(void *data)
{
	unsigned long index =3D (unsigned long)data;
	return read_hcall_count(index);
}
DEFINE_SIMPLE_ATTRIBUTE(hcall_fops, hcall_stats_get, NULL, "%llu\n");

void hcall_stats_create(void)
{
	unsigned long i;
	struct dentry *dir;

	dir =3D debugfs_create_dir("hcall_stats", NULL);
	for (i=3D0; i <=3D MAX_HCALL_OPCODES; i++) {
		char name[16];
		sprintf(name, "%d", i << 2);
		debugfs_create_file(name, 0444, dir, (void *)i, &hcall_fops);
	}
}

	Arnd <><

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls: add sysfs files

[Christopher Yeoh]

At 2006/6/14 09:22-0500  Nathan Lynch writes:
> > +
> > +static SYSDEV_ATTR(hcall_stats, 0666, show_hcall_stats, NULL);
> 
> Mode should be 0444 or 0400, no need to have write permissions.
> 

The original version zeroed all of the counters if you wrote anything
to the file. This was kind of handy when doing test runs.

Chris
-- 
cyeoh@au.ibm.com
IBM OzLabs Linux Development Group
Canberra, Australia

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Mike Kravetz]

On Thu, Jun 15, 2006 at 01:09:39PM +0200, Arnd Bergmann wrote:
> On Thursday 15 June 2006 01:52, Mike Kravetz wrote:
> > I'm really wondering what would be the best way to make these stats
> > available to userspace.  As mentioned, putting them into sysfs would
> > violate the one value/one file rule.  The use of /proc is discouraged.
> > debugfs was mentioned, but I have never used it.  Noticed that debugfs
> > was enabled in the default config (at least for pseries).  Looks like
> > that would be the best alternative.  Agree?
> 
> Debugfs sounds good. If you use one file per hcall, it's probably as
> easy as

But does debugfs have the same one value/one file rule?  Thought I could
dump all the data in a single debugfs file.

-- 
Mike

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Arnd Bergmann]

On Thursday 15 June 2006 16:58, Mike Kravetz wrote:
> But does debugfs have the same one value/one file rule? =A0Thought I could
> dump all the data in a single debugfs file.

Sure, you could. It just makes it a little more complicated. You probably
need to use a seq_file then.

	Arnd <><

Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Mike Kravetz]

On Thu, Jun 15, 2006 at 08:33:55AM +1000, Paul Mackerras wrote:
> Nathan Lynch writes:
> 
> > Without disabling preemption around the mfspr ... update_stats section
> > in these hcall wrappers, you risk updating the stats on the wrong cpu.
> 
> I think we are only looking for total counts and times anyway, so it
> doesn't really matter which cpu updates the stats, as long as the time
> gets accounted on some cpu.  The use of per-cpu counters is just for
> better cache behaviour.

Thought about this a little more.

We do an mfspr to get a 'timestamp' before the actual hcall.  Then, make
the hcall and do another mfspr after.  Isn't it possible to be preempted
and perform the mfspr's on separate CPUs?  Worse yet, wouldn't this possibly
add 'time preempted' to the hcall time?.  Do we have the same (skewed time)
issue with interrupts?

Thinking that enable/disable around the call might be worth the overhead.
But, disabling interrupts would not be worth it.

-- 
Mike

[PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

Here is an updated version of the hcall instrumentation patches.
Unfortunately, I have not been able to spend as much time as I
would like on this (and it seems that will continue).  

This version addresses all comments received except Arnd's issue
with an #ifdef for each function in the assembly file.  I like
Jimi's suggestion for creating macros(which could also minimize
the #ifdefs), but have not got around to implementing this.
Suggestions of others way to accomplish this are welcome.

Statistic files are moved to debugfs, and remain split out by CPU.
After some thought, disable/enable of preemption was added around
the statistic gathering(mostly for timing).

-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Segher Boessenkool]

> Statistic files are moved to debugfs, and remain split out by CPU.

Having done some if this myself long ago, I really like to see these
per-cpu statistics.  "Not useful" in real life, but for most testcases
they're invaluable.  It would be useful to also add a "total" column
as well, of course, but it can also be done in userland.


Segher

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

On Thu, Jun 22, 2006 at 03:56:09PM -0700, Mike Kravetz wrote:
> This version addresses all comments received except Arnd's issue
> with an #ifdef for each function in the assembly file.

I was thinking of changing the names of all the assembly routines from
plpar_hcall_*() to plpar_hcall_*_asm().  The instrumented version of the
routines would be named plpar_hcall_*_inst().  Then, the header file
would contain definitions such as:

#ifdef CONFIG_HCALL_STATS
#define plpar_hcall_*()		plpar_hcall_*_inst()
.
#else
#define plpar_hcall_*()		plpar_hcall_*_asm()
.
#endif

Is that any better than all the individual #ifdefs in the .S file?  Is it
still too ugly?

I'm open to any suggestions.

Thanks,
-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Arnd Bergmann]

On Monday 10 July 2006 22:35, Mike Kravetz wrote:
> On Thu, Jun 22, 2006 at 03:56:09PM -0700, Mike Kravetz wrote:
> > This version addresses all comments received except Arnd's issue
> > with an #ifdef for each function in the assembly file.
> 
> I was thinking of changing the names of all the assembly routines from
> plpar_hcall_*() to plpar_hcall_*_asm().  The instrumented version of the
> routines would be named plpar_hcall_*_inst().  Then, the header file
> would contain definitions such as:
> 
> #ifdef CONFIG_HCALL_STATS
> #define plpar_hcall_*()		plpar_hcall_*_inst()
> .
> #else
> #define plpar_hcall_*()		plpar_hcall_*_asm()
> .
> #endif
> 
> Is that any better than all the individual #ifdefs in the .S file?  Is it
> still too ugly?
> 

I guess it's better to have the #ifdef in the header file, but then
again, you could just as well save some source lines doing

#ifndef CONFIG_HCALL_STATS
#define plpar_hcalldef(x) plpar_call_ ## x ## _asm
#else
#define plpar_hcalldef(x) plpar_call_ ## x ## _inst
#endif

#define plpar_call_foo plpar_hcalldef(foo)

	Arnd <><

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

On Mon, Jul 10, 2006 at 01:35:10PM -0700, Mike Kravetz wrote:
> #ifdef CONFIG_HCALL_STATS
> #define plpar_hcall_*()		plpar_hcall_*_inst()

To answer my own question, this won't work because plpar_hcall_*()
routines are exported.  So, marcros can't be used.  Still thinking
about the best way to juggle function names for instrumentation.

-- 
Mike

[PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

Hi Paul,

Here is an updated version of the patch(es) to instrument hcalls.  All
issues from the previous versions have been addressed.  Although, I
haven't really discovered an elegant solution to the assembly routine
name juggling.

In addition, there has been some discussion of these patches on IRC.
Some remaining issues/questions are:
- Exactly how much overhead does the statistic gathering introduce?
- What would be the cost of disabling preemption for more accurate statistics?
- What would be the cost of disabling interrupts for more accurate statistics?
- Should we extend this statistic gathering to RTAS calls?

I would like to get this basic code integrated and then start to address
some of these questions.  The IBM performance group will help in
benchmarking to determine overhead.

Please note that all code is behind a config option (off by default) with
zero impact unless enabled.

-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Arnd Bergmann]

On Saturday 15 July 2006 01:37, Mike Kravetz wrote:
> In addition, there has been some discussion of these patches on IRC.
> Some remaining issues/questions are:
> - Exactly how much overhead does the statistic gathering introduce?
> - What would be the cost of disabling preemption for more accurate statistics?
> - What would be the cost of disabling interrupts for more accurate statistics?
> - Should we extend this statistic gathering to RTAS calls?

What happened to the question whether to use PURR values for also measuring
cycles spent executing the hcall as opposed to cycles that passed before
the hcall returns. Did that turn out not giving extra information after all
or was there a different reason to drop that idea?

	Arnd <><

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

On Sat, Jul 15, 2006 at 02:00:02AM +0200, Arnd Bergmann wrote:
> What happened to the question whether to use PURR values for also measuring
> cycles spent executing the hcall as opposed to cycles that passed before
> the hcall returns. Did that turn out not giving extra information after all
> or was there a different reason to drop that idea?

Oops, forgot that as an additional issue/question.  In this patch, I went
back to mftb() as 'wall time' made more sense for the group wanting this
functionality.  It is easy to switch, or collect both (I think).  Since I
started with mftb went to PURR and then back to mftb, it certainly does
look like an agument to try and collect both. :)

-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Anton Blanchard]

 
> What happened to the question whether to use PURR values for also measuring
> cycles spent executing the hcall as opposed to cycles that passed before
> the hcall returns. Did that turn out not giving extra information after all
> or was there a different reason to drop that idea?

You have to be careful with PURR since it may be context switched
between partitions.

Anton

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Arnd Bergmann]

On Saturday 15 July 2006 17:30, Anton Blanchard wrote:
> > What happened to the question whether to use PURR values for also measuring
> > cycles spent executing the hcall as opposed to cycles that passed before
> > the hcall returns. Did that turn out not giving extra information after all
> > or was there a different reason to drop that idea?
> 
> You have to be careful with PURR since it may be context switched
> between partitions.
> 

Not sure I follow you. I would expect the PURR value to be restored after
a context switch, even if we continue on a different physical CPU.

The idea behind monitoring both PURR and timebase is that the difference
between the two tells you how long the partition was suspended during
the hcall.

	Arnd <><

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Anton Blanchard]

 
> Not sure I follow you. I would expect the PURR value to be restored after
> a context switch, even if we continue on a different physical CPU.
> 
> The idea behind monitoring both PURR and timebase is that the difference
> between the two tells you how long the partition was suspended during
> the hcall.

Sounds good, last time I looked at the patch I thought it was gathering
the PURR only. That on its own would make for some confusing results.

Anton

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Olof Johansson]

On Sat, Jul 15, 2006 at 02:00:02AM +0200, Arnd Bergmann wrote:

> What happened to the question whether to use PURR values for also measuring
> cycles spent executing the hcall as opposed to cycles that passed before
> the hcall returns. Did that turn out not giving extra information after all
> or was there a different reason to drop that idea?

Just so people don't forget: this can't be done on all processors. For
example, PPC970 and POWER4 don't implement the PURR SPR. And it doesn't
make sense to use H_PURR to get the software emulated ones there. Not
really an issue on POWER4 since they don't do shared processor LPAR, but
on JS21 I think they might do?


-Olof

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Luke Browning]

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

linuxppc-dev-bounces+lukebrowning=us.ibm.com@ozlabs.org wrote on 
07/16/2006 12:53:54 AM:

> On Sat, Jul 15, 2006 at 02:00:02AM +0200, Arnd Bergmann wrote:
> 
> > What happened to the question whether to use PURR values for also 
measuring
> > cycles spent executing the hcall as opposed to cycles that passed 
before
> > the hcall returns. Did that turn out not giving extra information 
after all
> > or was there a different reason to drop that idea?
> 
> Just so people don't forget: this can't be done on all processors. For
> example, PPC970 and POWER4 don't implement the PURR SPR. And it doesn't
> make sense to use H_PURR to get the software emulated ones there. Not
> really an issue on POWER4 since they don't do shared processor LPAR, but
> on JS21 I think they might do?
> 
Isn't there someway to do a platform specific processor overlay, where you 

get PURR if it exists and TB otherwise.

Luke

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

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Luke Browning]

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

linuxppc-dev-bounces+lukebrowning=us.ibm.com@ozlabs.org wrote on 
07/15/2006 07:07:58 PM:

> 
> > Not sure I follow you. I would expect the PURR value to be restored 
after
> > a context switch, even if we continue on a different physical CPU.
> > 
> > The idea behind monitoring both PURR and timebase is that the 
difference
> > between the two tells you how long the partition was suspended during
> > the hcall.
> 
> Sounds good, last time I looked at the patch I thought it was gathering
> the PURR only. That on its own would make for some confusing results.
>

It would be more efficient to have a separate trace log for PHYP 
dispatches as 
there are many more hypervisor calls than PHYP dispatches.  I believe PHYP 
provides
a trace records for dispatches, which could be seperately written.  Perf 
tools can 
assemble the information from multiple records.  It would be nice to have 
processor
specific time function that reads either PURR or TB based on the platform.

Luke


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

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Olof Johansson]

On Sun, Jul 16, 2006 at 08:53:21PM -0200, Luke Browning wrote:

> > Just so people don't forget: this can't be done on all processors. For
> > example, PPC970 and POWER4 don't implement the PURR SPR. And it doesn't
> > make sense to use H_PURR to get the software emulated ones there. Not
> > really an issue on POWER4 since they don't do shared processor LPAR, but
> > on JS21 I think they might do?
> > 
> Isn't there someway to do a platform specific processor overlay, where you 
> get PURR if it exists and TB otherwise.

Sure, it's what we have cpu feature bits for. It's not a difficult
problem to solve, I was just reminding of the lack of SPRN_PURR on some
processors that can run hypervisors.


-Olof

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Luke Browning]

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

linuxppc-dev-bounces+lukebrowning=us.ibm.com@ozlabs.org wrote on 
07/15/2006 12:30:54 PM:

> 
> > What happened to the question whether to use PURR values for also 
measuring
> > cycles spent executing the hcall as opposed to cycles that passed 
before
> > the hcall returns. Did that turn out not giving extra information 
after all
> > or was there a different reason to drop that idea?
> 
> You have to be careful with PURR since it may be context switched
> between partitions.
> 

That shouldn't be an issue as the PURR is supposed to be virtualized by 
PHYP at 
the virtual processor level.

Luke

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

[PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

A small update from the last version.  By popular demand, both
wall time (mftb) and cpu cycles(PURR) are collected for each call.
It is interesting to see these two values side by side in the
output files.

-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Paul Mackerras]

Mike Kravetz writes:

> A small update from the last version.  By popular demand, both
> wall time (mftb) and cpu cycles(PURR) are collected for each call.
> It is interesting to see these two values side by side in the
> output files.

Did you see the patch from Anton that I posted a week or so ago, which
reduces the number of plpar_hcall_* functions?  I'd rather the
instrumentation stuff was based on that.

My other comment is this: wouldn't it actually turn out simpler if we
read the timebase (and PURR, if we really want to do that too) in the
assembly code that implements plpar_hcall_*?

Paul.

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

On Tue, Jul 18, 2006 at 01:47:23PM -0700, Mike Kravetz wrote:
> A small update from the last version.  By popular demand, both
> wall time (mftb) and cpu cycles(PURR) are collected for each call.
> It is interesting to see these two values side by side in the
> output files.

Arnd asked on IRC, so I thought others may be interested in a
sample output file.  Remember, there is one file per CPU. All
values are decimal.  First number is hcall # (in decimal), second
is number of calls, third is total time (mftb) and fourth is cpu
time (PURR).  Note that 'total times' are still in cycles and
can be converted to another format in user space.

[elm3b125]/sys/kernel/debug/hcall_inst >cat cpu0
4 140651 21538230 14456409
8 130064 11855094 9459178
24 14259 1496316 986702
32 1809 354584 315399
84 103 27196 17763
88 1046 108106 109322
100 2596 251229 205154
104 1 1040 1061
108 2208 320491 242708
116 2669 1030894 713272
220 2 532 510
224 130662 47508588184 169383891
228 5 2689902 4622
260 3018 261893 230753

-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Olof Johansson]

On Tue, Jul 18, 2006 at 02:29:13PM -0700, Mike Kravetz wrote:

> 88 1046 108106 109322
[...]
> 104 1 1040 1061

Heh. Shouldn't PURR/TB be <= 1 at all times? :-) I bet this is because
they're not sampled at exactly the same time, etc.

Maybe recoding it to assembly so you can do mftb and mfspr right after
each other, and do the arithmetics/stores later could help. Inline asm
might be sufficient.


-Olof

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

On Wed, Jul 19, 2006 at 07:00:22AM +1000, Paul Mackerras wrote:
> Mike Kravetz writes:
> > A small update from the last version.  By popular demand, both
> > wall time (mftb) and cpu cycles(PURR) are collected for each call.
> > It is interesting to see these two values side by side in the
> > output files.
> 
> Did you see the patch from Anton that I posted a week or so ago, which
> reduces the number of plpar_hcall_* functions?  I'd rather the
> instrumentation stuff was based on that.

Agreed.  I missed the patch from last week, but saw the one from Anton
that was posted today.  Future instrumentation patches will assume this
change.

> My other comment is this: wouldn't it actually turn out simpler if we
> read the timebase (and PURR, if we really want to do that too) in the
> assembly code that implements plpar_hcall_*?

I'll give that a try.  My assembly skills are a bit rusty, so it may
take a little longer to produce something.

-- 
Mike

Re: [PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

On Tue, Jul 18, 2006 at 08:36:18PM -0700, Mike Kravetz wrote:
> > My other comment is this: wouldn't it actually turn out simpler if we
> > read the timebase (and PURR, if we really want to do that too) in the
> > assembly code that implements plpar_hcall_*?
> 
> I'll give that a try.  My assembly skills are a bit rusty, so it may
> take a little longer to produce something.

Getting the timebase and PURR in assembly is easy enough.  So, I thought
about updating the per-cpu statistics while in there.  But, I couldn't
find any other assembly code that does this.  Sure there is asm code that
updated fields in the PACA, but none that messes with paca.data_offset
(that I could find).

Should the statistic updates also be done in the assembly routines?  Or,
how about a call out to a C routine for this?

-- 
Mike

[PATCH 0/3] powerpc: Instrument Hypervisor Calls

[Mike Kravetz]

Here is a version of the instrumentation patches built on top of
Anton's hcall cleanup patch.  In addition, I have gathered the
timebase and PURR snapshots in assembly code.  However, it still
performs a call out to C code that updates the data structures.

I'd appreciate some comments on the assembly code as it has been
a looooong time since I've written any.

-- 
Mike