Skip to contentSkip to conversation

kvmppc: add detailed instruction

PATCH In review [email protected] 2 participants 4 messages Apr 14, 2008 – Apr 15, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/3 kvmppc: add detailed instruction

From: Christian Ehrhardt <[email protected]>

This extends the kvm_stat reports for kvm on embedded powerpc. Since kvmppc is using emulation (no hardware support yet) this gives people interested in performance a detailed split of the emul_instruction counter already available. This statistic does not cover e.g. operants of the commands, but that way it should have only a small perf impact (never break what you want to measure). This feature is configurable in .config under the kvmppc virtualization itself.

---

[diffstat]

 arch/powerpc/kvm/Kconfig       |    8 +++
 arch/powerpc/kvm/emulate.c     |   61 +++++++++++++++++++++++++++++
 arch/powerpc/kvm/powerpc.c     |   84 +++++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/kvm_host.h |   40 +++++++++++++++++++
 include/asm-powerpc/kvm_ppc.h  |   50 ++++++++++++++++++++++++
 5 files changed, 243 insertions(+)
arch/powerpc/kvm/Kconfig +8 -0
@@ -41,6 +41,14 @@ config KVM_POWERPC_440
 	---help---
 	  Provides support for KVM on 440 processors.
 
+config KVM_POWERPC_440_INSTRUCTION_STAT
+	bool "ppc440 instruction emulation statistics"
+	depends on KVM && 44x && KVM_POWERPC_440
+	---help---
+	  Adds the tracking of the different emulated instructions. This is
+	  used to debug performance issues and adds a slight runtime overhead.
+	  If unsure, say N.
+
 config KVM_PPC_VIRTIO
        bool "Virtio Support"
        select VIRTIO
arch/powerpc/kvm/emulate.c +61 -0

282 lines not on this page · raw message

arch/powerpc/kvm/powerpc.c +84 -0

92 lines not on this page · raw message

include/asm-powerpc/kvm_host.h +40 -0
@@ -59,6 +59,46 @@ struct kvm_vcpu_stat {
 	u32 emulated_inst_exits;
 	u32 dec_exits;
 	u32 ext_intr_exits;
+#ifdef CONFIG_KVM_POWERPC_440_INSTRUCTION_STAT
+	u32 instr_trap;
+	u32 instr_rfi;
+	u32 instr_mfmsr;
+	u32 instr_lbzx;
+	u32 instr_wrtee;
+	u32 instr_mtmsr;
+	u32 instr_wrteei;
+	u32 instr_stbx;
+	u32 instr_stbux;
+	u32 instr_lhzx;
+	u32 instr_lhzux;
+	u32 instr_mfdcr;
+	u32 instr_mfspr;
+	u32 instr_sthx;
+	u32 instr_sthux;
+	u32 instr_mtdcr;
+	u32 instr_mtspr;
+	u32 instr_dcbi;
+	u32 instr_lwbrx;
+	u32 instr_tlbsync;
+	u32 instr_stwbrx;
+	u32 instr_tlbwe;
+	u32 instr_tlbsx;
+	u32 instr_lhbrx;
+	u32 instr_sthbrx;
+	u32 instr_iccci;
+	u32 instr_lwz;
+	u32 instr_lwzu;
+	u32 instr_lbz;
+	u32 instr_lbzu;
+	u32 instr_stw;
+	u32 instr_stwu;
+	u32 instr_stb;
+	u32 instr_stbu;
+	u32 instr_lhz;
+	u32 instr_lhzu;
+	u32 instr_sth;
+	u32 instr_sthu;
+#endif /* CONFIG_KVM_POWERPC_440_INSTRUCTION_STAT */
 };
 
 struct tlbe {
include/asm-powerpc/kvm_ppc.h +50 -0

56 lines not on this page · raw message

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

#1 lore raw
Quoted 9 lines · On Monday 14 April 2008 07:23:56 [email protected] wrote:

From: Christian Ehrhardt <[email protected]>

This extends the kvm_stat reports for kvm on embedded powerpc. Since kvmppc is using emulation (no hardware support yet) this gives people interested in performance a detailed split of the emul_instruction counter already available. This statistic does not cover e.g. operants of the commands, but that way it should have only a small perf impact (never break what you want to measure). This feature is configurable in .config under the kvmppc virtualization itself.

This array-based approach seems to add a lot of lines of code, and it's also copy/paste stuff that is just begging for a typo (e.g. miscounting STHX in the STHUX bucket) or being forgotten entirely when adding new emulation code.

A more general approach would be to record just the opcode/extended opcode in a variable-sized structure, allocating a new bucket as we encounter a previously unseen instruction. I'm thinking of something like this:

log_instruction(inst) {
	bucket = hash_u32(inst, 5);
	list_for_each(l, vcpu->instlog[bucket]) {
		struct instlog = list_entry(l);
		if (instlog->inst = inst) {
			instlog->count++;
			break;
		}
	}
}
emulate(inst) {
	log = get_op(inst);
	switch (get_op(inst)) {
		...
	case 31:
		log |= get_xop(inst);
		switch (inst) {
			...
		}
	}
	log_instruction(log);
}

It looks like we could build a hash table pretty easily with hash_long() and list_entry stuff (see fs/mbcache.c for example). So far you've found 17 different instructions types emulated in the "boot" workload, so 32 entries would probably be a reasonable hash size. The same approach could be used for SPR accesses, where you've hit 31 different registers. Really I think those numbers won't vary much by workload, but rather by guest kernel...

Signature · 12 lines
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel
#2 lore raw
In reply to [email protected]
Quoted 11 lines · Hollis Blanchard
Quoted 9 lines · On Monday 14 April 2008 07:23:56 [email protected] wrote:

From: Christian Ehrhardt <[email protected]>

This extends the kvm_stat reports for kvm on embedded powerpc. Since kvmppc is using emulation (no hardware support yet) this gives people interested in performance a detailed split of the emul_instruction counter already available. This statistic does not cover e.g. operants of the commands, but that way it should have only a small perf impact (never break what you want to measure). This feature is configurable in .config under the kvmppc virtualization itself.

Quoted 7 lines

This array-based approach seems to add a lot of lines of code, and it's also copy/paste stuff that is just begging for a typo (e.g. miscounting STHX in the STHUX bucket) or being forgotten entirely when adding new emulation code.

A more general approach would be to record just the opcode/extended opcode in a variable-sized structure, allocating a new bucket as we encounter a previously unseen instruction. I'm thinking of something like this:

I already thought about some bucket like counting, but I focused small runtime overhead above everything else here by hijacking the already existent switch/case and avoiding any new if's & function calls. I agree that the array like solution is messy in the aspect of lines and is more error prone by forgetting something.

Quoted 11 lines
log_instruction(inst) {
	bucket = hash_u32(inst, 5);
	list_for_each(l, vcpu->instlog[bucket]) {
		struct instlog = list_entry(l);
		if (instlog->inst = inst) {
			instlog->count++;
			break;
		}
	}
}
Quoted 12 lines
emulate(inst) {
	log = get_op(inst);
	switch (get_op(inst)) {
		...
	case 31:
		log |= get_xop(inst);
		switch (inst) {
			...
		}
	}
	log_instruction(log);
}

I agree that this looks better, but is has per instruction:
1x hash function
a loop wich runs ~1-2 times
an if (do we actually have long pipelines that suffer from wrong branch prediction?)
When we add more functions here like mapping sprn's and whatever else comes in mind later this might get even more complex, while this statistics should be the low overhead stats.
The thing I want to point out is that the tracing with the relay channel also only have (speaking of the reduced version discussed in the other thread with only 3 integers) 3 integers to move into a buffer and some overhead to select the right buffer. The userspace application reads 4096b => 341 records per scheduling of the reading app and we might even increase that size. I really like the "full trace" feeling of the relay based patch, so I don't like to increase the functionality&overhead of this patch which was intended to be low-overhead.

As stated above I agree with you about lines of code and error proneness of that patch. What about that:
- for integration into our upstream code only the relay based approach is take into account, giving a full trace while it has some overhead
- we keep the slight overhead, but error prone kvm_stat based patch in our private queues and only use it for our own measurements until we either need or change it once we have more experience with the relay based tracing (e.g. know the perf. overhead of that better)
- That way we would have two very similar solutions that share code and are configurable so developers can use them as they want
- additionally I think again about Arnd's comments and if we might need/want a runtime switch e.g. proc/sys interface to enable/disable that tracing functions on the fly

Comments welcome

Quoted 7 lines

It looks like we could build a hash table pretty easily with hash_long() and list_entry stuff (see fs/mbcache.c for example). So far you've found 17 different instructions types emulated in the "boot" workload, so 32 entries would probably be a reasonable hash size. The same approach could be used for SPR accesses, where you've hit 31 different registers. Really I think those numbers won't vary much by workload, but rather by guest kernel...

Signature · 13 lines
Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel
#3 lore raw
In reply to [email protected]

Could you please use a blank line when you intend to start a new paragraph in your email? The lack of whitespace here hurts readability.

Quoted 52 lines · On Tuesday 15 April 2008 02:34:47 Christian Ehrhardt wrote:
Quoted 16 lines · Hollis Blanchard
Quoted 9 lines · On Monday 14 April 2008 07:23:56 [email protected] wrote:

From: Christian Ehrhardt <[email protected]>

This extends the kvm_stat reports for kvm on embedded powerpc. Since kvmppc is using emulation (no hardware support yet) this gives people interested in performance a detailed split of the emul_instruction counter already available. This statistic does not cover e.g. operants of the commands, but that way it should have only a small perf impact (never break what you want to measure). This feature is configurable in .config under the kvmppc virtualization itself.

This array-based approach seems to add a lot of lines of code, and it's also copy/paste stuff that is just begging for a typo (e.g. miscounting STHX in the STHUX bucket) or being forgotten entirely when adding new emulation code.

Quoted 4 lines

A more general approach would be to record just the opcode/extended opcode in a variable-sized structure, allocating a new bucket as we encounter a previously unseen instruction. I'm thinking of something like this:

I already thought about some bucket like counting, but I focused small runtime overhead above everything else here by hijacking the already existent switch/case and avoiding any new if's & function calls. I agree that the array like solution is messy in the aspect of lines and is more error prone by forgetting something.

Quoted 11 lines
log_instruction(inst) {
	bucket = hash_u32(inst, 5);
	list_for_each(l, vcpu->instlog[bucket]) {
		struct instlog = list_entry(l);
		if (instlog->inst = inst) {
			instlog->count++;
			break;
		}
	}
}
Quoted 12 lines
emulate(inst) {
	log = get_op(inst);
	switch (get_op(inst)) {
		...
	case 31:
		log |= get_xop(inst);
		switch (inst) {
			...
		}
	}
	log_instruction(log);
}
Quoted 2 lines

I agree that this looks better, but is has per instruction:
1x hash function

This is a shift.

Quoted 1 line

a loop wich runs ~1-2 times

A couple additions and memory accesses.

Quoted 2 lines

an if (do we actually have long pipelines that suffer from wrong branch prediction?)

I believe we have many "if" statements. :)

Quoted 3 lines

When we add more functions here like mapping sprn's and whatever else comes in mind later this might get even more complex, while this statistics should be the low overhead stats.

I don't know what "mapping SPRNs" means.

In general though, I'm not so worried about adding some memory references or branches in this path. First, there are much bigger problems to worry about (need I remind you about our TLB faults?). Second, it's the trace path: if we care that much, we would just disable tracing. I think the benefits of the more general approach are well worth it.

Signature · 12 lines
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-ppc-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel