powerpc: Instrument Hypervisor Calls
Subsystems (2)
cover powerpc: Instrument Hypervisor Calls
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.
Signature · 1 line
Mike
PATCH 1/3 powerpc: Instrument Hypervisor Calls: merge headers
Move all the Hypervisor call definitions to to a single header file. --
drivers/net/ibmveth.h +0 -10
@@ -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)
include/asm-powerpc/hvcall.h +9 -0
@@ -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
In reply to Mike KravetzAdd wrappers which perform the actual hypervisor call instrumentation. --
arch/powerpc/Kconfig.debug +13 -0
@@ -18,6 +18,19 @@ config DEBUG_STACK_USAGE This option will slow down process creation somewhat. +config HCALL_STATS + bool "Hypervisor call instrumentation" + depends on PPC_PSERIES && DEBUG_FS + help + Adds code to keep track of the number of hypervisor calls made and + the amount of time spent in hypervisor calls. A directory named + hcall_inst is added at the root of the debugfs filesystem. Within + the hcall_inst directory are files that contain CPU specific call + statistics. + + This option will add a small amount of overhead to all hypervisor + calls. + config DEBUGGER bool "Enable debugger hooks" depends on DEBUG_KERNEL
arch/powerpc/platforms/pseries/Makefile +1 -0
@@ -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
arch/powerpc/platforms/pseries/hvCall.S +35 -7
84 lines not on this page · raw message
arch/powerpc/platforms/pseries/hvCall_inst.c +207 -0
207 lines not on this page · raw message
include/asm-powerpc/hvcall.h +80 -0
86 lines not on this page · raw message
Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers
Quoted 20 lines · Mike Kravetz
arch/powerpc/platforms/pseries/hvCall_inst.c +20
-0+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]); + + hs->total_time += (mftb () - t_before); + hs->num_calls++; +} + +/* +* plpar_hcall wrapper +*/ +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)
arch/powerpc/platforms/pseries/hvCall_inst.c line 63
Quoted 10 lines
arch/powerpc/platforms/pseries/hvCall_inst.c +10
-0+{ +long rc; +unsigned long t_before; + + t_before =mftb (); + rc =plpar_hcall_base (opcode, arg1, arg2, arg3, arg4, out1, out2, out3); + +update_stats (opcode, t_before); +return rc; +}
Hmm, isn't it possible that the stats could be corrupted if we process an interrupt/softirq which does an hcall while the stats are being updated? Maybe it's not a show-stopper, but it seems to me that accuracy could suffer under some workloads.
Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers
Quoted 4 lines · Nathan Lynch
Hmm, isn't it possible that the stats could be corrupted if we process an interrupt/softirq which does an hcall while the stats are being updated? Maybe it's not a show-stopper, but it seems to me that accuracy could suffer under some workloads.
Yes, they can become inaccurate. Is this what you mean by 'corrupted' or you thinking of something worse?
There is going to be a trade off between accuracy and the overhead of the data gathering. What really needs to happen is some good benchmarking to measure the costs of different approaches. I'm pretty sure I can get someone to provide such data.
In this patch, I have minimized the overhead in data collection at the cost of accuracy.
Signature · 1 line
Mike
Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers
Quoted 8 lines · Mike Kravetz
Quoted 4 lines · Nathan Lynch
Hmm, isn't it possible that the stats could be corrupted if we process an interrupt/softirq which does an hcall while the stats are being updated? Maybe it's not a show-stopper, but it seems to me that accuracy could suffer under some workloads.
Yes, they can become inaccurate. Is this what you mean by 'corrupted' or you thinking of something worse?
Yes, "corrupted" was perhaps too strong, but we could lose the accounting for hcalls from interrupts that we take at exactly the wrong time. I guess it's a pretty tiny risk.
I just wanted to make sure this had been considered, but if you're comfortable with the overhead/accuracy tradeoff, then okay.
PATCH 3/3 powerpc: Instrument Hypervisor Calls: add debugfs files
In reply to Mike KravetzMake statistics available via files in debugfs.
--
arch/powerpc/platforms/pseries/hvCall_inst.c +91 -0
@@ -205,3 +205,94 @@ long plpar_hcall_9arg_9ret(unsigned longupdate_stats (opcode, t_before);return rc; } + +/* +* Routines for displaying the statistics in debugfs +*/ +static void *hc_start (struct seq_file *m,loff_t *pos) +{ +if ((int )*pos <MAX_HCALL_OPCODES ) +return (void *)(unsigned long )(*pos +1 ); + +return NULL ; +} + +static void *hc_next (struct seq_file *m,void *p,loff_t * pos) +{ + ++*pos; + +return hc_start (m, pos); +} + +static void hc_stop (struct seq_file *m,void *p) +{ +} + +static int hc_show (struct seq_file *m,void *p) +{ +unsigned long h_num = (unsigned long )p; +struct hcall_stats *hs = (struct hcall_stats *)m->private; + +if (hs[h_num].num_calls) +seq_printf (m,"%lu %lu %lu\n" , h_num<<2 , hs[h_num].num_calls, + hs[h_num].total_time); + +return 0 ; +} + +static struct seq_operations hcall_inst_seq_ops = { + .start = hc_start, + .next = hc_next, + .stop = hc_stop, + .show = hc_show +}; + +static int hcall_inst_seq_open (struct inode *inode,struct file *file) +{ +int rc; +struct seq_file *seq; + + rc =seq_open (file, &hcall_inst_seq_ops); + seq = file->private_data; + seq->private = file->f_dentry->d_inode->u.generic_ip; + +return rc; +} + +static struct file_operations hcall_inst_seq_fops = { + .open = hcall_inst_seq_open, + .read = seq_read, + .llseek = seq_lseek, + .release = seq_release, +}; + +#define HCALL_ROOT_DIR "hcall_inst" +#define CPU_NAME_BUF_SIZE 32 + +static int __init hcall_inst_init (void ) +{ +struct dentry *hcall_root; +struct dentry *hcall_file; +char cpu_name_buf[CPU_NAME_BUF_SIZE ]; +int cpu; + +if (!firmware_has_feature (FW_FEATURE_LPAR )) +return 0 ; + + hcall_root =debugfs_create_dir (HCALL_ROOT_DIR ,NULL ); +if (!hcall_root) +return -ENOMEM ; + +for_each_cpu (cpu) { +snprintf (cpu_name_buf,CPU_NAME_BUF_SIZE ,"cpu%d" , cpu); + hcall_file =debugfs_create_file (cpu_name_buf,S_IRUGO , + hcall_root, +per_cpu (hcall_stats, cpu), + &hcall_inst_seq_fops); +if (!hcall_file) +return -ENOMEM ; + } + +return 0 ; +} +__initcall (hcall_inst_init);
Quoted 6 lines · Mike Kravetz
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 <><
Quoted 4 lines · Arnd Bergmann
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. :)
Signature · 1 line
Mike
Quoted 5 lines
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
Quoted 8 lines · Anton Blanchard
Quoted 4 lines
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 <><
Quoted 7 lines
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
[email protected] wrote on 07/15/2006 07:07:58 PM:
Quoted 2 lines
Quoted 2 lines
Not sure I follow you. I would expect the PURR value to be restored
after
Quoted 3 lines
Quoted 3 lines
a context switch, even if we continue on a different physical CPU.
The idea behind monitoring both PURR and timebase is that the
difference
Quoted 6 lines
Quoted 2 lines
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
[email protected] wrote on 07/15/2006 12:30:54 PM:
Quoted 2 lines
Quoted 2 lines
What happened to the question whether to use PURR values for also
measuring
Quoted 1 line
Quoted 1 line
cycles spent executing the hcall as opposed to cycles that passed
before
Quoted 1 line
Quoted 1 line
the hcall returns. Did that turn out not giving extra information
after all
Quoted 5 lines
Quoted 1 line
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
Quoted 4 lines · Arnd Bergmann
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
[email protected] wrote on 07/16/2006 12:53:54 AM:
Quoted 3 lines · Arnd Bergmann
Quoted 2 lines
What happened to the question whether to use PURR values for also
measuring
Quoted 1 line
Quoted 1 line
cycles spent executing the hcall as opposed to cycles that passed
before
Quoted 1 line
Quoted 1 line
the hcall returns. Did that turn out not giving extra information
after all
Quoted 8 lines
Quoted 1 line
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_PURRto 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
Quoted 8 lines · Luke Browning
Quoted 6 lines
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_PURRto 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