linux-kernel - Re: [PATCH v2 2/2] KVM: selftests: access_tracking_perf_test: add option to skip the sanity check

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-RVDQC4HNxrD-pI@google.com>
Date: Wed, 26 Mar 2025 12:27:09 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org, 
	Muhammad Usama Anjum <usama.anjum@...labora.com>, linux-kernel@...r.kernel.org, 
	Shuah Khan <shuah@...nel.org>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, 
	Oliver Upton <oliver.upton@...ux.dev>, Paolo Bonzini <pbonzini@...hat.com>, 
	linux-kselftest@...r.kernel.org, Anup Patel <anup@...infault.org>
Subject: Re: [PATCH v2 2/2] KVM: selftests: access_tracking_perf_test: add
 option to skip the sanity check

On Wed, Mar 26, 2025, James Houghton wrote:
> On Wed, Mar 26, 2025 at 11:41 AM Sean Christopherson <seanjc@...gle.com> wrote:
> > Then the auto resolving works as below, and as James pointed out, the assert
> > becomes
> >
> >                 TEST_ASSERT(!warn_only, ....);
> 
> I think the auto-resolving below needs to be flipped, and the
> TEST_ASSERT should be for `warn_only`, not `!warn_only`.
> 
> If warn_only == 1, the assert should pass.

/facepalm, yep

> > > > +                       break;
> > > >                 case 'h':
> > > >                 default:
> > > >                         help(argv[0]);
> > > > @@ -386,6 +394,23 @@ int main(int argc, char *argv[])
> > > >         page_idle_fd = open("/sys/kernel/mm/page_idle/bitmap", O_RDWR);
> > > >         __TEST_REQUIRE(page_idle_fd >= 0,
> > > >                        "CONFIG_IDLE_PAGE_TRACKING is not enabled");
> > > > +       if (warn_on_too_many_idle_pages == -1) {
> > > > +#ifdef __x86_64__
> > > > +               if (this_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > > > +                       printf("Skipping idle page count sanity check, because the test is run nested\n");
> > > > +                       warn_on_too_many_idle_pages = 0;
> > > > +               } else
> > > > +#endif
> > > > +               if (is_numa_balancing_enabled()) {
> > > > +                       printf("Skipping idle page count sanity check, because NUMA balance is enabled\n");
> > > > +                       warn_on_too_many_idle_pages = 0;
> > > > +               } else {
> > > > +                       warn_on_too_many_idle_pages = 1;
> > > > +               }
> > > > +       } else if (!warn_on_too_many_idle_pages) {
> > > > +               printf("Skipping idle page count sanity check, because this was requested by the user\n");
> >
> > Eh, I vote to omit this.  The sanity check is still there, it's just degraded to
> > a warn.  I'm not totally against it, just seems superfluous and potentially confusing.
> 
> I agree, it's not adding much.
> 
> Separately: I've finished the MGLRU version of this test. It uses
> MGLRU if it is available, and marking pages as idle is much faster
> when using it. If MGLRU is enabled but otherwise not usable, the test
> fails, as the idle page bitmap is no longer usable for this test.
> 
> I'm happy to post a new version of Maxim's patch with the MGLRU
> patches too, Maxim, if you're okay with that.