linux-kernel - Re: [PATCH v4 0/2] selftests: KVM: Add a test for eager page splitting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y9lIfGtMEEsaw6cd@google.com>
Date:   Tue, 31 Jan 2023 16:57:32 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vipin Sharma <vipinsh@...gle.com>
Cc:     Ben Gardon <bgardon@...gle.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        David Matlack <dmatlack@...gle.com>,
        Ricardo Koller <ricarkol@...gle.com>
Subject: Re: [PATCH v4 0/2] selftests: KVM: Add a test for eager page
 splitting

On Thu, Jan 26, 2023, Vipin Sharma wrote:
> On Wed, Jan 25, 2023 at 10:23 AM Ben Gardon <bgardon@...gle.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