Re: [PATCH 5/5] KVM: selftests: Introduce the dirty log perf test

[Peter Xu <peterx@redhat.com>]

On Tue, Nov 03, 2020 at 02:17:53PM -0800, Ben Gardon wrote:
> On Mon, Nov 2, 2020 at 5:12 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Mon, Nov 02, 2020 at 03:56:05PM -0800, Ben Gardon wrote:
> > > On Mon, Nov 2, 2020 at 2:21 PM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Tue, Oct 27, 2020 at 04:37:33PM -0700, Ben Gardon wrote:
> > > > > The dirty log perf test will time verious dirty logging operations
> > > > > (enabling dirty logging, dirtying memory, getting the dirty log,
> > > > > clearing the dirty log, and disabling dirty logging) in order to
> > > > > quantify dirty logging performance. This test can be used to inform
> > > > > future performance improvements to KVM's dirty logging infrastructure.
> > > >
> > > > One thing to mention is that there're a few patches in the kvm dirty ring
> > > > series that reworked the dirty log test quite a bit (to add similar test for
> > > > dirty ring).  For example:
> > > >
> > > >   https://lore.kernel.org/kvm/20201023183358.50607-11-peterx@redhat.com/
> > > >
> > > > Just a FYI if we're going to use separate test programs.  Merging this tests
> > > > should benefit in many ways, of course (e.g., dirty ring may directly runnable
> > > > with the perf tests too; so we can manually enable this "perf mode" as a new
> > > > parameter in dirty_log_test, if possible?), however I don't know how hard -
> > > > maybe there's some good reason to keep them separate...
> > >
> > > Absolutely, we definitely need a performance test for both modes. I'll
> > > take a look at the patch you linked and see what it would take to
> > > support dirty ring in this test.
> >
> > That would be highly appreciated.
> >
> > > Do you think that should be done in this series, or would it make
> > > sense to add as a follow up?
> >
> > To me I slightly lean toward working upon those patches, since we should
> > potentially share quite some code there (e.g., the clear dirty log cleanup
> > seems necessary, or not easy to add the dirty ring tests anyway).  But current
> > one is still ok to me at least as initial version - we should always be more
> > tolerant for test cases, aren't we? :)
> >
> > So maybe we can wait for a 3rd opinion before you change the direction.
> 
> I took a look at your patches for dirty ring and dirty logging modes
> and thought about this some more.
> I think your patch to merge the get and clear dirty log tests is
> great, and I can try to include it and build on it in my series as
> well if desired. I don't think it would be hard to use the same mode
> approach in the dirty log perf test. That said, I think it would be
> easier to keep the functional test (dirty_log_test,
> clear_dirty_log_test) separate from the performance test because the
> dirty log validation is extra time and complexity not needed in the
> dirty log perf test. I did try building them in the same test
> initially, but it was really ugly. Perhaps a future refactoring could
> merge them better.

We can conditionally bypass the validation part.  Let's keep it separate for
now - which is totally fine by me.  Actually I also don't want the dirty ring
series to block your series since I still don't know when it'll land.  That'll
be unnecessary depencency.  Thanks,

-- 
Peter Xu