Re: [PATCH v3 2/2] selftests: KVM: use dirty logging to check if page stats work correctly

[Mingwei Zhang <mizhang@google.com>]

On Mon, Sep 6, 2021 at 1:05 PM Mingwei Zhang <mizhang@google.com> wrote:
>
> On Tue, Aug 31, 2021 at 9:58 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Aug 30, 2021, Ben Gardon wrote:
> > > On Sun, Aug 29, 2021 at 9:44 PM Mingwei Zhang <mizhang@google.com> wrote:
> > > > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> > > > index af1031fed97f..07eb6b5c125e 100644
> > > > --- a/tools/testing/selftests/kvm/lib/test_util.c
> > > > +++ b/tools/testing/selftests/kvm/lib/test_util.c
> > > > @@ -15,6 +15,13 @@
> > > >  #include "linux/kernel.h"
> > > >
> > > >  #include "test_util.h"
> > > > +#include "processor.h"
> > > > +
> > > > +static const char * const pagestat_filepaths[] = {
> > > > +       "/sys/kernel/debug/kvm/pages_4k",
> > > > +       "/sys/kernel/debug/kvm/pages_2m",
> > > > +       "/sys/kernel/debug/kvm/pages_1g",
> > > > +};
> > >
> > > I think these should only be defined for x86_64 too. Is this the right
> > > file for these definitions or is there an arch specific file they
> > > should go in?
> >
> > The stats also need to be pulled from the selftest's VM, not from the overall KVM
> > stats, otherwise the test will fail if there are any other active VMs on the host,
> > e.g. I like to run to selftests and kvm-unit-tests in parallel.
>
> That is correct. But since this selftest is not the 'default' selftest
> that people normally run, can we make an assumption on running these
> tests at this moment? I am planning to submit this test and improve it
> in the next series by using Jing's fd based KVM stats interface to
> eliminate the assumption of the existence of a single running VM.
> Right now, this interface still needs some work, so I am taking a
> shortcut that directly uses the whole-system metricfs based interface.
>
> But I can choose to do that and submit the fd-based API together with
> this series. What do you suggest?


I will take my point back, since some of the "TEST_ASSERT" in this
selftest does assume that there is no other VM running even on
'default' case (ie., run ./dirty_logging_perf_test without arguments).
Therefore, this patch will make the test flaky.

I will go back to implement the fd based API and submit the code along
with this selftest.

Thanks.
-Mingwei