Message-ID: <976bbe1a-ef16-4006-acac-8b6be46ee5ea@lucifer.local>
Date: Thu, 5 Jun 2025 18:09:09 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Mark Brown <broonie@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>,
        David Hildenbrand <david@...hat.com>, linux-mm@...ck.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in
 gup_longterm

On Thu, Jun 05, 2025 at 05:42:55PM +0100, Mark Brown wrote:
> On Thu, Jun 05, 2025 at 05:26:05PM +0100, Lorenzo Stoakes wrote:
> > On Thu, Jun 05, 2025 at 05:15:51PM +0100, Mark Brown wrote:
>
> > > That's the thing with memfd being special and skipping on setup failure
> > > that David mentioned, I've got a patch as part of the formatting series
> > > I was going to send after the merge window.
>
> > where did he mention this?
>
> I can't remember off hand, sorry.
>

I see his reply here and I guess it's because you now determine the result at
the top level, I see you are doing this in do_test():

+       int result = KSFT_PASS;
        int ret;

+       if (fd < 0) {
+               result = KSFT_FAIL;
+               goto report;
+       }
+

And in run_with_memfd_hugetlb():

        fd = memfd_create("test", flags);
        if (fd < 0) {
-               ksft_test_result_skip("memfd_create() failed (%s)\n", strerror(errno));
-               return;
+               ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));
        }

So previously this would skip, now it fails.

I've not double, triple checked this is it, but seems likely!

You said you had a fix

> > I mean I'd argue that making a test that previously worked now fail due to how
> > somebody's set up their system is a reason not to merge that patch.
>
> Well, it's a bit late now given that this is in Linus' tree and actually
> it turns out this was the only update for gup_longterm so I just rebased
> it onto Linus' tree and kicked off my tests.

Ack.

>
> > Better to do all of these formating fixes and maintain the _same behaviour_ then
> > separately tackle whether or not we should skip.
>
> I'm confused, that's generally the opposite of the standard advice for
> the kernel - usually it's fixes first, then deal with anything cosmetic
> or new?

I mean the crux is that the 'cosmetic' changes also included a 'this might
break things' change.

I'm saying do the cosmetic things in _isolation_, or fix the brokenness
before doing the whole lot.

Anyway there's no point dwelling on this, let's just get a fix sorted.

>
> > Obviously the better option would be to somehow determine if hugetlb is
> > available in advance (of course, theoretically somebody could come in and
> > reserve pages but that's not veyr likely).
>
> The tests do enumerate the set of available hugepage sizes at runtime
> (see the loop in run_test_case()) but detect_hugetlb_page_sizes() just
> looks in /sys/kernel/mm/hugepages/ for subdirectories and doesn't look
> inside those directories to see if there are actually any huge pages
> available for the huge page sizes advertised.  There's probably utility
> in at least a version of that function that checks.

Right yes, I mean obviously this whole thing is a mess already that's not
your fault, and ideally we'd have some general way of looking this up
across _all_ tests and just switch things on/off accordingly.

There's a whole Pandora's box about what the tests should assume/not and
yeah. Anyway. Maybe leave it closed for now :)

Cheers, Lorenzo

Powered by blists - more mailing lists