diff mbox series

[07/12] KVM: X86/MMU: Remove the useless struct mmu_page_path

Message ID 20220605064342.309219-8-jiangshanlai@gmail.com (mailing list archive)
State New, archived
Headers show
Series KVM: X86/MMU: Simpliy mmu_unsync_walk() | expand

Commit Message

Lai Jiangshan June 5, 2022, 6:43 a.m. UTC 
From: Lai Jiangshan <jiangshan.ljs@antgroup.com>

struct mmu_page_path is set and updated but never used since
mmu_pages_clear_parents() is removed.

Remove it.

Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
---
 arch/x86/kvm/mmu/mmu.c | 37 +++++++++----------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

Comments

Sean Christopherson July 19, 2022, 8:15 p.m. UTC | #1 
Nit, s/useless/now-unused, or "no longer used".  I associate "useless" in shortlogs
as "this <xyz> is pointless and always has been pointless", whereas "now-unused"
is likely to be interpreted as "remove <xyz> as it's no longer used after recent
changes".

Alternatively, can this patch be squashed with the patch that removes
mmu_pages_clear_parents()?  Yeah, it'll be a (much?) larger patch, but leaving
dead code behind is arguably worse.

On Sun, Jun 05, 2022, Lai Jiangshan wrote:
> From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> 
> struct mmu_page_path is set and updated but never used since
> mmu_pages_clear_parents() is removed.
> 
> Remove it.
> 
> Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> ---
Lai Jiangshan July 21, 2022, 9:43 a.m. UTC | #2 
On Wed, Jul 20, 2022 at 4:15 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Nit, s/useless/now-unused, or "no longer used".  I associate "useless" in shortlogs
> as "this <xyz> is pointless and always has been pointless", whereas "now-unused"
> is likely to be interpreted as "remove <xyz> as it's no longer used after recent
> changes".
>
> Alternatively, can this patch be squashed with the patch that removes
> mmu_pages_clear_parents()?  Yeah, it'll be a (much?) larger patch, but leaving
> dead code behind is arguably worse.

Defined by the C-language and the machine, struct mmu_page_path is used
in for_each_sp() and the data is set and updated every iteration.

It is not really dead code.

Defined by the sematic that we want, gathering unsync pages,
we don't need struct mmu_page_path, since the struct is not used
by "someone reading it".

>
> On Sun, Jun 05, 2022, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> >
> > struct mmu_page_path is set and updated but never used since
> > mmu_pages_clear_parents() is removed.
> >
> > Remove it.
> >
> > Signed-off-by: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> > ---
Sean Christopherson July 21, 2022, 3:25 p.m. UTC | #3 
On Thu, Jul 21, 2022, Lai Jiangshan wrote:
> On Wed, Jul 20, 2022 at 4:15 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Nit, s/useless/now-unused, or "no longer used".  I associate "useless" in shortlogs
> > as "this <xyz> is pointless and always has been pointless", whereas "now-unused"
> > is likely to be interpreted as "remove <xyz> as it's no longer used after recent
> > changes".
> >
> > Alternatively, can this patch be squashed with the patch that removes
> > mmu_pages_clear_parents()?  Yeah, it'll be a (much?) larger patch, but leaving
> > dead code behind is arguably worse.
> 
> Defined by the C-language and the machine, struct mmu_page_path is used
> in for_each_sp() and the data is set and updated every iteration.
> 
> It is not really dead code.

I'm not talking about just "struct mmu_page_path", but also the pointless updates
in for_each_sp().  And I think even if we're being super pedantic, it _is_ dead
code because C99 allows the compiler to drop code that the compiler can prove has
no side effects.  I learned this the hard way by discovering that an asm() blob
with an output constraint will be elided if the output isn't consumed and the asm()
blob isn't tagged volatile.

  In the abstract machine, all expressions are evaluated as specified by the
  semantics. An actual implementation need not evaluate part of an expression if
  it can deduce that its value is not used and that no needed side effects are
  produced (including any caused by calling a function or accessing a volatile object)

I don't see any advantage to separating this from mmu_pages_clear_parents().  It
doesn't make the code any easier to review.  I'd argue it does the opposite because
it makes it harder to see that mmu_pages_clear_parents() was the only user, i.e.
squashing this would provide further justification for dropping mmu_pages_clear_parents().
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index a56d328365e4..65a2f4a2ce25 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1897,39 +1897,28 @@  static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
 	       unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
 }
 
-struct mmu_page_path {
-	struct kvm_mmu_page *parent[PT64_ROOT_MAX_LEVEL];
-	unsigned int idx[PT64_ROOT_MAX_LEVEL];
-};
-
-#define for_each_sp(pvec, sp, parents, i)			\
-		for (i = mmu_pages_first(&pvec, &parents);	\
+#define for_each_sp(pvec, sp, i)					\
+		for (i = mmu_pages_first(&pvec);			\
 			i < pvec.nr && ({ sp = pvec.page[i].sp; 1;});	\
-			i = mmu_pages_next(&pvec, &parents, i))
+			i = mmu_pages_next(&pvec, i))
 
-static int mmu_pages_next(struct kvm_mmu_pages *pvec,
-			  struct mmu_page_path *parents,
-			  int i)
+static int mmu_pages_next(struct kvm_mmu_pages *pvec, int i)
 {
 	int n;
 
 	for (n = i+1; n < pvec->nr; n++) {
 		struct kvm_mmu_page *sp = pvec->page[n].sp;
-		unsigned idx = pvec->page[n].idx;
 		int level = sp->role.level;
 
-		parents->idx[level-1] = idx;
 		if (level == PG_LEVEL_4K)
 			break;
 
-		parents->parent[level-2] = sp;
 	}
 
 	return n;
 }
 
-static int mmu_pages_first(struct kvm_mmu_pages *pvec,
-			   struct mmu_page_path *parents)
+static int mmu_pages_first(struct kvm_mmu_pages *pvec)
 {
 	struct kvm_mmu_page *sp;
 	int level;
@@ -1943,13 +1932,7 @@  static int mmu_pages_first(struct kvm_mmu_pages *pvec,
 	level = sp->role.level;
 	WARN_ON(level == PG_LEVEL_4K);
 
-	parents->parent[level-2] = sp;
-
-	/* Also set up a sentinel.  Further entries in pvec are all
-	 * children of sp, so this element is never overwritten.
-	 */
-	parents->parent[level-1] = NULL;
-	return mmu_pages_next(pvec, parents, 0);
+	return mmu_pages_next(pvec, 0);
 }
 
 static int mmu_sync_children(struct kvm_vcpu *vcpu,
@@ -1957,7 +1940,6 @@  static int mmu_sync_children(struct kvm_vcpu *vcpu,
 {
 	int i;
 	struct kvm_mmu_page *sp;
-	struct mmu_page_path parents;
 	struct kvm_mmu_pages pages;
 	LIST_HEAD(invalid_list);
 	bool flush = false;
@@ -1965,7 +1947,7 @@  static int mmu_sync_children(struct kvm_vcpu *vcpu,
 	while (mmu_unsync_walk_and_clear(parent, &pages)) {
 		bool protected = false;
 
-		for_each_sp(pages, sp, parents, i)
+		for_each_sp(pages, sp, i)
 			protected |= kvm_vcpu_write_protect_gfn(vcpu, sp->gfn);
 
 		if (protected) {
@@ -1973,7 +1955,7 @@  static int mmu_sync_children(struct kvm_vcpu *vcpu,
 			flush = false;
 		}
 
-		for_each_sp(pages, sp, parents, i) {
+		for_each_sp(pages, sp, i) {
 			kvm_mmu_page_clear_unsync(vcpu->kvm, sp);
 			flush |= kvm_sync_page(vcpu, sp, &invalid_list) > 0;
 		}
@@ -2273,7 +2255,6 @@  static int mmu_zap_unsync_children(struct kvm *kvm,
 				   struct list_head *invalid_list)
 {
 	int i, zapped = 0;
-	struct mmu_page_path parents;
 	struct kvm_mmu_pages pages;
 
 	if (parent->role.level == PG_LEVEL_4K)
@@ -2282,7 +2263,7 @@  static int mmu_zap_unsync_children(struct kvm *kvm,
 	while (mmu_unsync_walk_and_clear(parent, &pages)) {
 		struct kvm_mmu_page *sp;
 
-		for_each_sp(pages, sp, parents, i) {
+		for_each_sp(pages, sp, i) {
 			kvm_mmu_prepare_zap_page(kvm, sp, invalid_list);
 			zapped++;
 		}