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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx/ > > > > > > > > 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
