diff mbox series

[v2,02/11] KVM: selftests: Remove deadcode

Message ID 20201111122636.73346-3-drjones@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: selftests: Cleanups, take 2 | expand

Commit Message

Andrew Jones Nov. 11, 2020, 12:26 p.m. UTC 
Nothing sets USE_CLEAR_DIRTY_LOG anymore, so anything it surrounds
is dead code.

Reviewed-by: Ben Gardon <bgardon@google.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../selftests/kvm/dirty_log_perf_test.c       | 44 -------------------
 1 file changed, 44 deletions(-)

Comments

Peter Xu Nov. 12, 2020, 6:19 p.m. UTC | #1 
On Wed, Nov 11, 2020 at 01:26:27PM +0100, Andrew Jones wrote:
> Nothing sets USE_CLEAR_DIRTY_LOG anymore, so anything it surrounds
> is dead code.
> 
> Reviewed-by: Ben Gardon <bgardon@google.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

It's kind of a pity that there seem to a few valid measurements for clear dirty
log from Ben. I'm just thinking whether clear dirty log should be even more
important since imho that should be the right way to use KVM_GET_DIRTY_LOG on a
kernel new enough, since it's a total win (not like dirty ring, which depends).

So far, the statement is definitely true above, since we can always work on
top.  So:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,
Ben Gardon Nov. 12, 2020, 6:34 p.m. UTC | #2 
On Thu, Nov 12, 2020 at 10:19 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Nov 11, 2020 at 01:26:27PM +0100, Andrew Jones wrote:
> > Nothing sets USE_CLEAR_DIRTY_LOG anymore, so anything it surrounds
> > is dead code.
> >
> > Reviewed-by: Ben Gardon <bgardon@google.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>
> It's kind of a pity that there seem to a few valid measurements for clear dirty
> log from Ben. I'm just thinking whether clear dirty log should be even more
> important since imho that should be the right way to use KVM_GET_DIRTY_LOG on a
> kernel new enough, since it's a total win (not like dirty ring, which depends).

I didn't review this patch closely enough, and assumed the clear dirty
log was still being done because of
afdb19600719 KVM: selftests: Use a single binary for dirty/clear log test

Looking back now, I see that that is not the case.

I'd like to retract my endorsement in that case. I'd prefer to leave
the dead code in and I'll send another series to actually use it once
this series is merged. I've already written the code to use it and
time the clearing, so it seems a pity to remove it now.

Alternatively I could just revert this commit in that future series,
though I suspect not removing the dead code would reduce the chances
of merge conflicts. Either way works.

I can extend the dirty log mode functions from dirty_log_test for
dirty_log_perf_test in that series too.


>
> So far, the statement is definitely true above, since we can always work on
> top.  So:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Thanks,
>
> --
> Peter Xu
>
Peter Xu Nov. 12, 2020, 6:50 p.m. UTC | #3 
On Thu, Nov 12, 2020 at 10:34:11AM -0800, Ben Gardon wrote:
> I didn't review this patch closely enough, and assumed the clear dirty
> log was still being done because of
> afdb19600719 KVM: selftests: Use a single binary for dirty/clear log test
> 
> Looking back now, I see that that is not the case.
> 
> I'd like to retract my endorsement in that case. I'd prefer to leave
> the dead code in and I'll send another series to actually use it once
> this series is merged. I've already written the code to use it and
> time the clearing, so it seems a pity to remove it now.
> 
> Alternatively I could just revert this commit in that future series,
> though I suspect not removing the dead code would reduce the chances
> of merge conflicts. Either way works.
> 
> I can extend the dirty log mode functions from dirty_log_test for
> dirty_log_perf_test in that series too.

Or... we can just remove all the "#ifdef" lines but assuming clear dirty log is
always there? :) Assuming that is still acceptable as long as the test is
matching latest kernel which definitely has the clear dirty log capability.

It's kind of weird to test get-dirty-log perf without clear dirty log, since
again if anyone really cares about the perf of that, then imho they should
first switch to a new kernel with clear dirty log, rather than measuring the
world without clear dirty log..
Andrew Jones Nov. 13, 2020, 7:57 a.m. UTC | #4 
On Thu, Nov 12, 2020 at 01:50:06PM -0500, Peter Xu wrote:
> On Thu, Nov 12, 2020 at 10:34:11AM -0800, Ben Gardon wrote:
> > I didn't review this patch closely enough, and assumed the clear dirty
> > log was still being done because of
> > afdb19600719 KVM: selftests: Use a single binary for dirty/clear log test
> > 
> > Looking back now, I see that that is not the case.
> > 
> > I'd like to retract my endorsement in that case. I'd prefer to leave
> > the dead code in and I'll send another series to actually use it once
> > this series is merged. I've already written the code to use it and
> > time the clearing, so it seems a pity to remove it now.
> > 
> > Alternatively I could just revert this commit in that future series,
> > though I suspect not removing the dead code would reduce the chances
> > of merge conflicts. Either way works.
> > 
> > I can extend the dirty log mode functions from dirty_log_test for
> > dirty_log_perf_test in that series too.
> 
> Or... we can just remove all the "#ifdef" lines but assuming clear dirty log is
> always there? :) Assuming that is still acceptable as long as the test is
> matching latest kernel which definitely has the clear dirty log capability.
> 
> It's kind of weird to test get-dirty-log perf without clear dirty log, since
> again if anyone really cares about the perf of that, then imho they should
> first switch to a new kernel with clear dirty log, rather than measuring the
> world without clear dirty log..
>

I have no opinion on this other than I'd prefer KVM selftests to not have
deadcode. If it makes more sense to fix the deadcode by bringing it back
to life than to drop the code, then please send patches to do that, at
which point I'd be happy to recommend dropping this patch.

Thanks,
drew
Paolo Bonzini Nov. 13, 2020, 4:36 p.m. UTC | #5 
On 12/11/20 19:34, Ben Gardon wrote:
> I didn't review this patch closely enough, and assumed the clear dirty
> log was still being done because of
> afdb19600719 KVM: selftests: Use a single binary for dirty/clear log test
> 
> Looking back now, I see that that is not the case.
> 
> I'd like to retract my endorsement in that case. I'd prefer to leave
> the dead code in and I'll send another series to actually use it once
> this series is merged. I've already written the code to use it and
> time the clearing, so it seems a pity to remove it now.
> 
> Alternatively I could just revert this commit in that future series,
> though I suspect not removing the dead code would reduce the chances
> of merge conflicts. Either way works.
> 
> I can extend the dirty log mode functions from dirty_log_test for
> dirty_log_perf_test in that series too.

For now I'll follow Peter's suggestion to always test manual clear:

-------------- 8< ------------
Subject: [PATCH] KVM: selftests: always use manual clear in 
dirty_log_perf_test

Nothing sets USE_CLEAR_DIRTY_LOG anymore, so anything it surrounds
is dead code.

However, it is the recommended way to use the dirty page bitmap
for new enough kernel, so use it whenever KVM has the
KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 capability.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c 
b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 85c9b8f73142..9c6a7be31e03 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -27,6 +27,7 @@
  #define TEST_HOST_LOOP_N		2UL

  /* Host variables */
+static u64 dirty_log_manual_caps;
  static bool host_quit;
  static uint64_t iteration;
  static uint64_t vcpu_last_completed_iteration[MAX_VCPUS];
@@ -88,10 +89,6 @@ static void *vcpu_worker(void *data)
  	return NULL;
  }

-#ifdef USE_CLEAR_DIRTY_LOG
-static u64 dirty_log_manual_caps;
-#endif
-
  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
  		     uint64_t phys_offset, int wr_fract)
  {
@@ -106,10 +103,8 @@ static void run_test(enum vm_guest_mode mode, 
unsigned long iterations,
  	struct timespec get_dirty_log_total = (struct timespec){0};
  	struct timespec vcpu_dirty_total = (struct timespec){0};
  	struct timespec avg;
-#ifdef USE_CLEAR_DIRTY_LOG
  	struct kvm_enable_cap cap = {};
  	struct timespec clear_dirty_log_total = (struct timespec){0};
-#endif

  	vm = create_vm(mode, nr_vcpus, guest_percpu_mem_size);

@@ -120,11 +115,11 @@ static void run_test(enum vm_guest_mode mode, 
unsigned long iterations,
  	host_num_pages = vm_num_host_pages(mode, guest_num_pages);
  	bmap = bitmap_alloc(host_num_pages);

-#ifdef USE_CLEAR_DIRTY_LOG
-	cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
-	cap.args[0] = dirty_log_manual_caps;
-	vm_enable_cap(vm, &cap);
-#endif
+	if (dirty_log_manual_caps) {
+		cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
+		cap.args[0] = dirty_log_manual_caps;
+		vm_enable_cap(vm, &cap);
+	}

  	vcpu_threads = malloc(nr_vcpus * sizeof(*vcpu_threads));
  	TEST_ASSERT(vcpu_threads, "Memory allocation failed");
@@ -190,17 +185,17 @@ static void run_test(enum vm_guest_mode mode, 
unsigned long iterations,
  		pr_info("Iteration %lu get dirty log time: %ld.%.9lds\n",
  			iteration, ts_diff.tv_sec, ts_diff.tv_nsec);

-#ifdef USE_CLEAR_DIRTY_LOG
-		clock_gettime(CLOCK_MONOTONIC, &start);
-		kvm_vm_clear_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap, 0,
-				       host_num_pages);
+		if (dirty_log_manual_caps) {
+			clock_gettime(CLOCK_MONOTONIC, &start);
+			kvm_vm_clear_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap, 0,
+					       host_num_pages);

-		ts_diff = timespec_diff_now(start);
-		clear_dirty_log_total = timespec_add(clear_dirty_log_total,
-						     ts_diff);
-		pr_info("Iteration %lu clear dirty log time: %ld.%.9lds\n",
-			iteration, ts_diff.tv_sec, ts_diff.tv_nsec);
-#endif
+			ts_diff = timespec_diff_now(start);
+			clear_dirty_log_total = timespec_add(clear_dirty_log_total,
+							     ts_diff);
+			pr_info("Iteration %lu clear dirty log time: %ld.%.9lds\n",
+				iteration, ts_diff.tv_sec, ts_diff.tv_nsec);
+		}
  	}

  	/* Tell the vcpu thread to quit */
@@ -220,12 +215,12 @@ static void run_test(enum vm_guest_mode mode, 
unsigned long iterations,
  		iterations, get_dirty_log_total.tv_sec,
  		get_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec);

-#ifdef USE_CLEAR_DIRTY_LOG
-	avg = timespec_div(clear_dirty_log_total, iterations);
-	pr_info("Clear dirty log over %lu iterations took %ld.%.9lds. (Avg 
%ld.%.9lds/iteration)\n",
-		iterations, clear_dirty_log_total.tv_sec,
-		clear_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec);
-#endif
+	if (dirty_log_manual_caps) {
+		avg = timespec_div(clear_dirty_log_total, iterations);
+		pr_info("Clear dirty log over %lu iterations took %ld.%.9lds. (Avg 
%ld.%.9lds/iteration)\n",
+			iterations, clear_dirty_log_total.tv_sec,
+			clear_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec);
+	}

  	free(bmap);
  	free(vcpu_threads);
@@ -284,16 +279,10 @@ int main(int argc, char *argv[])
  	int opt, i;
  	int wr_fract = 1;

-#ifdef USE_CLEAR_DIRTY_LOG
  	dirty_log_manual_caps =
  		kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
-	if (!dirty_log_manual_caps) {
-		print_skip("KVM_CLEAR_DIRTY_LOG not available");
-		exit(KSFT_SKIP);
-	}
  	dirty_log_manual_caps &= (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE |
  				  KVM_DIRTY_LOG_INITIALLY_SET);
-#endif

  #ifdef __x86_64__
  	guest_mode_init(VM_MODE_PXXV48_4K, true, true);
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 85c9b8f73142..b9115e8ef0ed 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -88,10 +88,6 @@  static void *vcpu_worker(void *data)
 	return NULL;
 }
 
-#ifdef USE_CLEAR_DIRTY_LOG
-static u64 dirty_log_manual_caps;
-#endif
-
 static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 		     uint64_t phys_offset, int wr_fract)
 {
@@ -106,10 +102,6 @@  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	struct timespec get_dirty_log_total = (struct timespec){0};
 	struct timespec vcpu_dirty_total = (struct timespec){0};
 	struct timespec avg;
-#ifdef USE_CLEAR_DIRTY_LOG
-	struct kvm_enable_cap cap = {};
-	struct timespec clear_dirty_log_total = (struct timespec){0};
-#endif
 
 	vm = create_vm(mode, nr_vcpus, guest_percpu_mem_size);
 
@@ -120,12 +112,6 @@  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	host_num_pages = vm_num_host_pages(mode, guest_num_pages);
 	bmap = bitmap_alloc(host_num_pages);
 
-#ifdef USE_CLEAR_DIRTY_LOG
-	cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
-	cap.args[0] = dirty_log_manual_caps;
-	vm_enable_cap(vm, &cap);
-#endif
-
 	vcpu_threads = malloc(nr_vcpus * sizeof(*vcpu_threads));
 	TEST_ASSERT(vcpu_threads, "Memory allocation failed");
 
@@ -189,18 +175,6 @@  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 						   ts_diff);
 		pr_info("Iteration %lu get dirty log time: %ld.%.9lds\n",
 			iteration, ts_diff.tv_sec, ts_diff.tv_nsec);
-
-#ifdef USE_CLEAR_DIRTY_LOG
-		clock_gettime(CLOCK_MONOTONIC, &start);
-		kvm_vm_clear_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap, 0,
-				       host_num_pages);
-
-		ts_diff = timespec_diff_now(start);
-		clear_dirty_log_total = timespec_add(clear_dirty_log_total,
-						     ts_diff);
-		pr_info("Iteration %lu clear dirty log time: %ld.%.9lds\n",
-			iteration, ts_diff.tv_sec, ts_diff.tv_nsec);
-#endif
 	}
 
 	/* Tell the vcpu thread to quit */
@@ -220,13 +194,6 @@  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 		iterations, get_dirty_log_total.tv_sec,
 		get_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec);
 
-#ifdef USE_CLEAR_DIRTY_LOG
-	avg = timespec_div(clear_dirty_log_total, iterations);
-	pr_info("Clear dirty log over %lu iterations took %ld.%.9lds. (Avg %ld.%.9lds/iteration)\n",
-		iterations, clear_dirty_log_total.tv_sec,
-		clear_dirty_log_total.tv_nsec, avg.tv_sec, avg.tv_nsec);
-#endif
-
 	free(bmap);
 	free(vcpu_threads);
 	ucall_uninit(vm);
@@ -284,17 +251,6 @@  int main(int argc, char *argv[])
 	int opt, i;
 	int wr_fract = 1;
 
-#ifdef USE_CLEAR_DIRTY_LOG
-	dirty_log_manual_caps =
-		kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
-	if (!dirty_log_manual_caps) {
-		print_skip("KVM_CLEAR_DIRTY_LOG not available");
-		exit(KSFT_SKIP);
-	}
-	dirty_log_manual_caps &= (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE |
-				  KVM_DIRTY_LOG_INITIALLY_SET);
-#endif
-
 #ifdef __x86_64__
 	guest_mode_init(VM_MODE_PXXV48_4K, true, true);
 #endif