mbox series

[v4,0/2] selftests: KVM: Add a test for eager page splitting

Message ID 20230125182311.2022303-1-bgardon@google.com
Headers
Series selftests: KVM: Add a test for eager page splitting |

Message

Ben Gardon Jan. 25, 2023, 6:23 p.m. UTC 
  David Matlack recently added a feature known as eager page splitting
to x86 KVM. This feature improves vCPU performance during dirty
logging because the splitting operation is moved out of the page
fault path, avoiding EPT/NPT violations or allowing the vCPU threads
to resolve the violation in the fast path.

While this feature is a great performance improvement, it does not
have adequate testing in KVM selftests. Add a test to provide coverage
of eager page splitting.

Patch 1 is a quick refactor to be able to re-use some code from
dirty_log_perf_test.
Patch 2 adds the actual test.

V1->V2:
	Run test in multiple modes, as suggested by David and Ricardo
	Cleanups from shameful copy-pasta, as suggested by David
V2->V3:
	Removed copyright notice from the top of
	dirty_log_page_splitting.c
	Adopted ASSERT_EQ for test assertions
	Now skipping testing with MANUAL_PROTECT if unsupported
V3->V4:
	Added the copyright notices back. Thanks Vipin for the right
	thing to do there.

Ben Gardon (2):
  selftests: KVM: Move dirty logging functions to memstress.(c|h)
  selftests: KVM: Add dirty logging page splitting test

 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/dirty_log_perf_test.c       |  84 +-----
 .../selftests/kvm/include/kvm_util_base.h     |   1 +
 .../testing/selftests/kvm/include/memstress.h |   8 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |   5 +
 tools/testing/selftests/kvm/lib/memstress.c   |  72 +++++
 .../x86_64/dirty_log_page_splitting_test.c    | 257 ++++++++++++++++++
 7 files changed, 351 insertions(+), 77 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c

Comments

Vipin Sharma Jan. 26, 2023, 8:09 p.m. UTC | #1 
On Wed, Jan 25, 2023 at 10:23 AM Ben Gardon <bgardon@google.com> wrote:
>
> David Matlack recently added a feature known as eager page splitting
> to x86 KVM. This feature improves vCPU performance during dirty
> logging because the splitting operation is moved out of the page
> fault path, avoiding EPT/NPT violations or allowing the vCPU threads
> to resolve the violation in the fast path.
>
> While this feature is a great performance improvement, it does not
> have adequate testing in KVM selftests. Add a test to provide coverage
> of eager page splitting.
>
> Patch 1 is a quick refactor to be able to re-use some code from
> dirty_log_perf_test.
> Patch 2 adds the actual test.
>
> V1->V2:
Links of previous versions of patches are helpful and avoid searching
if one wants to read previous discussions.

>         Run test in multiple modes, as suggested by David and Ricardo
>         Cleanups from shameful copy-pasta, as suggested by David
> V2->V3:
>         Removed copyright notice from the top of
>         dirty_log_page_splitting.c
>         Adopted ASSERT_EQ for test assertions
>         Now skipping testing with MANUAL_PROTECT if unsupported
> V3->V4:
>         Added the copyright notices back. Thanks Vipin for the right
>         thing to do there.
>
> Ben Gardon (2):
>   selftests: KVM: Move dirty logging functions to memstress.(c|h)
>   selftests: KVM: Add dirty logging page splitting test
>
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/dirty_log_perf_test.c       |  84 +-----
>  .../selftests/kvm/include/kvm_util_base.h     |   1 +
>  .../testing/selftests/kvm/include/memstress.h |   8 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    |   5 +
>  tools/testing/selftests/kvm/lib/memstress.c   |  72 +++++
>  .../x86_64/dirty_log_page_splitting_test.c    | 257 ++++++++++++++++++
>  7 files changed, 351 insertions(+), 77 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
>
> --
> 2.39.1.456.gfc5497dd1b-goog
>
Sean Christopherson Jan. 31, 2023, 4:57 p.m. UTC | #2 
On Thu, Jan 26, 2023, Vipin Sharma wrote:
> On Wed, Jan 25, 2023 at 10:23 AM Ben Gardon <bgardon@google.com> wrote:
> >
> > David Matlack recently added a feature known as eager page splitting
> > to x86 KVM. This feature improves vCPU performance during dirty
> > logging because the splitting operation is moved out of the page
> > fault path, avoiding EPT/NPT violations or allowing the vCPU threads
> > to resolve the violation in the fast path.
> >
> > While this feature is a great performance improvement, it does not
> > have adequate testing in KVM selftests. Add a test to provide coverage
> > of eager page splitting.
> >
> > Patch 1 is a quick refactor to be able to re-use some code from
> > dirty_log_perf_test.
> > Patch 2 adds the actual test.
> >
> > V1->V2:
> Links of previous versions of patches are helpful and avoid searching
> if one wants to read previous discussions.

+1

I also strongly prefer versioning to go from newest=>oldest instead of oldest=>newest,
that way reviewers can quickly see the delta for _this_ version.  It doesn't matter
too much when the delta is small, but oldest=>newest gets _really_ annoying the
longer the list gets.

> >         Run test in multiple modes, as suggested by David and Ricardo
> >         Cleanups from shameful copy-pasta, as suggested by David
> > V2->V3:
> >         Removed copyright notice from the top of
> >         dirty_log_page_splitting.c
> >         Adopted ASSERT_EQ for test assertions
> >         Now skipping testing with MANUAL_PROTECT if unsupported
> > V3->V4:
> >         Added the copyright notices back. Thanks Vipin for the right
> >         thing to do there.

Please do _something_ to differentiate line items.  I would also drop the tabs in
favor of a single space or two so that you don't end up wrapping so much.  E.g.
(choose your preferred flavor of list item identifiers)

V3->V4:
  - Added the copyright notices back. Thanks Vipin for the right thing to do
    there

V2->V3:
  - Removed copyright notice from the top of dirty_log_page_splitting.c
  - Adopted ASSERT_EQ for test assertions
  - Now skipping testing with MANUAL_PROTECT if unsupported