Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children
On Fri, Dec 16, 2011 at 06:13:42PM +0800, Xiao Guangrong wrote:
unsync is only used for the page of level == 1 and unsync_children is only
used in the upper page, so combine them to reduce the size of shadow page
structure
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
Documentation/virtual/kvm/mmu.txt |5 ++-
arch/x86/include/asm/kvm_host.h | 14 +++-
arch/x86/kvm/mmu.c| 39 +---
arch/x86/kvm/mmu_audit.c |6 ++--
arch/x86/kvm/mmutrace.h |3 +-
arch/x86/kvm/paging_tmpl.h|2 +-
6 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/Documentation/virtual/kvm/mmu.txt
b/Documentation/virtual/kvm/mmu.txt
index 5dc972c..4a5fedd 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
this page's spt. Otherwise, parent_ptes points at a data structure
with a list of parent_ptes.
unsync:
+It is used when role.level == 1, only level 1 shadow pages can be unsync.
If true, then the translations in this page may not match the guest's
translation. This is equivalent to the state of the tlb when a pte is
changed but before the tlb entry is flushed. Accordingly, unsync ptes
are synchronized when the guest executes invlpg or flushes its tlb by
other means. Valid for leaf pages.
unsync_children:
-How many sptes in the page point at pages that are unsync (or have
-unsynchronized children).
+It is used when role.level 1 and indicates how many sptes in the page
+point at unsync pages or unsynchronized children.
unsync_child_bitmap:
A bitmap indicating which sptes in spt point (directly or indirectly) at
pages that may be unsynchronized. Used to quickly locate all
unsychronized
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..c0a89cd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
* in this shadow page.
*/
DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
- bool unsync;
int root_count; /* Currently serving as active root */
- unsigned int unsync_children;
+
+ /*
+ * If role.level == 1, unsync indicates whether the sp is
+ * unsync, otherwise unsync_children indicates the number
+ * of sptes which point at unsync sp or unsychronized children.
+ * See sp_is_unsync() / sp_unsync_children_num.
+ */
+ union {
+ bool unsync;
+ unsigned int unsync_children;
+ };
+
unsigned long parent_ptes; /* Reverse mapping for parent_pte */
DECLARE_BITMAP(unsync_child_bitmap, 512);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 2a2a9b4..6913a16 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
#define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+ return sp-role.level == PT_PAGE_TABLE_LEVEL sp-unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+ unsigned int num = 0;
+
+ if (sp-role.level != PT_PAGE_TABLE_LEVEL)
+ num = sp-unsync_children;
+
+ return num;
+}
This adds significant complexity/trickery to the code for little gain.
For example, in kvm_mmu_page_get, you use sp_is_unsync() which depends
on sp-role.level, but that is not set yet.
In general, given the number of optimizations made to the shadow mmu
code, and the fact that over time NPT will dominate, i'd suggest time
should be spent improving other areas.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children
On 01/09/2012 07:16 PM, Marcelo Tosatti wrote: In general, given the number of optimizations made to the shadow mmu code, and the fact that over time NPT will dominate, i'd suggest time should be spent improving other areas. I totally agree with you, thanks Marcelo! -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children
On Fri, Dec 16, 2011 at 06:13:42PM +0800, Xiao Guangrong wrote:
unsync is only used for the page of level == 1 and unsync_children is only
used in the upper page, so combine them to reduce the size of shadow page
structure
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
Documentation/virtual/kvm/mmu.txt |5 ++-
arch/x86/include/asm/kvm_host.h | 14 +++-
arch/x86/kvm/mmu.c| 39 +---
arch/x86/kvm/mmu_audit.c |6 ++--
arch/x86/kvm/mmutrace.h |3 +-
arch/x86/kvm/paging_tmpl.h|2 +-
6 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/Documentation/virtual/kvm/mmu.txt
b/Documentation/virtual/kvm/mmu.txt
index 5dc972c..4a5fedd 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
this page's spt. Otherwise, parent_ptes points at a data structure
with a list of parent_ptes.
unsync:
+It is used when role.level == 1, only level 1 shadow pages can be unsync.
If true, then the translations in this page may not match the guest's
translation. This is equivalent to the state of the tlb when a pte is
changed but before the tlb entry is flushed. Accordingly, unsync ptes
are synchronized when the guest executes invlpg or flushes its tlb by
other means. Valid for leaf pages.
unsync_children:
-How many sptes in the page point at pages that are unsync (or have
-unsynchronized children).
+It is used when role.level 1 and indicates how many sptes in the page
+point at unsync pages or unsynchronized children.
unsync_child_bitmap:
A bitmap indicating which sptes in spt point (directly or indirectly) at
pages that may be unsynchronized. Used to quickly locate all
unsychronized
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..c0a89cd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
* in this shadow page.
*/
DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
- bool unsync;
int root_count; /* Currently serving as active root */
- unsigned int unsync_children;
+
+ /*
+ * If role.level == 1, unsync indicates whether the sp is
+ * unsync, otherwise unsync_children indicates the number
+ * of sptes which point at unsync sp or unsychronized children.
+ * See sp_is_unsync() / sp_unsync_children_num.
+ */
+ union {
+ bool unsync;
+ unsigned int unsync_children;
+ };
+
unsigned long parent_ptes; /* Reverse mapping for parent_pte */
DECLARE_BITMAP(unsync_child_bitmap, 512);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 2a2a9b4..6913a16 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
#define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+ return sp-role.level == PT_PAGE_TABLE_LEVEL sp-unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+ unsigned int num = 0;
+
+ if (sp-role.level != PT_PAGE_TABLE_LEVEL)
+ num = sp-unsync_children;
+
+ return num;
+}
IIRC there are windows where unsync_children is not accurate. Have you
verified this function is not called inside one of those windows?
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children
On 12/23/2011 12:06 AM, Marcelo Tosatti wrote:
On Fri, Dec 16, 2011 at 06:13:42PM +0800, Xiao Guangrong wrote:
unsync is only used for the page of level == 1 and unsync_children is only
used in the upper page, so combine them to reduce the size of shadow page
structure
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
Documentation/virtual/kvm/mmu.txt |5 ++-
arch/x86/include/asm/kvm_host.h | 14 +++-
arch/x86/kvm/mmu.c| 39
+---
arch/x86/kvm/mmu_audit.c |6 ++--
arch/x86/kvm/mmutrace.h |3 +-
arch/x86/kvm/paging_tmpl.h|2 +-
6 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/Documentation/virtual/kvm/mmu.txt
b/Documentation/virtual/kvm/mmu.txt
index 5dc972c..4a5fedd 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
this page's spt. Otherwise, parent_ptes points at a data structure
with a list of parent_ptes.
unsync:
+It is used when role.level == 1, only level 1 shadow pages can be
unsync.
If true, then the translations in this page may not match the guest's
translation. This is equivalent to the state of the tlb when a pte is
changed but before the tlb entry is flushed. Accordingly, unsync ptes
are synchronized when the guest executes invlpg or flushes its tlb by
other means. Valid for leaf pages.
unsync_children:
-How many sptes in the page point at pages that are unsync (or have
-unsynchronized children).
+It is used when role.level 1 and indicates how many sptes in the page
+point at unsync pages or unsynchronized children.
unsync_child_bitmap:
A bitmap indicating which sptes in spt point (directly or indirectly) at
pages that may be unsynchronized. Used to quickly locate all
unsychronized
diff --git a/arch/x86/include/asm/kvm_host.h
b/arch/x86/include/asm/kvm_host.h
index 52d6640..c0a89cd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
* in this shadow page.
*/
DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
-bool unsync;
int root_count; /* Currently serving as active root */
-unsigned int unsync_children;
+
+/*
+ * If role.level == 1, unsync indicates whether the sp is
+ * unsync, otherwise unsync_children indicates the number
+ * of sptes which point at unsync sp or unsychronized children.
+ * See sp_is_unsync() / sp_unsync_children_num.
+ */
+union {
+bool unsync;
+unsigned int unsync_children;
+};
+
unsigned long parent_ptes; /* Reverse mapping for parent_pte */
DECLARE_BITMAP(unsync_child_bitmap, 512);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 2a2a9b4..6913a16 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
#define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+return sp-role.level == PT_PAGE_TABLE_LEVEL sp-unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+unsigned int num = 0;
+
+if (sp-role.level != PT_PAGE_TABLE_LEVEL)
+num = sp-unsync_children;
+
+return num;
+}
IIRC there are windows where unsync_children is not accurate. Have you
verified this function is not called inside one of those windows?
Actually, this function is only used to see whether the sp has unsync children
in current code.
And this patch did not change the logic, it just use sp_unsync_children_num
instead of using sp-unsync_children directly.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children
About naming issues in the kvm mmu code.
Not restricted to your patch series, so please take as a suggestion
for the future.
(2011/12/16 19:13), Xiao Guangrong wrote:
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+ return sp-role.level == PT_PAGE_TABLE_LEVEL sp-unsync;
+}
is_unsync_sp() is more consistent with others?
e.g. is_large_pte(), is_writable_pte(), is_last_spte()
Takuya
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+ unsigned int num = 0;
+
+ if (sp-role.level != PT_PAGE_TABLE_LEVEL)
+ num = sp-unsync_children;
+
+ return num;
+}
+
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] KVM: MMU: combine unsync and unsync_children
unsync is only used for the page of level == 1 and unsync_children is only
used in the upper page, so combine them to reduce the size of shadow page
structure
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
Documentation/virtual/kvm/mmu.txt |5 ++-
arch/x86/include/asm/kvm_host.h | 14 +++-
arch/x86/kvm/mmu.c| 39 +---
arch/x86/kvm/mmu_audit.c |6 ++--
arch/x86/kvm/mmutrace.h |3 +-
arch/x86/kvm/paging_tmpl.h|2 +-
6 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/Documentation/virtual/kvm/mmu.txt
b/Documentation/virtual/kvm/mmu.txt
index 5dc972c..4a5fedd 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
this page's spt. Otherwise, parent_ptes points at a data structure
with a list of parent_ptes.
unsync:
+It is used when role.level == 1, only level 1 shadow pages can be unsync.
If true, then the translations in this page may not match the guest's
translation. This is equivalent to the state of the tlb when a pte is
changed but before the tlb entry is flushed. Accordingly, unsync ptes
are synchronized when the guest executes invlpg or flushes its tlb by
other means. Valid for leaf pages.
unsync_children:
-How many sptes in the page point at pages that are unsync (or have
-unsynchronized children).
+It is used when role.level 1 and indicates how many sptes in the page
+point at unsync pages or unsynchronized children.
unsync_child_bitmap:
A bitmap indicating which sptes in spt point (directly or indirectly) at
pages that may be unsynchronized. Used to quickly locate all unsychronized
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..c0a89cd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
* in this shadow page.
*/
DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
- bool unsync;
int root_count; /* Currently serving as active root */
- unsigned int unsync_children;
+
+ /*
+* If role.level == 1, unsync indicates whether the sp is
+* unsync, otherwise unsync_children indicates the number
+* of sptes which point at unsync sp or unsychronized children.
+* See sp_is_unsync() / sp_unsync_children_num.
+*/
+ union {
+ bool unsync;
+ unsigned int unsync_children;
+ };
+
unsigned long parent_ptes; /* Reverse mapping for parent_pte */
DECLARE_BITMAP(unsync_child_bitmap, 512);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 2a2a9b4..6913a16 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
#define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+ return sp-role.level == PT_PAGE_TABLE_LEVEL sp-unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+ unsigned int num = 0;
+
+ if (sp-role.level != PT_PAGE_TABLE_LEVEL)
+ num = sp-unsync_children;
+
+ return num;
+}
+
struct pte_list_desc {
u64 *sptes[PTE_LIST_EXT];
struct pte_list_desc *more;
@@ -1401,7 +1416,7 @@ static int mmu_pages_add(struct kvm_mmu_pages *pvec,
struct kvm_mmu_page *sp,
{
int i;
- if (sp-unsync)
+ if (sp_is_unsync(sp))
for (i=0; i pvec-nr; i++)
if (pvec-page[i].sp == sp)
return 0;
@@ -1426,7 +1441,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
child = page_header(ent PT64_BASE_ADDR_MASK);
- if (child-unsync_children) {
+ if (sp_unsync_children_num(child)) {
if (mmu_pages_add(pvec, child, i))
return -ENOSPC;
@@ -1437,7 +1452,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
nr_unsync_leaf += ret;
else
return ret;
- } else if (child-unsync) {
+ } else if (sp_is_unsync(child)) {
nr_unsync_leaf++;
if (mmu_pages_add(pvec, child, i))
return -ENOSPC;
@@ -1468,7 +1483,7 @@ static int mmu_unsync_walk(struct kvm_mmu_page *sp,
static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
{
- WARN_ON(!sp-unsync);
+ WARN_ON(!sp_is_unsync(sp));
trace_kvm_mmu_sync_page(sp);
sp-unsync = 0;
--kvm-stat.mmu_unsync;
@@ -1546,7 +1561,7 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t
